Auto merge of #18341 - ferjm:bug1384225.media.errors, r=jdm

stylo: Error reporting for unknown media features

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix [bug 1384225](https://bugzilla.mozilla.org/show_bug.cgi?id=1384225)

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18341)
<!-- Reviewable:end -->

components/script/dom/cssmediarule.rs

@@ -72,13 +72,15 @@ impl CSSMediaRule {
         let mut input = ParserInput::new(&text);
         let mut input = Parser::new(&mut input);
         let global = self.global();
-        let win = global.as_window();
+        let window = global.as_window();
-        let url = win.get_url();
+        let url = window.get_url();
-        let quirks_mode = win.Document().quirks_mode();
+        let quirks_mode = window.Document().quirks_mode();
         let context = ParserContext::new_for_cssom(&url, Some(CssRuleType::Media),
                                                    PARSING_MODE_DEFAULT,
                                                    quirks_mode);
-        let new_medialist = parse_media_query_list(&context, &mut input);
+
+        let new_medialist = parse_media_query_list(&context, &mut input,
+                                                   window.css_error_reporter());
         let mut guard = self.cssconditionrule.shared_lock().write();
 
         // Clone an Arc because we can’t borrow `guard` twice at the same time.

components/script/dom/htmllinkelement.rs

@@ -289,7 +289,9 @@ impl HTMLLinkElement {
         let context = CssParserContext::new_for_cssom(&doc_url, Some(CssRuleType::Media),
                                                       PARSING_MODE_DEFAULT,
                                                       document.quirks_mode());
-        let media = parse_media_query_list(&context, &mut css_parser);
+        let window = document.window();
+        let media = parse_media_query_list(&context, &mut css_parser,
+                                           window.css_error_reporter());
 
         let im_attribute = element.get_attribute(&ns!(), &local_name!("integrity"));
         let integrity_val = im_attribute.r().map(|a| a.value());

components/script/dom/htmlstyleelement.rs

@@ -74,7 +74,7 @@ impl HTMLStyleElement {
         let element = self.upcast::<Element>();
         assert!(node.is_in_doc());
 
-        let win = window_from_node(node);
+        let window = window_from_node(node);
         let doc = document_from_node(self);
 
         let mq_attribute = element.get_attribute(&ns!(), &local_name!("media"));
@@ -84,19 +84,22 @@ impl HTMLStyleElement {
         };
 
         let data = node.GetTextContent().expect("Element.textContent must be a string");
-        let url = win.get_url();
+        let url = window.get_url();
         let context = CssParserContext::new_for_cssom(&url,
                                                       Some(CssRuleType::Media),
                                                       PARSING_MODE_DEFAULT,
                                                       doc.quirks_mode());
         let shared_lock = node.owner_doc().style_shared_lock().clone();
         let mut input = ParserInput::new(&mq_str);
-        let mq = Arc::new(shared_lock.wrap(
+        let css_error_reporter = window.css_error_reporter();
-                    parse_media_query_list(&context, &mut CssParser::new(&mut input))));
+        let mq = Arc::new(shared_lock.wrap(parse_media_query_list(&context,
+                                                                  &mut CssParser::new(&mut input),
+                                                                  css_error_reporter)));
         let loader = StylesheetLoader::for_element(self.upcast());
-        let sheet = Stylesheet::from_str(&data, win.get_url(), Origin::Author, mq,
+        let sheet = Stylesheet::from_str(&data, window.get_url(),
+                                         Origin::Author, mq,
                                          shared_lock, Some(&loader),
-                                         win.css_error_reporter(),
+                                         css_error_reporter,
                                          doc.quirks_mode(),
                                          self.line_number as u32);
 

components/script/dom/medialist.rs

@@ -74,13 +74,14 @@ impl MediaListMethods for MediaList {
         let mut input = ParserInput::new(&value);
         let mut parser = Parser::new(&mut input);
         let global = self.global();
-        let win = global.as_window();
+        let window = global.as_window();
-        let url = win.get_url();
+        let url = window.get_url();
-        let quirks_mode = win.Document().quirks_mode();
+        let quirks_mode = window.Document().quirks_mode();
         let context = ParserContext::new_for_cssom(&url, Some(CssRuleType::Media),
                                                    PARSING_MODE_DEFAULT,
                                                    quirks_mode);
-        *media_queries = parse_media_query_list(&context, &mut parser);
+        *media_queries = parse_media_query_list(&context, &mut parser,
+                                                window.css_error_reporter());
     }
 
     // https://drafts.csswg.org/cssom/#dom-medialist-length

components/script/dom/window.rs

@@ -1012,7 +1012,8 @@ impl WindowMethods for Window {
         let context = CssParserContext::new_for_cssom(&url, Some(CssRuleType::Media),
                                                       PARSING_MODE_DEFAULT,
                                                       quirks_mode);
-        let media_query_list = media_queries::parse_media_query_list(&context, &mut parser);
+        let media_query_list = media_queries::parse_media_query_list(&context, &mut parser,
+                                                                     self.css_error_reporter());
         let document = self.Document();
         let mql = MediaQueryList::new(&document, media_query_list);
         self.media_query_lists.push(&*mql);

components/style/error_reporting.rs

@@ -19,7 +19,7 @@ pub enum ContextualParseError<'a> {
     UnsupportedPropertyDeclaration(&'a str, ParseError<'a>),
     /// A font face descriptor was not recognized.
     UnsupportedFontFaceDescriptor(&'a str, ParseError<'a>),
-    /// A font feature values descroptor was not recognized.
+    /// A font feature values descriptor was not recognized.
     UnsupportedFontFeatureValuesDescriptor(&'a str, ParseError<'a>),
     /// A keyframe rule was not valid.
     InvalidKeyframeRule(&'a str, ParseError<'a>),
@@ -44,7 +44,9 @@ pub enum ContextualParseError<'a> {
     /// A counter style rule had extends with symbols.
     InvalidCounterStyleExtendsWithSymbols,
     /// A counter style rule had extends with additive-symbols.
-    InvalidCounterStyleExtendsWithAdditiveSymbols
+    InvalidCounterStyleExtendsWithAdditiveSymbols,
+    /// A media rule was invalid for some reason.
+    InvalidMediaRule(&'a str, ParseError<'a>),
 }
 
 impl<'a> fmt::Display for ContextualParseError<'a> {
@@ -168,6 +170,10 @@ impl<'a> fmt::Display for ContextualParseError<'a> {
             ContextualParseError::InvalidCounterStyleExtendsWithAdditiveSymbols => {
                 write!(f, "Invalid @counter-style rule: 'system: extends …' with 'additive-symbols'")
             }
+            ContextualParseError::InvalidMediaRule(media_rule, ref err) => {
+                write!(f, "Invalid media rule: {}, ", media_rule)?;
+                parse_error_to_str(err, f)
+            }
         }
     }
 }

components/style/gecko/media_queries.rs

@@ -23,7 +23,6 @@ use media_queries::MediaType;
 use parser::ParserContext;
 use properties::{ComputedValues, StyleBuilder};
 use properties::longhands::font_size;
-use selectors::parser::SelectorParseError;
 use servo_arc::Arc;
 use std::fmt::{self, Write};
 use std::sync::atomic::{AtomicBool, AtomicIsize, Ordering};
@@ -469,6 +468,89 @@ unsafe fn find_in_table<F>(mut current_entry: *const nsCSSProps_KTableEntry,
     }
 }
 
+fn parse_feature_value<'i, 't>(feature: &nsMediaFeature,
+                               feature_value_type: nsMediaFeature_ValueType,
+                               context: &ParserContext,
+                               input: &mut Parser<'i, 't>)
+                               -> Result<MediaExpressionValue, ParseError<'i>> {
+    let value = match feature_value_type {
+        nsMediaFeature_ValueType::eLength => {
+           let length = Length::parse_non_negative(context, input)?;
+           // FIXME(canaltinova): See bug 1396057. Gecko doesn't support calc
+           // inside media queries. This check is for temporarily remove it
+           // for parity with gecko. We should remove this check when we want
+           // to support it.
+           if let Length::Calc(_) = length {
+               return Err(StyleParseError::UnspecifiedError.into())
+           }
+           MediaExpressionValue::Length(length)
+        },
+        nsMediaFeature_ValueType::eInteger => {
+           // FIXME(emilio): We should use `Integer::parse` to handle `calc`
+           // properly in integer expressions. Note that calc is still not
+           // supported in media queries per FIXME above.
+           let i = input.expect_integer()?;
+           if i < 0 {
+               return Err(StyleParseError::UnspecifiedError.into())
+           }
+           MediaExpressionValue::Integer(i as u32)
+        }
+        nsMediaFeature_ValueType::eBoolInteger => {
+           let i = input.expect_integer()?;
+           if i < 0 || i > 1 {
+               return Err(StyleParseError::UnspecifiedError.into())
+           }
+           MediaExpressionValue::BoolInteger(i == 1)
+        }
+        nsMediaFeature_ValueType::eFloat => {
+           MediaExpressionValue::Float(input.expect_number()?)
+        }
+        nsMediaFeature_ValueType::eIntRatio => {
+           let a = input.expect_integer()?;
+           if a <= 0 {
+               return Err(StyleParseError::UnspecifiedError.into())
+           }
+
+           input.expect_delim('/')?;
+
+           let b = input.expect_integer()?;
+           if b <= 0 {
+               return Err(StyleParseError::UnspecifiedError.into())
+           }
+           MediaExpressionValue::IntRatio(a as u32, b as u32)
+        }
+        nsMediaFeature_ValueType::eResolution => {
+           MediaExpressionValue::Resolution(Resolution::parse(input)?)
+        }
+        nsMediaFeature_ValueType::eEnumerated => {
+           let keyword = input.expect_ident()?;
+           let keyword = unsafe {
+               bindings::Gecko_LookupCSSKeyword(keyword.as_bytes().as_ptr(),
+               keyword.len() as u32)
+           };
+
+           let first_table_entry: *const nsCSSProps_KTableEntry = unsafe {
+               *feature.mData.mKeywordTable.as_ref()
+           };
+
+           let value =
+               match unsafe { find_in_table(first_table_entry, |kw, _| kw == keyword) } {
+                   Some((_kw, value)) => {
+                       value
+                   }
+                   None => return Err(StyleParseError::UnspecifiedError.into()),
+               };
+
+           MediaExpressionValue::Enumerated(value)
+        }
+        nsMediaFeature_ValueType::eIdent => {
+           MediaExpressionValue::Ident(input.expect_ident()?.as_ref().to_owned())
+        }
+    };
+
+    Ok(value)
+}
+
 impl Expression {
     /// Trivially construct a new expression.
     fn new(feature: &'static nsMediaFeature,
@@ -488,13 +570,24 @@ impl Expression {
     /// ```
     pub fn parse<'i, 't>(context: &ParserContext, input: &mut Parser<'i, 't>)
                          -> Result<Self, ParseError<'i>> {
-        input.expect_parenthesis_block()?;
+        input.expect_parenthesis_block().map_err(|err|
+            match err {
+                BasicParseError::UnexpectedToken(t) => StyleParseError::ExpectedIdentifier(t),
+                _ => StyleParseError::UnspecifiedError,
+            }
+        )?;
+
         input.parse_nested_block(|input| {
             // FIXME: remove extra indented block when lifetimes are non-lexical
             let feature;
             let range;
             {
-                let ident = input.expect_ident()?;
+                let ident = input.expect_ident().map_err(|err|
+                    match err {
+                        BasicParseError::UnexpectedToken(t) => StyleParseError::ExpectedIdentifier(t),
+                        _ => StyleParseError::UnspecifiedError,
+                    }
+                )?;
 
                 let mut flags = 0;
                 let result = {
@@ -530,17 +623,19 @@ impl Expression {
                     Ok((f, r)) => {
                         feature = f;
                         range = r;
-                    }
+                    },
-                    Err(()) => return Err(SelectorParseError::UnexpectedIdent(ident.clone()).into()),
+                    Err(()) => {
+                        return Err(StyleParseError::MediaQueryExpectedFeatureName(ident.clone()).into())
+                    },
                 }
 
                 if (feature.mReqFlags & !flags) != 0 {
-                    return Err(SelectorParseError::UnexpectedIdent(ident.clone()).into());
+                    return Err(StyleParseError::MediaQueryExpectedFeatureName(ident.clone()).into());
                 }
 
                 if range != nsMediaExpression_Range::eEqual &&
-                    feature.mRangeType != nsMediaFeature_RangeType::eMinMaxAllowed {
+                   feature.mRangeType != nsMediaFeature_RangeType::eMinMaxAllowed {
-                    return Err(SelectorParseError::UnexpectedIdent(ident.clone()).into());
+                    return Err(StyleParseError::MediaQueryExpectedFeatureName(ident.clone()).into());
                 }
             }
 
@@ -556,80 +651,11 @@ impl Expression {
                 return Ok(Expression::new(feature, None, range));
             }
 
-            let value = match feature.mValueType {
+            let value = parse_feature_value(feature,
-                nsMediaFeature_ValueType::eLength => {
+                                            feature.mValueType,
-                    let length = Length::parse_non_negative(context, input)?;
+                                            context, input).map_err(|_|
-                    // FIXME(canaltinova): See bug 1396057. Gecko doesn't support calc
+                StyleParseError::MediaQueryExpectedFeatureValue
-                    // inside media queries. This check is for temporarily remove it
+            )?;
-                    // for parity with gecko. We should remove this check when we want
-                    // to support it.
-                    if let Length::Calc(_) = length {
-                        return Err(StyleParseError::UnspecifiedError.into())
-                    }
-                    MediaExpressionValue::Length(length)
-                },
-                nsMediaFeature_ValueType::eInteger => {
-                    // FIXME(emilio): We should use `Integer::parse` to handle `calc`
-                    // properly in integer expressions. Note that calc is still not
-                    // supported in media queries per FIXME above.
-                    let i = input.expect_integer()?;
-                    if i < 0 {
-                        return Err(StyleParseError::UnspecifiedError.into())
-                    }
-                    MediaExpressionValue::Integer(i as u32)
-                }
-                nsMediaFeature_ValueType::eBoolInteger => {
-                    let i = input.expect_integer()?;
-                    if i < 0 || i > 1 {
-                        return Err(StyleParseError::UnspecifiedError.into())
-                    }
-                    MediaExpressionValue::BoolInteger(i == 1)
-                }
-                nsMediaFeature_ValueType::eFloat => {
-                    MediaExpressionValue::Float(input.expect_number()?)
-                }
-                nsMediaFeature_ValueType::eIntRatio => {
-                    let a = input.expect_integer()?;
-                    if a <= 0 {
-                        return Err(StyleParseError::UnspecifiedError.into())
-                    }
-
-                    input.expect_delim('/')?;
-
-                    let b = input.expect_integer()?;
-                    if b <= 0 {
-                        return Err(StyleParseError::UnspecifiedError.into())
-                    }
-                    MediaExpressionValue::IntRatio(a as u32, b as u32)
-                }
-                nsMediaFeature_ValueType::eResolution => {
-                    MediaExpressionValue::Resolution(Resolution::parse(input)?)
-                }
-                nsMediaFeature_ValueType::eEnumerated => {
-                    let keyword = input.expect_ident()?;
-                    let keyword = unsafe {
-                        bindings::Gecko_LookupCSSKeyword(keyword.as_bytes().as_ptr(),
-                                                         keyword.len() as u32)
-                    };
-
-                    let first_table_entry: *const nsCSSProps_KTableEntry = unsafe {
-                        *feature.mData.mKeywordTable.as_ref()
-                    };
-
-                    let value =
-                        match unsafe { find_in_table(first_table_entry, |kw, _| kw == keyword) } {
-                            Some((_kw, value)) => {
-                                value
-                            }
-                            None => return Err(StyleParseError::UnspecifiedError.into()),
-                        };
-
-                    MediaExpressionValue::Enumerated(value)
-                }
-                nsMediaFeature_ValueType::eIdent => {
-                    MediaExpressionValue::Ident(input.expect_ident()?.as_ref().to_owned())
-                }
-            };
 
             Ok(Expression::new(feature, Some(value), range))
         })

components/style/media_queries.rs

@@ -8,8 +8,10 @@
 
 use Atom;
 use context::QuirksMode;
-use cssparser::{Delimiter, Parser, Token, ParserInput};
+use cssparser::{Delimiter, Parser};
-use parser::ParserContext;
+use cssparser::{Token, ParserInput};
+use error_reporting::{ContextualParseError, ParseErrorReporter};
+use parser::{ParserContext, ParserErrorContext};
 use selectors::parser::SelectorParseError;
 use serialize_comma_separated_list;
 use std::fmt;
@@ -240,19 +242,32 @@ impl MediaQuery {
 /// media query list is only filled with the equivalent of "not all", see:
 ///
 /// https://drafts.csswg.org/mediaqueries/#error-handling
-pub fn parse_media_query_list(context: &ParserContext, input: &mut Parser) -> MediaList {
+pub fn parse_media_query_list<R>(
+    context: &ParserContext,
+    input: &mut Parser,
+    error_reporter: &R,
+) -> MediaList
+where
+    R: ParseErrorReporter,
+{
     if input.is_exhausted() {
         return MediaList::empty()
     }
 
     let mut media_queries = vec![];
     loop {
+        let start_position = input.position();
+        let start_location = input.current_source_location();
         match input.parse_until_before(Delimiter::Comma, |i| MediaQuery::parse(context, i)) {
             Ok(mq) => {
                 media_queries.push(mq);
             },
-            Err(..) => {
+            Err(err) => {
                 media_queries.push(MediaQuery::never_matching());
+                let error = ContextualParseError::InvalidMediaRule(
+                    input.slice_from(start_position), err);
+                let error_context = ParserErrorContext { error_reporter };
+                context.log_css_error(&error_context, start_location, error);
             },
         }
 

components/style/stylesheets/rule_parser.rs

@@ -178,7 +178,8 @@ impl<'a, 'i, R: ParseErrorReporter> AtRuleParser<'i> for TopLevelRuleParser<'a,
                 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);
+                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(specified_url, media, location);
@@ -354,7 +355,8 @@ impl<'a, 'b, 'i, R: ParseErrorReporter> AtRuleParser<'i> for NestedRuleParser<'a
 
         match_ignore_ascii_case! { &*name,
             "media" => {
-                let media_queries = parse_media_query_list(self.context, input);
+                let media_queries = parse_media_query_list(self.context, input,
+                                                           self.error_context.error_reporter);
                 let arc = Arc::new(self.shared_lock.wrap(media_queries));
                 Ok(AtRuleType::WithBlock(AtRuleBlockPrelude::Media(arc, location)))
             },

components/style_traits/lib.rs

@@ -107,7 +107,13 @@ pub enum StyleParseError<'i> {
     PropertyDeclarationValueNotExhausted,
     /// An unexpected dimension token was encountered.
     UnexpectedDimension(CowRcStr<'i>),
-    /// A media query using a ranged expression with no value was encountered.
+    /// Expected identifier not found.
+    ExpectedIdentifier(Token<'i>),
+    /// Missing or invalid media feature name.
+    MediaQueryExpectedFeatureName(CowRcStr<'i>),
+    /// Missing or invalid media feature value.
+    MediaQueryExpectedFeatureValue,
+    /// min- or max- properties must have a value.
     RangedExpressionWithNoValue,
     /// A function was encountered that was not expected.
     UnexpectedFunction(CowRcStr<'i>),

ports/geckolib/error_reporter.rs

@@ -141,6 +141,14 @@ fn extract_error_params<'a>(err: ParseError<'a>) -> Option<ErrorParams<'a>> {
                 PropertyDeclarationParseError::InvalidValue(property, Some(e))))) =>
             (Some(ErrorString::Snippet(property.into())), Some(extract_value_error_param(e))),
 
+        CssParseError::Custom(SelectorParseError::Custom(
+            StyleParseError::MediaQueryExpectedFeatureName(ident))) =>
+            (Some(ErrorString::Ident(ident)), None),
+
+        CssParseError::Custom(SelectorParseError::Custom(
+            StyleParseError::ExpectedIdentifier(token))) =>
+            (Some(ErrorString::UnexpectedToken(token)), None),
+
         CssParseError::Custom(SelectorParseError::UnexpectedTokenInAttributeSelector(t)) |
         CssParseError::Custom(SelectorParseError::BadValueInAttr(t)) |
         CssParseError::Custom(SelectorParseError::ExpectedBarInAttr(t)) |
@@ -189,7 +197,8 @@ impl<'a> ErrorHelpers<'a> for ContextualParseError<'a> {
             ContextualParseError::InvalidRule(s, err) |
             ContextualParseError::UnsupportedRule(s, err) |
             ContextualParseError::UnsupportedViewportDescriptorDeclaration(s, err) |
-            ContextualParseError::UnsupportedCounterStyleDescriptorDeclaration(s, err) =>
+            ContextualParseError::UnsupportedCounterStyleDescriptorDeclaration(s, err) |
+            ContextualParseError::InvalidMediaRule(s, err) =>
                 (s.into(), err),
             ContextualParseError::InvalidCounterStyleWithoutSymbols(s) |
             ContextualParseError::InvalidCounterStyleNotEnoughSymbols(s) =>
@@ -281,6 +290,30 @@ impl<'a> ErrorHelpers<'a> for ContextualParseError<'a> {
                 };
                 return (prefix, b"PEBadSelectorRSIgnored\0", Action::Nothing);
             }
+            ContextualParseError::InvalidMediaRule(_, ref err) => {
+                let err: &[u8] = match *err {
+                    CssParseError::Custom(SelectorParseError::Custom(
+                            StyleParseError::ExpectedIdentifier(..))) => {
+                        b"PEGatherMediaNotIdent\0"
+                    },
+                    CssParseError::Custom(SelectorParseError::Custom(
+                            StyleParseError::MediaQueryExpectedFeatureName(..))) => {
+                        b"PEMQExpectedFeatureName\0"
+                    },
+                    CssParseError::Custom(SelectorParseError::Custom(
+                            StyleParseError::MediaQueryExpectedFeatureValue)) => {
+                        b"PEMQExpectedFeatureValue\0"
+                    },
+                    CssParseError::Custom(SelectorParseError::Custom(
+                            StyleParseError::RangedExpressionWithNoValue)) => {
+                        b"PEMQNoMinMaxWithoutValue\0"
+                    },
+                    _ => {
+                        b"PEDeclDropped\0"
+                    },
+                };
+                (err, Action::Nothing)
+            }
             ContextualParseError::UnsupportedRule(..) =>
                 (b"PEDeclDropped\0", Action::Nothing),
             ContextualParseError::UnsupportedViewportDescriptorDeclaration(..) |

ports/geckolib/glue.rs

@@ -2491,7 +2491,7 @@ pub extern "C" fn Servo_MediaList_SetText(list: RawServoMediaListBorrowed, text:
                                                PARSING_MODE_DEFAULT,
                                                QuirksMode::NoQuirks);
      write_locked_arc(list, |list: &mut MediaList| {
-        *list = parse_media_query_list(&context, &mut parser);
+        *list = parse_media_query_list(&context, &mut parser, &NullReporter);
     })
 }