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
Signed-off-by: Emilio Cobos Álvarez <emilio@crisal.io>

components/style/stylesheets.rs

@@ -135,53 +135,6 @@ impl CssRules {
         })
     }
 
-    /// 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
@@ -207,6 +160,86 @@ impl CssRules {
     }
 }
 
+/// 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 {

ports/geckolib/glue.rs

@@ -83,8 +83,9 @@ 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::{CssRule, CssRules, CssRuleType, CssRulesHelpers};
-use style::stylesheets::{Origin, Stylesheet, StyleRule};
+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;
@@ -698,15 +699,20 @@ pub extern "C" fn Servo_CssRules_InsertRule(rules: ServoCssRulesBorrowed,
     };
     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) {
+    let global_style_data = &*GLOBAL_STYLE_DATA;
-            Ok(new_rule) => {
+    match Locked::<CssRules>::as_arc(&rules).insert_rule(&global_style_data.shared_lock,
-                *unsafe { rule_type.as_mut().unwrap() } = new_rule.rule_type() as u16;
+                                                         rule,
-                nsresult::NS_OK
+                                                         sheet,
-            }
+                                                         index as usize,
-            Err(err) => err.into()
+                                                         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]