Auto merge of #17926 - jdm:valueerr, r=heycam

stylo: Report a specific error for invalid CSS color values

Reviewed by heycam in https://bugzilla.mozilla.org/show_bug.cgi?id=1381143.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] There are tests for these changes (gecko devtools tests)

components/style/gecko/generated/bindings.rs

@@ -2881,10 +2881,15 @@ extern "C" {
                                           param:
                                               *const ::std::os::raw::c_char,
                                           paramLen: u32,
+                                          prefix:
+                                              *const ::std::os::raw::c_char,
+                                          prefixParam:
+                                              *const ::std::os::raw::c_char,
+                                          prefixParamLen: u32,
+                                          suffix:
+                                              *const ::std::os::raw::c_char,
                                           source:
                                               *const ::std::os::raw::c_char,
                                           sourceLen: u32, lineNumber: u32,
-                                          colNumber: u32, aURI: *mut nsIURI,
+                                          colNumber: u32);
-                                          followup:
-                                              *const ::std::os::raw::c_char);
 }

components/style/properties/properties.mako.rs

@@ -38,7 +38,7 @@ use selectors::parser::SelectorParseError;
 #[cfg(feature = "servo")] use servo_config::prefs::PREFS;
 use shared_lock::StylesheetGuards;
 use style_traits::{PARSING_MODE_DEFAULT, HasViewportPercentage, ToCss, ParseError};
-use style_traits::{PropertyDeclarationParseError, StyleParseError};
+use style_traits::{PropertyDeclarationParseError, StyleParseError, ValueParseError};
 use stylesheets::{CssRuleType, MallocSizeOf, MallocSizeOfFn, Origin, UrlExtraData};
 #[cfg(feature = "servo")] use values::Either;
 use values::generics::text::LineHeight;
@@ -1489,7 +1489,8 @@ impl PropertyDeclaration {
                     Ok(keyword) => DeclaredValueOwned::CSSWideKeyword(keyword),
                     Err(_) => match ::custom_properties::SpecifiedValue::parse(context, input) {
                         Ok(value) => DeclaredValueOwned::Value(value),
-                        Err(_) => return Err(PropertyDeclarationParseError::InvalidValue(name.to_string().into())),
+                        Err(e) => return Err(PropertyDeclarationParseError::InvalidValue(name.to_string().into(),
+                        ValueParseError::from_parse_error(e))),
                     }
                 };
                 declarations.push(PropertyDeclaration::Custom(name, value));
@@ -1502,13 +1503,14 @@ impl PropertyDeclaration {
                     input.look_for_var_functions();
                     let start = input.position();
                     input.parse_entirely(|input| id.parse_value(context, input))
-                    .or_else(|_| {
+                    .or_else(|err| {
                         while let Ok(_) = input.next() {}  // Look for var() after the error.
                         if input.seen_var_functions() {
                             input.reset(start);
                             let (first_token_type, css) =
-                                ::custom_properties::parse_non_custom_with_var(input).map_err(|_| {
+                                ::custom_properties::parse_non_custom_with_var(input).map_err(|e| {
-                                    PropertyDeclarationParseError::InvalidValue(id.name().into())
+                                    PropertyDeclarationParseError::InvalidValue(id.name().into(),
+                                        ValueParseError::from_parse_error(e))
                                 })?;
                             Ok(PropertyDeclaration::WithVariables(id, Arc::new(UnparsedValue {
                                 css: css.into_owned(),
@@ -1517,7 +1519,8 @@ impl PropertyDeclaration {
                                 from_shorthand: None,
                             })))
                         } else {
-                            Err(PropertyDeclarationParseError::InvalidValue(id.name().into()))
+                            Err(PropertyDeclarationParseError::InvalidValue(id.name().into(),
+                                ValueParseError::from_parse_error(err)))
                         }
                     })
                 }).map(|declaration| {
@@ -1539,13 +1542,14 @@ impl PropertyDeclaration {
                     let start = input.position();
                     // Not using parse_entirely here: each ${shorthand.ident}::parse_into function
                     // needs to do so *before* pushing to `declarations`.
-                    id.parse_into(declarations, context, input).or_else(|_| {
+                    id.parse_into(declarations, context, input).or_else(|err| {
                         while let Ok(_) = input.next() {}  // Look for var() after the error.
                         if input.seen_var_functions() {
                             input.reset(start);
                             let (first_token_type, css) =
-                                ::custom_properties::parse_non_custom_with_var(input).map_err(|_| {
+                                ::custom_properties::parse_non_custom_with_var(input).map_err(|e| {
-                                    PropertyDeclarationParseError::InvalidValue(id.name().into())
+                                    PropertyDeclarationParseError::InvalidValue(id.name().into(),
+                                        ValueParseError::from_parse_error(e))
                                 })?;
                             let unparsed = Arc::new(UnparsedValue {
                                 css: css.into_owned(),
@@ -1564,7 +1568,8 @@ impl PropertyDeclaration {
                             }
                             Ok(())
                         } else {
-                            Err(PropertyDeclarationParseError::InvalidValue(id.name().into()))
+                            Err(PropertyDeclarationParseError::InvalidValue(id.name().into(),
+                                ValueParseError::from_parse_error(err)))
                         }
                     })
                 }

components/style/values/specified/color.rs

@@ -13,7 +13,7 @@ use parser::{ParserContext, Parse};
 use properties::longhands::color::SystemColor;
 use std::fmt;
 use std::io::Write;
-use style_traits::{ToCss, ParseError, StyleParseError};
+use style_traits::{ToCss, ParseError, StyleParseError, ValueParseError};
 use super::AllowQuirks;
 use values::computed::{Color as ComputedColor, Context, ToComputedValue};
 
@@ -76,24 +76,28 @@ impl Parse for Color {
             _ => None,
         };
         input.reset(start_position);
-        if let Ok(value) = input.try(CSSParserColor::parse) {
+        match input.try(CSSParserColor::parse) {
-            Ok(match value {
+            Ok(value) =>
-                CSSParserColor::CurrentColor => Color::CurrentColor,
+                Ok(match value {
-                CSSParserColor::RGBA(rgba) => Color::Numeric {
+                    CSSParserColor::CurrentColor => Color::CurrentColor,
-                    parsed: rgba,
+                    CSSParserColor::RGBA(rgba) => Color::Numeric {
-                    authored: authored,
+                        parsed: rgba,
-                },
+                        authored: authored,
-            })
+                    },
-        } else {
+                }),
-            #[cfg(feature = "gecko")] {
+            Err(e) => {
-                if let Ok(system) = input.try(SystemColor::parse) {
+                #[cfg(feature = "gecko")] {
-                    Ok(Color::System(system))
+                    if let Ok(system) = input.try(SystemColor::parse) {
-                } else {
+                        return Ok(Color::System(system));
-                    gecko::SpecialColorKeyword::parse(input).map(Color::Special)
+                    } else if let Ok(c) = gecko::SpecialColorKeyword::parse(input) {
+                        return Ok(Color::Special(c));
+                    }
+                }
+                match e {
+                    BasicParseError::UnexpectedToken(t) =>
+                        Err(StyleParseError::ValueError(ValueParseError::InvalidColor(t)).into()),
+                    e => Err(e.into())
                 }
-            }
-            #[cfg(not(feature = "gecko"))] {
-                Err(StyleParseError::UnspecifiedError.into())
             }
         }
     }
@@ -161,11 +165,13 @@ impl Color {
                                 input: &mut Parser<'i, 't>,
                                 allow_quirks: AllowQuirks)
                                 -> Result<Self, ParseError<'i>> {
-        input.try(|i| Self::parse(context, i)).or_else(|_| {
+        input.try(|i| Self::parse(context, i)).or_else(|e| {
             if !allow_quirks.allowed(context.quirks_mode) {
-                return Err(StyleParseError::UnspecifiedError.into());
+                return Err(e);
             }
-            Color::parse_quirky_color(input).map(|rgba| Color::rgba(rgba))
+            Color::parse_quirky_color(input)
+                .map(|rgba| Color::rgba(rgba))
+                .map_err(|_| e)
         })
     }
 

components/style_traits/lib.rs

@@ -124,10 +124,31 @@ pub enum StyleParseError<'i> {
     UnspecifiedError,
     /// An unexpected token was found within a namespace rule.
     UnexpectedTokenWithinNamespace(Token<'i>),
+    /// An error was encountered while parsing a property value.
+    ValueError(ValueParseError<'i>),
+}
+
+/// Specific errors that can be encountered while parsing property values.
+#[derive(Clone, Debug, PartialEq)]
+pub enum ValueParseError<'i> {
+    /// An invalid token was encountered while parsing a color value.
+    InvalidColor(Token<'i>),
+}
+
+impl<'i> ValueParseError<'i> {
+    /// Attempt to extract a ValueParseError value from a ParseError.
+    pub fn from_parse_error(this: ParseError<'i>) -> Option<ValueParseError<'i>> {
+        match this {
+            cssparser::ParseError::Custom(
+                SelectorParseError::Custom(
+                    StyleParseError::ValueError(e))) => Some(e),
+            _ => None,
+        }
+    }
 }
 
 /// The result of parsing a property declaration.
-#[derive(Eq, PartialEq, Clone, Debug)]
+#[derive(PartialEq, Clone, Debug)]
 pub enum PropertyDeclarationParseError<'i> {
     /// The property declaration was for an unknown property.
     UnknownProperty(CowRcStr<'i>),
@@ -136,7 +157,7 @@ pub enum PropertyDeclarationParseError<'i> {
     /// The property declaration was for a disabled experimental property.
     ExperimentalProperty,
     /// The property declaration contained an invalid value.
-    InvalidValue(CowRcStr<'i>),
+    InvalidValue(CowRcStr<'i>, Option<ValueParseError<'i>>),
     /// The declaration contained an animation property, and we were parsing
     /// this as a keyframe block (so that property should be ignored).
     ///

ports/geckolib/error_reporter.rs

@@ -18,7 +18,7 @@ use style::gecko_bindings::structs::ErrorReporter as GeckoErrorReporter;
 use style::gecko_bindings::structs::URLExtraData as RawUrlExtraData;
 use style::gecko_bindings::sugar::refptr::RefPtr;
 use style::stylesheets::UrlExtraData;
-use style_traits::{ParseError, StyleParseError, PropertyDeclarationParseError};
+use style_traits::{ParseError, StyleParseError, PropertyDeclarationParseError, ValueParseError};
 
 /// Wrapper around an instance of Gecko's CSS error reporter.
 pub struct ErrorReporter(*mut GeckoErrorReporter);
@@ -194,8 +194,59 @@ enum Action {
 
 trait ErrorHelpers<'a> {
     fn error_data(self) -> (CowRcStr<'a>, ParseError<'a>);
-    fn error_param(self) -> ErrorString<'a>;
+    fn error_params(self) -> (ErrorString<'a>, Option<ErrorString<'a>>);
-    fn to_gecko_message(&self) -> (&'static [u8], Action);
+    fn to_gecko_message(&self) -> (Option<&'static [u8]>, &'static [u8], Action);
+}
+
+fn extract_error_param<'a>(err: ParseError<'a>) -> Option<ErrorString<'a>> {
+    Some(match err {
+        CssParseError::Basic(BasicParseError::UnexpectedToken(t)) =>
+            ErrorString::UnexpectedToken(t),
+
+        CssParseError::Basic(BasicParseError::AtRuleInvalid(i)) =>
+            ErrorString::Snippet(format!("@{}", escape_css_ident(&i)).into()),
+
+        CssParseError::Custom(SelectorParseError::Custom(
+            StyleParseError::PropertyDeclaration(
+                PropertyDeclarationParseError::InvalidValue(property, None)))) =>
+            ErrorString::Snippet(property),
+
+        CssParseError::Custom(SelectorParseError::UnexpectedIdent(ident)) =>
+            ErrorString::Ident(ident),
+
+        CssParseError::Custom(SelectorParseError::ExpectedNamespace(namespace)) =>
+            ErrorString::Ident(namespace),
+
+        CssParseError::Custom(SelectorParseError::Custom(
+            StyleParseError::PropertyDeclaration(
+                PropertyDeclarationParseError::UnknownProperty(property)))) =>
+            ErrorString::Ident(property),
+
+        CssParseError::Custom(SelectorParseError::Custom(
+            StyleParseError::UnexpectedTokenWithinNamespace(token))) =>
+            ErrorString::UnexpectedToken(token),
+
+        _ => return None,
+    })
+}
+
+fn extract_value_error_param<'a>(err: ValueParseError<'a>) -> ErrorString<'a> {
+    match err {
+        ValueParseError::InvalidColor(t) => ErrorString::UnexpectedToken(t),
+    }
+}
+
+/// If an error parameter is present in the given error, return it. Additionally return
+/// a second parameter if it exists, for use in the prefix for the eventual error message.
+fn extract_error_params<'a>(err: ParseError<'a>) -> Option<(ErrorString<'a>, Option<ErrorString<'a>>)> {
+    match err {
+        CssParseError::Custom(SelectorParseError::Custom(
+            StyleParseError::PropertyDeclaration(
+                PropertyDeclarationParseError::InvalidValue(property, Some(e))))) =>
+            Some((ErrorString::Snippet(property.into()), Some(extract_value_error_param(e)))),
+
+        err => extract_error_param(err).map(|e| (e, None)),
+    }
 }
 
 impl<'a> ErrorHelpers<'a> for ContextualParseError<'a> {
@@ -222,40 +273,13 @@ impl<'a> ErrorHelpers<'a> for ContextualParseError<'a> {
         }
     }
 
-    fn error_param(self) -> ErrorString<'a> {
+    fn error_params(self) -> (ErrorString<'a>, Option<ErrorString<'a>>) {
-        match self.error_data() {
+        let (s, error) = self.error_data();
-            (_, CssParseError::Basic(BasicParseError::UnexpectedToken(t))) =>
+        extract_error_params(error).unwrap_or((ErrorString::Snippet(s), None))
-                ErrorString::UnexpectedToken(t),
-
-            (_, CssParseError::Basic(BasicParseError::AtRuleInvalid(i))) =>
-                ErrorString::Snippet(format!("@{}", escape_css_ident(&i)).into()),
-
-            (_, CssParseError::Custom(SelectorParseError::Custom(
-                StyleParseError::PropertyDeclaration(
-                    PropertyDeclarationParseError::InvalidValue(property))))) =>
-                ErrorString::Snippet(property),
-
-            (_, CssParseError::Custom(SelectorParseError::UnexpectedIdent(ident))) =>
-                ErrorString::Ident(ident),
-
-            (_, CssParseError::Custom(SelectorParseError::ExpectedNamespace(namespace))) =>
-                ErrorString::Ident(namespace),
-
-            (_, CssParseError::Custom(SelectorParseError::Custom(
-                StyleParseError::PropertyDeclaration(
-                    PropertyDeclarationParseError::UnknownProperty(property))))) =>
-                ErrorString::Ident(property),
-
-            (_, CssParseError::Custom(SelectorParseError::Custom(
-                StyleParseError::UnexpectedTokenWithinNamespace(token)))) =>
-                ErrorString::UnexpectedToken(token),
-
-            (s, _)  => ErrorString::Snippet(s)
-        }
     }
 
-    fn to_gecko_message(&self) -> (&'static [u8], Action) {
+    fn to_gecko_message(&self) -> (Option<&'static [u8]>, &'static [u8], Action) {
-        match *self {
+        let (msg, action): (&[u8], Action) = match *self {
             ContextualParseError::UnsupportedPropertyDeclaration(
                 _, CssParseError::Basic(BasicParseError::UnexpectedToken(_))) |
             ContextualParseError::UnsupportedPropertyDeclaration(
@@ -264,8 +288,13 @@ impl<'a> ErrorHelpers<'a> for ContextualParseError<'a> {
             ContextualParseError::UnsupportedPropertyDeclaration(
                 _, CssParseError::Custom(SelectorParseError::Custom(
                     StyleParseError::PropertyDeclaration(
-                        PropertyDeclarationParseError::InvalidValue(_))))) =>
+                        PropertyDeclarationParseError::InvalidValue(_, ref err))))) => {
-                (b"PEValueParsingError\0", Action::Drop),
+                let prefix = match *err {
+                    Some(ValueParseError::InvalidColor(_)) => Some(&b"PEColorNotColor\0"[..]),
+                    _ => None,
+                };
+                return (prefix, b"PEValueParsingError\0", Action::Drop);
+            }
             ContextualParseError::UnsupportedPropertyDeclaration(..) =>
                 (b"PEUnknownProperty\0", Action::Drop),
             ContextualParseError::UnsupportedFontFaceDescriptor(..) =>
@@ -295,7 +324,8 @@ impl<'a> ErrorHelpers<'a> for ContextualParseError<'a> {
             ContextualParseError::UnsupportedFontFeatureValuesDescriptor(..) |
             ContextualParseError::InvalidFontFeatureValuesRule(..) =>
                 (b"PEUnknownAtRule\0", Action::Skip),
-        }
+        };
+        (None, msg, action)
     }
 }
 
@@ -304,18 +334,21 @@ impl ParseErrorReporter for ErrorReporter {
                         input: &mut Parser,
                         position: SourcePosition,
                         error: ContextualParseError<'a>,
-                        url: &UrlExtraData,
+                        _url: &UrlExtraData,
                         line_number_offset: u64) {
         let location = input.source_location(position);
         let line_number = location.line + line_number_offset as u32;
 
-        let (name, action) = error.to_gecko_message();
+        let (pre, name, action) = error.to_gecko_message();
-        let followup = match action {
+        let suffix = match action {
             Action::Nothing => ptr::null(),
             Action::Skip => b"PEDeclSkipped\0".as_ptr(),
             Action::Drop => b"PEDeclDropped\0".as_ptr(),
         };
-        let param = error.error_param().into_str();
+        let (param, pre_param) = error.error_params();
+        let param = param.into_str();
+        let pre_param = pre_param.map(|p| p.into_str());
+        let pre_param_ptr = pre_param.as_ref().map_or(ptr::null(), |p| p.as_ptr());
         // The CSS source text is unused and will be removed in bug 1381188.
         let source = "";
         unsafe {
@@ -323,12 +356,14 @@ impl ParseErrorReporter for ErrorReporter {
                                            name.as_ptr() as *const _,
                                            param.as_ptr() as *const _,
                                            param.len() as u32,
+                                           pre.map_or(ptr::null(), |p| p.as_ptr()) as *const _,
+                                           pre_param_ptr as *const _,
+                                           pre_param.as_ref().map_or(0, |p| p.len()) as u32,
+                                           suffix as *const _,
                                            source.as_ptr() as *const _,
                                            source.len() as u32,
                                            line_number as u32,
-                                           location.column as u32,
+                                           location.column as u32);
-                                           url.mBaseURI.raw::<nsIURI>(),
-                                           followup as *const _);
         }
     }
 }

tests/unit/style/stylesheets.rs

@@ -378,7 +378,7 @@ fn test_report_error_stylesheet() {
 
     let error = errors.pop().unwrap();
     assert_eq!("Unsupported property declaration: 'display: invalid;', \
-                Custom(PropertyDeclaration(InvalidValue(\"display\")))", error.message);
+                Custom(PropertyDeclaration(InvalidValue(\"display\", None)))", error.message);
     assert_eq!(8, error.line);
     assert_eq!(8, error.column);