servo: Merge #17918 - style: Don't remain in an invalid state when encountering an at-rule in the wrong place (from heycam:rule-hierarchy); r=emilio
authorCameron McCormack <cam@mcc.id.au>
Sat, 29 Jul 2017 11:16:33 -0500
changeset 420541 770c9d02352d1af7af74f6cc18f209727db7cc7a
parent 420540 45f201fe8a9a04031281696a9a25b8d96d636355
child 420542 34c157df46d2be2e0f71f5f87e6a649c79ccff4c
push id7566
push usermtabara@mozilla.com
push dateWed, 02 Aug 2017 08:25:16 +0000
treeherdermozilla-beta@86913f512c3c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersemilio
milestone56.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
servo: Merge #17918 - style: Don't remain in an invalid state when encountering an at-rule in the wrong place (from heycam:rule-hierarchy); r=emilio Currently, attempting to parse an at-rule that is out of place, such as an @import rule after a regular style rule, will cause the parser state to be set to Invalid. This will cause any following at-rule to be rejected until we encounter a regular style rule, at which point we'll go back to the Body state. There's nothing in the CSS specs about needing to reject all following at-rules (or, as the comment above Invalid says, ignoring the entire rest of the style sheet). <!-- Please describe your changes on the following line: --> --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: d859734702366cec5bc4a48b6c0d2ae93d1179b0
servo/components/style/stylesheets/mod.rs
servo/components/style/stylesheets/rule_parser.rs
servo/components/style/stylesheets/stylesheet.rs
--- a/servo/components/style/stylesheets/mod.rs
+++ b/servo/components/style/stylesheets/mod.rs
@@ -250,28 +250,29 @@ impl CssRule {
         // 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,
             shared_lock: &shared_lock,
             loader: loader,
             state: state,
+            had_hierarchy_error: false,
             namespaces: Some(&mut *guard),
         };
 
-        match parse_one_rule(&mut input, &mut rule_parser) {
-            Ok(result) => Ok((result, rule_parser.state)),
-            Err(_) => {
-                Err(match rule_parser.state {
-                    State::Invalid => SingleRuleParseError::Hierarchy,
-                    _ => SingleRuleParseError::Syntax,
-                })
-            }
-        }
+        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
+                }
+            })
     }
 }
 
 impl DeepCloneWithLock for CssRule {
     /// Deep clones this CssRule.
     fn deep_clone_with_lock(
         &self,
         lock: &SharedRwLock,
--- a/servo/components/style/stylesheets/rule_parser.rs
+++ b/servo/components/style/stylesheets/rule_parser.rs
@@ -42,16 +42,20 @@ pub struct TopLevelRuleParser<'a> {
     pub shared_lock: &'a SharedRwLock,
     /// A reference to a stylesheet loader if applicable, for `@import` rules.
     pub loader: Option<&'a StylesheetLoader>,
     /// The parser context. This initially won't contain any namespaces, but
     /// will be populated after parsing namespace rules, if any.
     pub context: ParserContext<'a>,
     /// 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,
     /// 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: Option<&'a mut Namespaces>,
 }
 
 impl<'b> TopLevelRuleParser<'b> {
     fn nested<'a: 'b>(&'a self) -> NestedRuleParser<'a, 'b> {
@@ -66,32 +70,39 @@ impl<'b> TopLevelRuleParser<'b> {
     pub fn context(&self) -> &ParserContext {
         &self.context
     }
 
     /// 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
+    }
 }
 
 /// The current state of the parser.
 #[derive(Eq, PartialEq, Ord, PartialOrd, Copy, Clone)]
 pub enum State {
     /// We haven't started parsing rules.
     Start = 1,
     /// We're parsing `@import` rules.
     Imports = 2,
     /// We're parsing `@namespace` rules.
     Namespaces = 3,
     /// We're parsing the main body of the stylesheet.
     Body = 4,
-    /// We've found an invalid state (as, a namespace rule after style rules),
-    /// and the rest of the stylesheet should be ignored.
-    Invalid = 5,
 }
 
 #[derive(Clone, Debug)]
 #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
 /// Vendor prefix.
 pub enum VendorPrefix {
     /// -moz prefix.
     Moz,
@@ -148,18 +159,18 @@ impl<'a, 'i> AtRuleParser<'i> for TopLev
         name: CowRcStr<'i>,
         input: &mut Parser<'i, 't>
     ) -> Result<AtRuleType<AtRulePrelude, CssRule>, ParseError<'i>> {
         let location = get_location_with_offset(input.current_source_location(),
                                                 self.context.line_number_offset);
         match_ignore_ascii_case! { &*name,
             "import" => {
                 if self.state > State::Imports {
-                    self.state = State::Invalid;
                     // "@import must be before any rule but @charset"
+                    self.had_hierarchy_error = true;
                     return Err(StyleParseError::UnexpectedImportRule.into())
                 }
 
                 self.state = State::Imports;
                 let url_string = input.expect_url_or_string()?.as_ref().to_owned();
                 let specified_url = SpecifiedUrl::parse_from_string(url_string, &self.context)?;
 
                 let media = parse_media_query_list(&self.context, input);
@@ -175,18 +186,18 @@ impl<'a, 'i> AtRuleParser<'i> for TopLev
                     &self.shared_lock,
                     media,
                 );
 
                 return Ok(AtRuleType::WithoutBlock(CssRule::Import(import_rule)))
             },
             "namespace" => {
                 if self.state > State::Namespaces {
-                    self.state = State::Invalid;
                     // "@namespace must be before any rule but @charset and @import"
+                    self.had_hierarchy_error = true;
                     return Err(StyleParseError::UnexpectedNamespaceRule.into())
                 }
                 self.state = State::Namespaces;
 
                 let prefix_result = input.try(|i| i.expect_ident_cloned());
                 let maybe_namespace = match input.expect_url_or_string() {
                     Ok(url_or_string) => url_or_string,
                     Err(BasicParseError::UnexpectedToken(t)) =>
@@ -216,24 +227,22 @@ impl<'a, 'i> AtRuleParser<'i> for TopLev
                         prefix: opt_prefix,
                         url: url,
                         source_location: location,
                     })
                 ))))
             },
             // @charset is removed by rust-cssparser if it’s the first rule in the stylesheet
             // anything left is invalid.
-            "charset" => return Err(StyleParseError::UnexpectedCharsetRule.into()),
+            "charset" => {
+                self.had_hierarchy_error = true;
+                return Err(StyleParseError::UnexpectedCharsetRule.into())
+            }
             _ => {}
         }
-        // Don't allow starting with an invalid state
-        if self.state > State::Body {
-            self.state = State::Invalid;
-            return Err(StyleParseError::UnspecifiedError.into());
-        }
         self.state = State::Body;
 
         // "Freeze" the namespace map (no more namespace rules can be parsed
         // after this point), and stick it in the context.
         if self.namespaces.is_some() {
             let namespaces = &*self.namespaces.take().unwrap();
             self.context.namespaces = Some(namespaces);
         }
--- a/servo/components/style/stylesheets/stylesheet.rs
+++ b/servo/components/style/stylesheets/stylesheet.rs
@@ -332,16 +332,17 @@ impl Stylesheet {
             );
 
         let rule_parser = TopLevelRuleParser {
             stylesheet_origin: origin,
             shared_lock: shared_lock,
             loader: stylesheet_loader,
             context: context,
             state: State::Start,
+            had_hierarchy_error: false,
             namespaces: Some(namespaces),
         };
 
         input.look_for_viewport_percentages();
 
         {
             let mut iter =
                 RuleListParser::new_for_stylesheet(&mut input, rule_parser);