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
--- 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()