Bug 1464865: Don't let @namespace rules that aren't going to be inserted affect the namespace map. r?xidorn draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Mon, 28 May 2018 23:56:20 +0200
changeset 800843 3998b8b9367785c716907a111cdafcb577312e91
parent 800842 3c85c39533f506374eb76333c3f152305674e261
child 800844 d5952d88090e8222903b763f57cc0a62b600cd56
push id111503
push userbmo:emilio@crisal.io
push dateTue, 29 May 2018 11:06:36 +0000
reviewersxidorn
bugs1464865
milestone62.0a1
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
@@ -326913,16 +326913,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": [
     [
@@ -545788,16 +545794,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"
   ],
@@ -546749,17 +546759,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",
@@ -615589,17 +615599,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>
+