Bug 1464865: Don't let @namespace rules that aren't going to be inserted affect the namespace map. r=xidorn
authorEmilio Cobos Álvarez <emilio@crisal.io>
Mon, 28 May 2018 23:56:20 +0200
changeset 420288 09d9ee3fe4bbe37634ead56e93241e761326af89
parent 420287 1268d562bda02ec1084c64679ec201783b07c218
child 420289 4d23f192ffe345cf60527325bb56bc7d4f27c4a4
push id34069
push usernerli@mozilla.com
push dateTue, 29 May 2018 21:42:06 +0000
treeherdermozilla-central@89d79c2258be [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersxidorn
bugs1464865
milestone62.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1464865: Don't let @namespace rules that aren't going to be inserted affect the namespace map. r=xidorn MozReview-Commit-ID: 9bjlEBExqsr
servo/components/style/stylesheets/mod.rs
servo/components/style/stylesheets/rule_list.rs
servo/components/style/stylesheets/rule_parser.rs
servo/components/style/stylesheets/stylesheet.rs
testing/web-platform/meta/MANIFEST.json
testing/web-platform/tests/css/cssom/at-namespace.html
--- a/servo/components/style/stylesheets/mod.rs
+++ b/servo/components/style/stylesheets/mod.rs
@@ -41,17 +41,17 @@ pub use self::font_face_rule::FontFaceRu
 pub use self::font_feature_values_rule::FontFeatureValuesRule;
 pub use self::import_rule::ImportRule;
 pub use self::keyframes_rule::KeyframesRule;
 pub use self::loader::StylesheetLoader;
 pub use self::media_rule::MediaRule;
 pub use self::namespace_rule::NamespaceRule;
 pub use self::origin::{Origin, OriginSet, OriginSetIterator, PerOrigin, PerOriginIter};
 pub use self::page_rule::PageRule;
-pub use self::rule_parser::{State, TopLevelRuleParser};
+pub use self::rule_parser::{State, TopLevelRuleParser, InsertRuleContext};
 pub use self::rule_list::{CssRules, CssRulesHelpers};
 pub use self::rules_iterator::{AllRules, EffectiveRules};
 pub use self::rules_iterator::{NestedRuleIterationCondition, RulesIterator};
 pub use self::stylesheet::{DocumentStyleSheet, Namespaces, Stylesheet};
 pub use self::stylesheet::{StylesheetContents, StylesheetInDocument, UserAgentStylesheets};
 pub use self::style_rule::StyleRule;
 pub use self::supports_rule::SupportsRule;
 pub use self::viewport_rule::ViewportRule;
@@ -174,38 +174,23 @@ pub enum CssRuleType {
     Document = 13,
     // https://drafts.csswg.org/css-fonts-3/#om-fontfeaturevalues
     FontFeatureValues = 14,
     // https://drafts.csswg.org/css-device-adapt/#css-rule-interface
     Viewport = 15,
 }
 
 #[allow(missing_docs)]
-pub enum SingleRuleParseError {
-    Syntax,
-    Hierarchy,
-}
-
-#[allow(missing_docs)]
 pub enum RulesMutateError {
     Syntax,
     IndexSize,
     HierarchyRequest,
     InvalidState,
 }
 
-impl From<SingleRuleParseError> for RulesMutateError {
-    fn from(other: SingleRuleParseError) -> Self {
-        match other {
-            SingleRuleParseError::Syntax => RulesMutateError::Syntax,
-            SingleRuleParseError::Hierarchy => RulesMutateError::HierarchyRequest,
-        }
-    }
-}
-
 impl CssRule {
     /// Returns the CSSOM rule type of this rule.
     pub fn rule_type(&self) -> CssRuleType {
         match *self {
             CssRule::Style(_) => CssRuleType::Style,
             CssRule::Import(_) => CssRuleType::Import,
             CssRule::Media(_) => CssRuleType::Media,
             CssRule::FontFace(_) => CssRuleType::FontFace,
@@ -231,59 +216,55 @@ impl CssRule {
 
     /// Parse a CSS rule.
     ///
     /// Returns a parsed CSS rule and the final state of the parser.
     ///
     /// Input state is None for a nested rule
     pub fn parse(
         css: &str,
+        insert_rule_context: InsertRuleContext,
         parent_stylesheet_contents: &StylesheetContents,
         shared_lock: &SharedRwLock,
-        state: Option<State>,
+        state: State,
         loader: Option<&StylesheetLoader>,
-    ) -> Result<(Self, State), SingleRuleParseError> {
+    ) -> Result<Self, RulesMutateError> {
         let url_data = parent_stylesheet_contents.url_data.read();
         let error_reporter = NullReporter;
         let context = ParserContext::new(
             parent_stylesheet_contents.origin,
             &url_data,
             None,
             ParsingMode::DEFAULT,
             parent_stylesheet_contents.quirks_mode,
         );
 
         let mut input = ParserInput::new(css);
         let mut input = Parser::new(&mut input);
 
         let mut guard = parent_stylesheet_contents.namespaces.write();
 
         // nested rules are in the body state
-        let state = state.unwrap_or(State::Body);
         let mut rule_parser = TopLevelRuleParser {
             stylesheet_origin: parent_stylesheet_contents.origin,
-            context: context,
+            context,
             error_context: ParserErrorContext {
                 error_reporter: &error_reporter,
             },
             shared_lock: &shared_lock,
-            loader: loader,
-            state: state,
-            had_hierarchy_error: false,
+            loader,
+            state,
+            dom_error: None,
             namespaces: &mut *guard,
+            insert_rule_context: Some(insert_rule_context),
         };
 
         parse_one_rule(&mut input, &mut rule_parser)
-            .map(|result| (result, rule_parser.state))
             .map_err(|_| {
-                if rule_parser.take_had_hierarchy_error() {
-                    SingleRuleParseError::Hierarchy
-                } else {
-                    SingleRuleParseError::Syntax
-                }
+                rule_parser.dom_error.unwrap_or(RulesMutateError::Syntax)
             })
     }
 }
 
 impl DeepCloneWithLock for CssRule {
     /// Deep clones this CssRule.
     fn deep_clone_with_lock(
         &self,
--- a/servo/components/style/stylesheets/rule_list.rs
+++ b/servo/components/style/stylesheets/rule_list.rs
@@ -8,17 +8,17 @@
 use malloc_size_of::{MallocShallowSizeOf, MallocSizeOfOps};
 use servo_arc::{Arc, RawOffsetArc};
 use shared_lock::{DeepCloneParams, DeepCloneWithLock, Locked};
 use shared_lock::{SharedRwLock, SharedRwLockReadGuard, ToCssWithGuard};
 use std::fmt::{self, Write};
 use str::CssStringWriter;
 use stylesheets::{CssRule, RulesMutateError};
 use stylesheets::loader::StylesheetLoader;
-use stylesheets::rule_parser::State;
+use stylesheets::rule_parser::{InsertRuleContext, State};
 use stylesheets::stylesheet::StylesheetContents;
 
 /// A list of CSS rules.
 #[derive(Debug)]
 pub struct CssRules(pub Vec<CssRule>);
 
 impl CssRules {
     /// Whether this CSS rules is empty.
@@ -136,57 +136,51 @@ impl CssRulesHelpers for RawOffsetArc<Lo
         &self,
         lock: &SharedRwLock,
         rule: &str,
         parent_stylesheet_contents: &StylesheetContents,
         index: usize,
         nested: bool,
         loader: Option<&StylesheetLoader>,
     ) -> Result<CssRule, RulesMutateError> {
-        let state = {
+        let new_rule = {
             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
+            let state = if nested {
+                State::Body
             } else if index == 0 {
-                Some(State::Start)
+                State::Start
             } else {
-                rules.0.get(index - 1).map(CssRule::rule_state)
-            }
-        };
+                rules.0.get(index - 1).map(CssRule::rule_state).unwrap_or(State::Body)
+            };
+
+            let insert_rule_context = InsertRuleContext {
+                rule_list: &rules.0,
+                index,
+            };
 
-        // Step 3, 4
-        // XXXManishearth should we also store the namespace map?
-        let (new_rule, new_state) =
-            CssRule::parse(&rule, parent_stylesheet_contents, lock, state, loader)?;
+            // Steps 3, 4, 5, 6
+            CssRule::parse(
+                &rule,
+                insert_rule_context,
+                parent_stylesheet_contents,
+                lock,
+                state,
+                loader,
+            )?
+        };
 
         {
             let mut write_guard = lock.write();
             let 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)
     }
 }
--- a/servo/components/style/stylesheets/rule_parser.rs
+++ b/servo/components/style/stylesheets/rule_parser.rs
@@ -14,28 +14,36 @@ use media_queries::{parse_media_query_li
 use parser::{Parse, ParserContext, ParserErrorContext};
 use properties::parse_property_declaration_list;
 use selector_parser::{SelectorImpl, SelectorParser};
 use selectors::SelectorList;
 use servo_arc::Arc;
 use shared_lock::{Locked, SharedRwLock};
 use str::starts_with_ignore_ascii_case;
 use style_traits::{ParseError, StyleParseErrorKind};
-use stylesheets::{CssRule, CssRuleType, CssRules, Origin, StylesheetLoader};
+use stylesheets::{CssRule, CssRuleType, CssRules, Origin, RulesMutateError, StylesheetLoader};
 use stylesheets::{DocumentRule, FontFeatureValuesRule, KeyframesRule, MediaRule};
 use stylesheets::{NamespaceRule, PageRule, StyleRule, SupportsRule, ViewportRule};
 use stylesheets::document_rule::DocumentCondition;
 use stylesheets::font_feature_values_rule::parse_family_name_list;
 use stylesheets::keyframes_rule::parse_keyframe_list;
 use stylesheets::stylesheet::Namespaces;
 use stylesheets::supports_rule::SupportsCondition;
 use stylesheets::viewport_rule;
 use values::{CssUrl, CustomIdent, KeyframesName};
 use values::computed::font::FamilyName;
 
+/// The information we need particularly to do CSSOM insertRule stuff.
+pub struct InsertRuleContext<'a> {
+    /// The rule list we're about to insert into.
+    pub rule_list: &'a [CssRule],
+    /// The index we're about to get inserted at.
+    pub index: usize,
+}
+
 /// The parser for the top-level rules in a stylesheet.
 pub struct TopLevelRuleParser<'a, R: 'a> {
     /// The origin of the stylesheet we're parsing.
     pub stylesheet_origin: Origin,
     /// A reference to the lock we need to use to create rules.
     pub shared_lock: &'a SharedRwLock,
     /// A reference to a stylesheet loader if applicable, for `@import` rules.
     pub loader: Option<&'a StylesheetLoader>,
@@ -46,21 +54,23 @@ pub struct TopLevelRuleParser<'a, R: 'a>
     pub context: ParserContext<'a>,
     /// The context required for reporting parse errors.
     pub error_context: ParserErrorContext<'a, R>,
     /// The current state of the parser.
     pub state: State,
     /// Whether we have tried to parse was invalid due to being in the wrong
     /// place (e.g. an @import rule was found while in the `Body` state). Reset
     /// to `false` when `take_had_hierarchy_error` is called.
-    pub had_hierarchy_error: bool,
+    pub dom_error: Option<RulesMutateError>,
     /// The namespace map we use for parsing. Needs to start as `Some()`, and
     /// will be taken out after parsing namespace rules, and that reference will
     /// be moved to `ParserContext`.
     pub namespaces: &'a mut Namespaces,
+    /// The info we need insert a rule in a list.
+    pub insert_rule_context: Option<InsertRuleContext<'a>>,
 }
 
 impl<'b, R> TopLevelRuleParser<'b, R> {
     fn nested<'a: 'b>(&'a self) -> NestedRuleParser<'a, 'b, R> {
         NestedRuleParser {
             stylesheet_origin: self.stylesheet_origin,
             shared_lock: self.shared_lock,
             context: &self.context,
@@ -69,24 +79,53 @@ impl<'b, R> TopLevelRuleParser<'b, R> {
         }
     }
 
     /// Returns the current state of the parser.
     pub fn state(&self) -> State {
         self.state
     }
 
-    /// Returns whether we previously tried to parse a rule that was invalid
-    /// due to being in the wrong place (e.g. an @import rule was found after
-    /// a regular style rule).  The state of this flag is reset when this
-    /// function is called.
-    pub fn take_had_hierarchy_error(&mut self) -> bool {
-        let had_hierarchy_error = self.had_hierarchy_error;
-        self.had_hierarchy_error = false;
-        had_hierarchy_error
+    /// Checks whether we can parse a rule that would transition us to
+    /// `new_state`.
+    ///
+    /// This is usually a simple branch, but we may need more bookkeeping if
+    /// doing `insertRule` from CSSOM.
+    fn check_state(&mut self, new_state: State) -> bool {
+        if self.state > new_state {
+            self.dom_error = Some(RulesMutateError::HierarchyRequest);
+            return false;
+        }
+
+        let ctx = match self.insert_rule_context {
+            Some(ref ctx) => ctx,
+            None => return true,
+        };
+
+        let next_rule_state = match ctx.rule_list.get(ctx.index) {
+            None => return true,
+            Some(rule) => rule.rule_state(),
+        };
+
+        if new_state > next_rule_state {
+            self.dom_error = Some(RulesMutateError::HierarchyRequest);
+            return false;
+        }
+
+        // If there's anything that isn't a namespace rule (or import rule, but
+        // we checked that already at the beginning), reject with a
+        // StateError.
+        if new_state == State::Namespaces &&
+            ctx.rule_list[ctx.index..].iter().any(|r| !matches!(*r, CssRule::Namespace(..)))
+        {
+            self.dom_error = Some(RulesMutateError::InvalidState);
+            return false;
+        }
+
+        true
     }
 }
 
 /// The current state of the parser.
 #[derive(Clone, Copy, Eq, Ord, PartialEq, PartialOrd)]
 pub enum State {
     /// We haven't started parsing rules.
     Start = 1,
@@ -146,36 +185,32 @@ impl<'a, 'i, R: ParseErrorReporter> AtRu
     fn parse_prelude<'t>(
         &mut self,
         name: CowRcStr<'i>,
         input: &mut Parser<'i, 't>,
     ) -> Result<AtRuleType<AtRuleNonBlockPrelude, AtRuleBlockPrelude>, ParseError<'i>> {
         let location = input.current_source_location();
         match_ignore_ascii_case! { &*name,
             "import" => {
-                if self.state > State::Imports {
-                    // "@import must be before any rule but @charset"
-                    self.had_hierarchy_error = true;
+                if !self.check_state(State::Imports) {
                     return Err(input.new_custom_error(StyleParseErrorKind::UnexpectedImportRule))
                 }
 
                 let url_string = input.expect_url_or_string()?.as_ref().to_owned();
                 let url = CssUrl::parse_from_string(url_string, &self.context);
 
                 let media = parse_media_query_list(&self.context, input,
                                                    self.error_context.error_reporter);
                 let media = Arc::new(self.shared_lock.wrap(media));
 
                 let prelude = AtRuleNonBlockPrelude::Import(url, media, location);
                 return Ok(AtRuleType::WithoutBlock(prelude));
             },
             "namespace" => {
-                if self.state > State::Namespaces {
-                    // "@namespace must be before any rule but @charset and @import"
-                    self.had_hierarchy_error = true;
+                if !self.check_state(State::Namespaces) {
                     return Err(input.new_custom_error(StyleParseErrorKind::UnexpectedNamespaceRule))
                 }
 
                 let prefix = input.try(|i| i.expect_ident_cloned())
                                   .map(|s| Prefix::from(s.as_ref())).ok();
                 let maybe_namespace = match input.expect_url_or_string() {
                     Ok(url_or_string) => url_or_string,
                     Err(BasicParseError { kind: BasicParseErrorKind::UnexpectedToken(t), location }) => {
@@ -185,22 +220,26 @@ impl<'a, 'i, R: ParseErrorReporter> AtRu
                 };
                 let url = Namespace::from(maybe_namespace.as_ref());
                 let prelude = AtRuleNonBlockPrelude::Namespace(prefix, url, location);
                 return Ok(AtRuleType::WithoutBlock(prelude));
             },
             // @charset is removed by rust-cssparser if it’s the first rule in the stylesheet
             // anything left is invalid.
             "charset" => {
-                self.had_hierarchy_error = true;
+                self.dom_error = Some(RulesMutateError::HierarchyRequest);
                 return Err(input.new_custom_error(StyleParseErrorKind::UnexpectedCharsetRule))
             }
             _ => {}
         }
 
+        if !self.check_state(State::Body) {
+            return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError))
+        }
+
         AtRuleParser::parse_prelude(&mut self.nested(), name, input)
     }
 
     #[inline]
     fn parse_block<'t>(
         &mut self,
         prelude: AtRuleBlockPrelude,
         input: &mut Parser<'i, 't>,
@@ -259,16 +298,20 @@ impl<'a, 'i, R: ParseErrorReporter> Qual
     type QualifiedRule = CssRule;
     type Error = StyleParseErrorKind<'i>;
 
     #[inline]
     fn parse_prelude<'t>(
         &mut self,
         input: &mut Parser<'i, 't>,
     ) -> Result<QualifiedRuleParserPrelude, ParseError<'i>> {
+        if !self.check_state(State::Body) {
+            return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError))
+        }
+
         QualifiedRuleParser::parse_prelude(&mut self.nested(), input)
     }
 
     #[inline]
     fn parse_block<'t>(
         &mut self,
         prelude: QualifiedRuleParserPrelude,
         input: &mut Parser<'i, 't>,
--- a/servo/components/style/stylesheets/stylesheet.rs
+++ b/servo/components/style/stylesheets/stylesheet.rs
@@ -363,17 +363,18 @@ impl Stylesheet {
 
         let rule_parser = TopLevelRuleParser {
             stylesheet_origin: origin,
             shared_lock,
             loader: stylesheet_loader,
             context,
             error_context,
             state: State::Start,
-            had_hierarchy_error: false,
+            dom_error: None,
+            insert_rule_context: None,
             namespaces,
         };
 
         {
             let mut iter = RuleListParser::new_for_stylesheet(&mut input, rule_parser);
 
             while let Some(result) = iter.next() {
                 match result {
--- a/testing/web-platform/meta/MANIFEST.json
+++ b/testing/web-platform/meta/MANIFEST.json
@@ -326962,16 +326962,22 @@
     ]
    ],
    "css/cssom/StyleSheetList.html": [
     [
      "/css/cssom/StyleSheetList.html",
      {}
     ]
    ],
+   "css/cssom/at-namespace.html": [
+    [
+     "/css/cssom/at-namespace.html",
+     {}
+    ]
+   ],
    "css/cssom/computed-style-001.html": [
     [
      "/css/cssom/computed-style-001.html",
      {}
     ]
    ],
    "css/cssom/computed-style-002.html": [
     [
@@ -545871,16 +545877,20 @@
   "css/cssom/OWNERS": [
    "f131f271cb2f747e845584abcc445348e8c86521",
    "support"
   ],
   "css/cssom/StyleSheetList.html": [
    "0a1cd8ed56ac3a5b1a9556835d94fb80325199bf",
    "testharness"
   ],
+  "css/cssom/at-namespace.html": [
+   "96da2dd244e9e19ff8ca1ca81b06c3ebdcee8267",
+   "testharness"
+  ],
   "css/cssom/computed-style-001.html": [
    "0331a648e6b0d56f0e7365f1ff7d991ea77ce3e4",
    "testharness"
   ],
   "css/cssom/computed-style-002.html": [
    "d6579788bcfaf1d4c09324ba877a26ff95d6965d",
    "testharness"
   ],
@@ -546832,17 +546842,17 @@
    "a8cfa83e572a766b61e4eae5946e7efb62e9eab7",
    "testharness"
   ],
   "css/geometry/DOMMatrix-002.html": [
    "c38b9321ebb06ecae2a4217b36493f46b8649636",
    "testharness"
   ],
   "css/geometry/DOMMatrix-003.html": [
-   "9e2d031f83fbcc4d32a3891fdf2c2d8bc2cc774c",
+   "ed9ca53d25463a9b38414ab9062cf3eb2f8991cb",
    "testharness"
   ],
   "css/geometry/DOMMatrix-a-f-alias.html": [
    "6041bd4e7fd1535d9c8515f1b2f07981b2bdd366",
    "testharness"
   ],
   "css/geometry/DOMMatrix-attributes.html": [
    "433984b90fc4579257db57b07a09ae36f7e5b4d3",
@@ -615676,17 +615686,17 @@
    "e0918d83187c0fbdadaebb14be72c6f34f8dfc03",
    "support"
   ],
   "web-animations/resources/xhr-doc.py": [
    "de68c45fc1d38a49946f9046f34031e9278a1531",
    "support"
   ],
   "web-animations/testcommon.js": [
-   "039db95bc6b54f00d6e106241d001ae980566f92",
+   "57883097f4a440ae95b9e665bf8d9e36a187f7d8",
    "support"
   ],
   "web-animations/timing-model/animation-effects/active-time.html": [
    "f05ff3594dde7248c84db42f8a80a6d0136b5f54",
    "testharness"
   ],
   "web-animations/timing-model/animation-effects/current-iteration.html": [
    "617bfd8c533d159c4e56ea823917d580fe262bf6",
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/cssom/at-namespace.html
@@ -0,0 +1,29 @@
+<!doctype html>
+<title>CSS Test: @namespace in CSSOM is not severely broken</title>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1464865">
+<link rel="help" href="https://drafts.csswg.org/cssom/#insert-a-css-rule">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<style id="s">
+  div { color: green }
+</style>
+<div>Should be green</div>
+<script>
+test(function() {
+  assert_throws("InvalidStateError", function() {
+    s.sheet.insertRule('@namespace myhtml url("http://www.w3.org/1999/xhtml")', 0);
+  });
+  assert_equals(s.sheet.cssRules.length, 1, "Shouldn't have been inserted");
+  assert_throws("SyntaxError", function() {
+    s.sheet.insertRule("myhtml|div { color: red !important }", 0);
+  });
+  assert_equals(s.sheet.cssRules.length, 1);
+  assert_equals(
+    getComputedStyle(document.querySelector("div")).color,
+    "rgb(0, 128, 0)",
+    "Namespace shouldn't be registered"
+  );
+});
+</script>
+