stylo: Add uses of fallible Vec, SmallVec and HashMap facilities.

Bug: 1395064
Reviewed-by: emilio

Cargo.lock

@@ -976,6 +976,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 name = "fallible"
 version = "0.0.1"
 dependencies = [
+ "hashglobe 0.1.0",
  "smallvec 0.4.3 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
 

components/fallible/Cargo.toml

@@ -11,3 +11,4 @@ path = "lib.rs"
 
 [dependencies]
 smallvec = "0.4"
+hashglobe = { path = "../hashglobe" }

components/fallible/lib.rs

@@ -2,8 +2,10 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+extern crate hashglobe;
 extern crate smallvec;
 
+use hashglobe::FailedAllocationError;
 use smallvec::Array;
 use smallvec::SmallVec;
 use std::mem;
@@ -17,8 +19,8 @@ extern "C" {
 
 pub trait FallibleVec<T> {
     /// Append |val| to the end of |vec|.  Returns Ok(()) on success,
-    /// Err(()) if it fails, which can only be due to lack of memory.
-    fn try_push(&mut self, value: T) -> Result<(), ()>;
+    /// Err(reason) if it fails, with |reason| describing the failure.
+    fn try_push(&mut self, value: T) -> Result<(), FailedAllocationError>;
 }
 
 
@@ -27,7 +29,7 @@ pub trait FallibleVec<T> {
 
 impl<T> FallibleVec<T> for Vec<T> {
     #[inline]
-    fn try_push(&mut self, val: T) -> Result<(), ()> {
+    fn try_push(&mut self, val: T) -> Result<(), FailedAllocationError> {
         if self.capacity() == self.len() {
             try_double_vec(self)?;
             debug_assert!(self.capacity() > self.len());
@@ -38,19 +40,25 @@ impl<T> FallibleVec<T> for Vec<T> {
 }
 
 // Double the capacity of |vec|, or fail to do so due to lack of memory.
-// Returns Ok(()) on success, Err(()) on failure.
+// Returns Ok(()) on success, Err(..) on failure.
 #[inline(never)]
 #[cold]
-fn try_double_vec<T>(vec: &mut Vec<T>) -> Result<(), ()> {
+fn try_double_vec<T>(vec: &mut Vec<T>) -> Result<(), FailedAllocationError> {
     let old_ptr = vec.as_mut_ptr();
     let old_len = vec.len();
 
     let old_cap: usize = vec.capacity();
-    let new_cap: usize =
-        if old_cap == 0 { 4 } else { old_cap.checked_mul(2).ok_or(()) ? };
+    let new_cap: usize = if old_cap == 0 {
+        4
+    } else {
+        old_cap.checked_mul(2).ok_or(FailedAllocationError::new(
+            "capacity overflow for Vec",
+        ))?
+    };
 
-    let new_size_bytes =
-        new_cap.checked_mul(mem::size_of::<T>()).ok_or(()) ? ;
+    let new_size_bytes = new_cap.checked_mul(mem::size_of::<T>()).ok_or(
+        FailedAllocationError::new("capacity overflow for Vec"),
+    )?;
 
     let new_ptr = unsafe {
         if old_cap == 0 {
@@ -61,7 +69,9 @@ fn try_double_vec<T>(vec: &mut Vec<T>) -> Result<(), ()> {
     };
 
     if new_ptr.is_null() {
-        return Err(());
+        return Err(FailedAllocationError::new(
+            "out of memory when allocating Vec",
+        ));
     }
 
     let new_vec = unsafe {
@@ -78,7 +88,7 @@ fn try_double_vec<T>(vec: &mut Vec<T>) -> Result<(), ()> {
 
 impl<T: Array> FallibleVec<T::Item> for SmallVec<T> {
     #[inline]
-    fn try_push(&mut self, val: T::Item) -> Result<(), ()> {
+    fn try_push(&mut self, val: T::Item) -> Result<(), FailedAllocationError> {
         if self.capacity() == self.len() {
             try_double_small_vec(self)?;
             debug_assert!(self.capacity() > self.len());
@@ -88,11 +98,12 @@ impl<T: Array> FallibleVec<T::Item> for SmallVec<T> {
     }
 }
 
-// Double the capacity of |vec|, or fail to do so due to lack of memory.
-// Returns Ok(()) on success, Err(()) on failure.
+// Double the capacity of |svec|, or fail to do so due to lack of memory.
+// Returns Ok(()) on success, Err(..) on failure.
 #[inline(never)]
 #[cold]
-fn try_double_small_vec<T>(svec: &mut SmallVec<T>) -> Result<(), ()>
+fn try_double_small_vec<T>(svec: &mut SmallVec<T>)
+-> Result<(), FailedAllocationError>
 where
     T: Array,
 {
@@ -100,16 +111,23 @@ where
     let old_len = svec.len();
 
     let old_cap: usize = svec.capacity();
-    let new_cap: usize =
-        if old_cap == 0 { 4 } else { old_cap.checked_mul(2).ok_or(()) ? };
+    let new_cap: usize = if old_cap == 0 {
+        4
+    } else {
+        old_cap.checked_mul(2).ok_or(FailedAllocationError::new(
+            "capacity overflow for SmallVec",
+        ))?
+    };
 
     // This surely shouldn't fail, if |old_cap| was previously accepted as a
     // valid value.  But err on the side of caution.
-    let old_size_bytes =
-        old_cap.checked_mul(mem::size_of::<T>()).ok_or(()) ? ;
+    let old_size_bytes = old_cap.checked_mul(mem::size_of::<T>()).ok_or(
+        FailedAllocationError::new("capacity overflow for SmallVec"),
+    )?;
 
-    let new_size_bytes =
-        new_cap.checked_mul(mem::size_of::<T>()).ok_or(()) ? ;
+    let new_size_bytes = new_cap.checked_mul(mem::size_of::<T>()).ok_or(
+        FailedAllocationError::new("capacity overflow for SmallVec"),
+    )?;
 
     let new_ptr;
     if svec.spilled() {
@@ -130,7 +148,9 @@ where
     }
 
     if new_ptr.is_null() {
-        return Err(());
+        return Err(FailedAllocationError::new(
+            "out of memory when allocating SmallVec",
+        ));
     }
 
     let new_vec = unsafe {

components/hashglobe/src/lib.rs

@@ -33,6 +33,12 @@ pub struct FailedAllocationError {
     reason: &'static str,
 }
 
+impl FailedAllocationError {
+    pub fn new(reason: &'static str) -> Self {
+        Self { reason }
+    }
+}
+
 impl error::Error for FailedAllocationError {
     fn description(&self) -> &str {
         self.reason

components/style/invalidation/element/invalidation_map.rs

@@ -7,6 +7,8 @@
 use {Atom, LocalName, Namespace};
 use context::QuirksMode;
 use element_state::ElementState;
+use fallible::FallibleVec;
+use hashglobe::FailedAllocationError;
 use selector_map::{MaybeCaseInsensitiveHashMap, SelectorMap, SelectorMapEntry};
 use selector_parser::SelectorImpl;
 use selectors::attr::NamespaceConstraint;
@@ -184,12 +186,13 @@ impl InvalidationMap {
         })
     }
 
-    /// Adds a selector to this `InvalidationMap`.
+    /// Adds a selector to this `InvalidationMap`.  Returns Err(..) to
+    /// signify OOM.
     pub fn note_selector(
         &mut self,
         selector: &Selector<SelectorImpl>,
-        quirks_mode: QuirksMode)
-    {
+        quirks_mode: QuirksMode
+    ) -> Result<(), FailedAllocationError> {
         self.collect_invalidations_for(selector, quirks_mode)
     }
 
@@ -203,11 +206,12 @@ impl InvalidationMap {
         self.has_class_attribute_selectors = false;
     }
 
+    // Returns Err(..) to signify OOM.
     fn collect_invalidations_for(
         &mut self,
         selector: &Selector<SelectorImpl>,
-        quirks_mode: QuirksMode)
-    {
+        quirks_mode: QuirksMode
+    ) -> Result<(), FailedAllocationError> {
         debug!("InvalidationMap::collect_invalidations_for({:?})", selector);
 
         let mut iter = selector.iter();
@@ -246,20 +250,20 @@ impl InvalidationMap {
                 self.class_to_selector
                     .entry(class, quirks_mode)
                     .or_insert_with(SmallVec::new)
-                    .push(Dependency {
+                    .try_push(Dependency {
                         selector: selector.clone(),
                         selector_offset: sequence_start,
-                    })
+                    })?;
             }
 
             for id in compound_visitor.ids {
                 self.id_to_selector
                     .entry(id, quirks_mode)
                     .or_insert_with(SmallVec::new)
-                    .push(Dependency {
+                    .try_push(Dependency {
                         selector: selector.clone(),
                         selector_offset: sequence_start,
-                    })
+                    })?;
             }
 
             if !compound_visitor.state.is_empty() {
@@ -270,7 +274,7 @@ impl InvalidationMap {
                             selector_offset: sequence_start,
                         },
                         state: compound_visitor.state,
-                    }, quirks_mode);
+                    }, quirks_mode)?;
             }
 
             if compound_visitor.other_attributes {
@@ -278,7 +282,7 @@ impl InvalidationMap {
                     .insert(Dependency {
                         selector: selector.clone(),
                         selector_offset: sequence_start,
-                    }, quirks_mode);
+                    }, quirks_mode)?;
             }
 
             combinator = iter.next_sequence();
@@ -288,6 +292,8 @@ impl InvalidationMap {
 
             index += 1; // Account for the combinator.
         }
+
+        Ok(())
     }
 
     /// Measures heap usage.

components/style/lib.rs

@@ -47,6 +47,7 @@ extern crate bitflags;
 #[cfg(feature = "gecko")] #[macro_use] #[no_link] extern crate cfg_if;
 #[macro_use] extern crate cssparser;
 extern crate euclid;
+extern crate fallible;
 extern crate fnv;
 #[cfg(feature = "gecko")] #[macro_use] pub mod gecko_string_cache;
 extern crate hashglobe;

components/style/selector_map.rs

@@ -9,8 +9,10 @@ use {Atom, LocalName};
 use applicable_declarations::ApplicableDeclarationBlock;
 use context::QuirksMode;
 use dom::TElement;
+use fallible::FallibleVec;
 use hash::{HashMap, HashSet};
 use hash::map as hash_map;
+use hashglobe::FailedAllocationError;
 use pdqsort::sort_by;
 use precomputed_hash::PrecomputedHash;
 use rule_tree::CascadeLevel;
@@ -272,18 +274,20 @@ impl SelectorMap<Rule> {
 
 impl<T: SelectorMapEntry> SelectorMap<T> {
     /// Inserts into the correct hash, trying id, class, and localname.
-    pub fn insert(&mut self, entry: T, quirks_mode: QuirksMode) {
+    pub fn insert(
+        &mut self,
+        entry: T,
+        quirks_mode: QuirksMode
+    ) -> Result<(), FailedAllocationError> {
         self.count += 1;
 
         let vector = match find_bucket(entry.selector()) {
             Bucket::ID(id) => {
-                self.id_hash
-                    .entry(id.clone(), quirks_mode)
+                self.id_hash.try_entry(id.clone(), quirks_mode)?
                     .or_insert_with(SmallVec::new)
             }
             Bucket::Class(class) => {
-                self.class_hash
-                    .entry(class.clone(), quirks_mode)
+                self.class_hash.try_entry(class.clone(), quirks_mode)?
                     .or_insert_with(SmallVec::new)
             }
             Bucket::LocalName { name, lower_name } => {
@@ -300,12 +304,11 @@ impl<T: SelectorMapEntry> SelectorMap<T> {
                 // subsequent selector matching work will filter them out.
                 if name != lower_name {
                     self.local_name_hash
-                        .entry(lower_name.clone())
+                        .try_entry(lower_name.clone())?
                         .or_insert_with(SmallVec::new)
-                        .push(entry.clone());
+                        .try_push(entry.clone())?;
                 }
-                self.local_name_hash
-                    .entry(name.clone())
+                self.local_name_hash.try_entry(name.clone())?
                     .or_insert_with(SmallVec::new)
             }
             Bucket::Universal => {
@@ -313,7 +316,7 @@ impl<T: SelectorMapEntry> SelectorMap<T> {
             }
         };
 
-        vector.push(entry);
+        vector.try_push(entry)
     }
 
     /// Looks up entries by id, class, local name, and other (in order).
@@ -510,6 +513,18 @@ impl<V: 'static> MaybeCaseInsensitiveHashMap<Atom, V> {
         self.0.entry(key)
     }
 
+    /// HashMap::try_entry
+    pub fn try_entry(
+        &mut self,
+        mut key: Atom,
+        quirks_mode: QuirksMode
+    ) -> Result<hash_map::Entry<Atom, V>, FailedAllocationError> {
+        if quirks_mode == QuirksMode::Quirks {
+            key = key.to_ascii_lowercase()
+        }
+        self.0.try_entry(key)
+    }
+
     /// HashMap::iter
     pub fn iter(&self) -> hash_map::Iter<Atom, V> {
         self.0.iter()

components/style/stylesheets/stylesheet.rs

@@ -6,6 +6,7 @@ use {Prefix, Namespace};
 use context::QuirksMode;
 use cssparser::{Parser, RuleListParser, ParserInput};
 use error_reporting::{ParseErrorReporter, ContextualParseError};
+use fallible::FallibleVec;
 use fnv::FnvHashMap;
 use invalidation::media_queries::{MediaListKey, ToMediaListKey};
 use media_queries::{MediaList, Device};
@@ -382,7 +383,14 @@ impl Stylesheet {
 
             while let Some(result) = iter.next() {
                 match result {
-                    Ok(rule) => rules.push(rule),
+                    Ok(rule) => {
+                        // Use a fallible push here, and if it fails, just
+                        // fall out of the loop.  This will cause the page to
+                        // be shown incorrectly, but it's better than OOMing.
+                        if rules.try_push(rule).is_err() {
+                            break;
+                        }
+                    },
                     Err(err) => {
                         let error = ContextualParseError::InvalidRule(err.slice, err.error);
                         iter.parser.context.log_css_error(&iter.parser.error_context,

components/style/stylist.rs

@@ -13,6 +13,7 @@ use element_state::ElementState;
 use font_metrics::FontMetricsProvider;
 #[cfg(feature = "gecko")]
 use gecko_bindings::structs::{nsIAtom, ServoStyleSetSizes, StyleRuleInclusion};
+use hashglobe::FailedAllocationError;
 use invalidation::element::invalidation_map::InvalidationMap;
 use invalidation::media_queries::{EffectiveMediaQueryResults, ToMediaListKey};
 use media_queries::Device;
@@ -87,7 +88,8 @@ impl DocumentCascadeData {
     }
 
     /// Rebuild the cascade data for the given document stylesheets, and
-    /// optionally with a set of user agent stylesheets.
+    /// optionally with a set of user agent stylesheets.  Returns Err(..)
+    /// to signify OOM.
     fn rebuild<'a, 'b, S>(
         &mut self,
         device: &Device,
@@ -96,7 +98,7 @@ impl DocumentCascadeData {
         guards: &StylesheetGuards,
         ua_stylesheets: Option<&UserAgentStylesheets>,
         extra_data: &mut PerOrigin<ExtraStyleData>,
-    )
+    ) -> Result<(), FailedAllocationError>
     where
         'b: 'a,
         S: StylesheetInDocument + ToMediaListKey + PartialEq + 'static,
@@ -154,7 +156,7 @@ impl DocumentCascadeData {
                     guards.ua_or_user,
                     extra_data,
                     SheetRebuildKind::Full,
-                );
+                )?;
             }
 
             if quirks_mode != QuirksMode::NoQuirks {
@@ -183,7 +185,7 @@ impl DocumentCascadeData {
                         guards.ua_or_user,
                         extra_data,
                         SheetRebuildKind::Full,
-                    );
+                    )?;
                 }
             }
         }
@@ -196,10 +198,13 @@ impl DocumentCascadeData {
                 guards.author,
                 extra_data,
                 rebuild_kind,
-            );
-        }
+            )?;
         }
 
+        Ok(())
+    }
+
+    // Returns Err(..) to signify OOM
     fn add_stylesheet<S>(
         &mut self,
         device: &Device,
@@ -208,13 +213,13 @@ impl DocumentCascadeData {
         guard: &SharedRwLockReadGuard,
         _extra_data: &mut PerOrigin<ExtraStyleData>,
         rebuild_kind: SheetRebuildKind,
-    )
+    ) -> Result<(), FailedAllocationError>
     where
         S: StylesheetInDocument + ToMediaListKey + 'static,
     {
         if !stylesheet.enabled() ||
            !stylesheet.is_effective_for_device(device, guard) {
-            return;
+            return Ok(());
         }
 
         let origin = stylesheet.origin(guard);
@@ -277,12 +282,12 @@ impl DocumentCascadeData {
                             origin_cascade_data.rules_source_order
                         );
 
-                        map.insert(rule, quirks_mode);
+                        map.insert(rule, quirks_mode)?;
 
                         if rebuild_kind.should_rebuild_invalidation() {
                             origin_cascade_data
                                 .invalidation_map
-                                .note_selector(selector, quirks_mode);
+                                .note_selector(selector, quirks_mode)?;
                             let mut visitor = StylistSelectorVisitor {
                                 needs_revalidation: false,
                                 passed_rightmost_selector: false,
@@ -298,7 +303,7 @@ impl DocumentCascadeData {
                                 origin_cascade_data.selectors_for_cache_revalidation.insert(
                                     RevalidationSelectorAndHashes::new(selector.clone(), hashes),
                                     quirks_mode
-                                );
+                                )?;
                             }
                         }
                     }
@@ -336,7 +341,8 @@ impl DocumentCascadeData {
                         let animation = KeyframesAnimation::from_keyframes(
                             &keyframes_rule.keyframes, keyframes_rule.vendor_prefix.clone(), guard);
                         debug!("Found valid keyframe animation: {:?}", animation);
-                        origin_cascade_data.animations.insert(keyframes_rule.name.as_atom().clone(), animation);
+                        origin_cascade_data.animations
+                            .try_insert(keyframes_rule.name.as_atom().clone(), animation)?;
                     }
                 }
                 #[cfg(feature = "gecko")]
@@ -367,6 +373,8 @@ impl DocumentCascadeData {
                 _ => {}
             }
         }
+
+        Ok(())
     }
 
     /// Measures heap usage.
@@ -604,7 +612,7 @@ impl Stylist {
             guards,
             ua_sheets,
             extra_data,
-        );
+        ).unwrap_or_else(|_| warn!("OOM in Stylist::flush"));
 
         had_invalidations
     }

tests/unit/style/stylist.rs

@@ -169,9 +169,11 @@ fn test_rule_ordering_same_specificity() {
 fn test_insert() {
     let (rules_list, _) = get_mock_rules(&[".intro.foo", "#top"]);
     let mut selector_map = SelectorMap::new();
-    selector_map.insert(rules_list[1][0].clone(), QuirksMode::NoQuirks);
+    selector_map.insert(rules_list[1][0].clone(), QuirksMode::NoQuirks)
+                .expect("OOM");
     assert_eq!(1, selector_map.id_hash.get(&Atom::from("top"), QuirksMode::NoQuirks).unwrap()[0].source_order);
-    selector_map.insert(rules_list[0][0].clone(), QuirksMode::NoQuirks);
+    selector_map.insert(rules_list[0][0].clone(), QuirksMode::NoQuirks)
+                .expect("OOM");
     assert_eq!(0, selector_map.class_hash.get(&Atom::from("foo"), QuirksMode::NoQuirks).unwrap()[0].source_order);
     assert!(selector_map.class_hash.get(&Atom::from("intro"), QuirksMode::NoQuirks).is_none());
 }