Bug 1325878: Rewrite insert_rule to avoid lock re-entrancy problems. r?xidorn draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 11 Apr 2017 13:36:20 +0800
changeset 561043 18070b9357120caad8b822b26d7d83a42cb8815c
parent 561042 e0a002731fb565a797dd5028df0f5fd2671d7684
child 561044 83edd541e0cefb1d39bdab4cacdd859600a7ae8c
push id53604
push userbmo:emilio+bugs@crisal.io
push dateWed, 12 Apr 2017 05:05:04 +0000
reviewersxidorn
bugs1325878
milestone55.0a1
Bug 1325878: Rewrite insert_rule to avoid lock re-entrancy problems. r?xidorn In particular, you can insertRule("@import url(\"already-loaded.css\");") and we would try to clone the stylesheet, which without this change would borrow the lock for reading. MozReview-Commit-ID: 4jdNL5rngh7
servo/components/style/stylesheets.rs
servo/ports/geckolib/glue.rs
--- a/servo/components/style/stylesheets.rs
+++ b/servo/components/style/stylesheets.rs
@@ -130,63 +130,16 @@ impl CssRules {
             match *r {
                 CssRule::Namespace(..) |
                 CssRule::Import(..) => true,
                 _ => false
             }
         })
     }
 
-    /// https://drafts.csswg.org/cssom/#insert-a-css-rule
-    pub fn insert_rule(&mut self,
-                       rule: &str,
-                       parent_stylesheet: &Stylesheet,
-                       index: usize,
-                       nested: bool,
-                       loader: Option<&StylesheetLoader>)
-                       -> Result<CssRule, RulesMutateError> {
-        // Step 1, 2
-        if index > self.0.len() {
-            return Err(RulesMutateError::IndexSize);
-        }
-
-        // Computes the parser state at the given index
-        let state = if nested {
-            None
-        } else if index == 0 {
-            Some(State::Start)
-        } else {
-            self.0.get(index - 1).map(CssRule::rule_state)
-        };
-
-        // Step 3, 4
-        // XXXManishearth should we also store the namespace map?
-        let (new_rule, new_state) =
-            try!(CssRule::parse(&rule, parent_stylesheet, state, loader));
-
-        // Step 5
-        // Computes the maximum allowed parser state at a given index.
-        let rev_state = self.0.get(index).map_or(State::Body, CssRule::rule_state);
-        if new_state > rev_state {
-            // We inserted a rule too early, e.g. inserting
-            // a regular style rule before @namespace rules
-            return Err(RulesMutateError::HierarchyRequest);
-        }
-
-        // Step 6
-        if let CssRule::Namespace(..) = new_rule {
-            if !self.only_ns_or_import() {
-                return Err(RulesMutateError::InvalidState);
-            }
-        }
-
-        self.0.insert(index, new_rule.clone());
-        Ok(new_rule)
-    }
-
     /// https://drafts.csswg.org/cssom/#remove-a-css-rule
     pub fn remove_rule(&mut self, index: usize) -> Result<(), RulesMutateError> {
         // Step 1, 2
         if index >= self.0.len() {
             return Err(RulesMutateError::IndexSize);
         }
 
         {
@@ -202,16 +155,96 @@ impl CssRules {
         }
 
         // Step 5, 6
         self.0.remove(index);
         Ok(())
     }
 }
 
+/// A trait to implement helpers for `Arc<Locked<CssRules>>`.
+pub trait CssRulesHelpers {
+    /// https://drafts.csswg.org/cssom/#insert-a-css-rule
+    ///
+    /// Written in this funky way because parsing an @import rule may cause us
+    /// to clone a stylesheet from the same document due to caching in the CSS
+    /// loader.
+    ///
+    /// TODO(emilio): We could also pass the write guard down into the loader
+    /// instead, but that seems overkill.
+    fn insert_rule(&self,
+                   lock: &SharedRwLock,
+                   rule: &str,
+                   parent_stylesheet: &Stylesheet,
+                   index: usize,
+                   nested: bool,
+                   loader: Option<&StylesheetLoader>)
+                   -> Result<CssRule, RulesMutateError>;
+}
+
+impl CssRulesHelpers for Arc<Locked<CssRules>> {
+    fn insert_rule(&self,
+                   lock: &SharedRwLock,
+                   rule: &str,
+                   parent_stylesheet: &Stylesheet,
+                   index: usize,
+                   nested: bool,
+                   loader: Option<&StylesheetLoader>)
+                   -> Result<CssRule, RulesMutateError> {
+        let state = {
+            let read_guard = lock.read();
+            let rules = self.read_with(&read_guard);
+
+            // Step 1, 2
+            if index > rules.0.len() {
+                return Err(RulesMutateError::IndexSize);
+            }
+
+            // Computes the parser state at the given index
+            if nested {
+                None
+            } else if index == 0 {
+                Some(State::Start)
+            } else {
+                rules.0.get(index - 1).map(CssRule::rule_state)
+            }
+        };
+
+        // Step 3, 4
+        // XXXManishearth should we also store the namespace map?
+        let (new_rule, new_state) =
+            try!(CssRule::parse(&rule, parent_stylesheet, state, loader));
+
+        {
+            let mut write_guard = lock.write();
+            let mut rules = self.write_with(&mut write_guard);
+            // Step 5
+            // Computes the maximum allowed parser state at a given index.
+            let rev_state = rules.0.get(index).map_or(State::Body, CssRule::rule_state);
+            if new_state > rev_state {
+                // We inserted a rule too early, e.g. inserting
+                // a regular style rule before @namespace rules
+                return Err(RulesMutateError::HierarchyRequest);
+            }
+
+            // Step 6
+            if let CssRule::Namespace(..) = new_rule {
+                if !rules.only_ns_or_import() {
+                    return Err(RulesMutateError::InvalidState);
+                }
+            }
+
+            rules.0.insert(index, new_rule.clone());
+        }
+
+        Ok(new_rule)
+    }
+
+}
+
 /// The structure servo uses to represent a stylesheet.
 #[derive(Debug)]
 pub struct Stylesheet {
     /// List of rules in the order they were found (important for
     /// cascading order)
     pub rules: Arc<Locked<CssRules>>,
     /// List of media associated with the Stylesheet.
     pub media: Arc<Locked<MediaList>>,
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -78,18 +78,19 @@ use style::properties::SKIP_ROOT_AND_ITE
 use style::properties::animated_properties::{AnimationValue, Interpolate, TransitionProperty};
 use style::properties::parse_one_declaration;
 use style::restyle_hints::{self, RestyleHint};
 use style::rule_tree::StyleSource;
 use style::selector_parser::PseudoElementCascadeType;
 use style::sequential;
 use style::shared_lock::{SharedRwLock, SharedRwLockReadGuard, StylesheetGuards, ToCssWithGuard, Locked};
 use style::string_cache::Atom;
-use style::stylesheets::{CssRule, CssRules, CssRuleType, ImportRule, MediaRule, NamespaceRule, PageRule};
-use style::stylesheets::{Origin, Stylesheet, StyleRule};
+use style::stylesheets::{CssRule, CssRules, CssRuleType, CssRulesHelpers};
+use style::stylesheets::{ImportRule, MediaRule, NamespaceRule, Origin};
+use style::stylesheets::{PageRule, Stylesheet, StyleRule};
 use style::stylesheets::StylesheetLoader as StyleStylesheetLoader;
 use style::supports::parse_condition_or_declaration;
 use style::thread_state;
 use style::timer::Timer;
 use style::traversal::{ANIMATION_ONLY, FOR_RECONSTRUCT, UNSTYLED_CHILDREN_ONLY};
 use style::traversal::{resolve_style, DomTraversal, TraversalDriver, TraversalFlags};
 use style_traits::ToCss;
 use super::stylesheet_loader::StylesheetLoader;
@@ -656,25 +657,30 @@ pub extern "C" fn Servo_CssRules_InsertR
     let sheet = Stylesheet::as_arc(&sheet);
     let loader = if loader.is_null() {
         None
     } else {
         Some(StylesheetLoader::new(loader, gecko_stylesheet))
     };
     let loader = loader.as_ref().map(|loader| loader as &StyleStylesheetLoader);
     let rule = unsafe { rule.as_ref().unwrap().as_str_unchecked() };
-    write_locked_arc(rules, |rules: &mut CssRules| {
-        match rules.insert_rule(rule, sheet, index as usize, nested, loader) {
-            Ok(new_rule) => {
-                *unsafe { rule_type.as_mut().unwrap() } = new_rule.rule_type() as u16;
-                nsresult::NS_OK
-            }
-            Err(err) => err.into()
+
+    let global_style_data = &*GLOBAL_STYLE_DATA;
+    match Locked::<CssRules>::as_arc(&rules).insert_rule(&global_style_data.shared_lock,
+                                                         rule,
+                                                         sheet,
+                                                         index as usize,
+                                                         nested,
+                                                         loader) {
+        Ok(new_rule) => {
+            *unsafe { rule_type.as_mut().unwrap() } = new_rule.rule_type() as u16;
+            nsresult::NS_OK
         }
-    })
+        Err(err) => err.into(),
+    }
 }
 
 #[no_mangle]
 pub extern "C" fn Servo_CssRules_DeleteRule(rules: ServoCssRulesBorrowed, index: u32) -> nsresult {
     write_locked_arc(rules, |rules: &mut CssRules| {
         match rules.remove_rule(index as usize) {
             Ok(_) => nsresult::NS_OK,
             Err(err) => err.into()