From 4b736354c443a0a9e22c8e81d76fc4099bdca249 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Mon, 31 Jul 2017 15:34:29 -0400 Subject: [PATCH] Report an error for invalid CSS color values (bug 1381143). --- components/style/gecko/generated/bindings.rs | 11 +- .../style/properties/properties.mako.rs | 25 ++-- components/style/values/specified/color.rs | 48 ++++--- components/style_traits/lib.rs | 25 +++- ports/geckolib/error_reporter.rs | 125 +++++++++++------- tests/unit/style/stylesheets.rs | 2 +- 6 files changed, 154 insertions(+), 82 deletions(-) diff --git a/components/style/gecko/generated/bindings.rs b/components/style/gecko/generated/bindings.rs index c29c7eb52a2..f0078fa23c2 100644 --- a/components/style/gecko/generated/bindings.rs +++ b/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, - followup: - *const ::std::os::raw::c_char); + colNumber: u32); } diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 0e11d2ac0ad..d98e173db5b 100644 --- a/components/style/properties/properties.mako.rs +++ b/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(|_| { - PropertyDeclarationParseError::InvalidValue(id.name().into()) + ::custom_properties::parse_non_custom_with_var(input).map_err(|e| { + 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(|_| { - PropertyDeclarationParseError::InvalidValue(id.name().into()) + ::custom_properties::parse_non_custom_with_var(input).map_err(|e| { + 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))) } }) } diff --git a/components/style/values/specified/color.rs b/components/style/values/specified/color.rs index 2f91768cc5c..1b2a8c69707 100644 --- a/components/style/values/specified/color.rs +++ b/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) { - Ok(match value { - CSSParserColor::CurrentColor => Color::CurrentColor, - CSSParserColor::RGBA(rgba) => Color::Numeric { - parsed: rgba, - authored: authored, - }, - }) - } else { - #[cfg(feature = "gecko")] { - if let Ok(system) = input.try(SystemColor::parse) { - Ok(Color::System(system)) - } else { - gecko::SpecialColorKeyword::parse(input).map(Color::Special) + match input.try(CSSParserColor::parse) { + Ok(value) => + Ok(match value { + CSSParserColor::CurrentColor => Color::CurrentColor, + CSSParserColor::RGBA(rgba) => Color::Numeric { + parsed: rgba, + authored: authored, + }, + }), + Err(e) => { + #[cfg(feature = "gecko")] { + if let Ok(system) = input.try(SystemColor::parse) { + return Ok(Color::System(system)); + } 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> { - 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) }) } diff --git a/components/style_traits/lib.rs b/components/style_traits/lib.rs index 33c3d15658c..78bb0e3e0e7 100644 --- a/components/style_traits/lib.rs +++ b/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> { + 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>), /// The declaration contained an animation property, and we were parsing /// this as a keyframe block (so that property should be ignored). /// diff --git a/ports/geckolib/error_reporter.rs b/ports/geckolib/error_reporter.rs index 536ad19aa69..f13dc340a11 100644 --- a/ports/geckolib/error_reporter.rs +++ b/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 to_gecko_message(&self) -> (&'static [u8], Action); + fn error_params(self) -> (ErrorString<'a>, Option>); + fn to_gecko_message(&self) -> (Option<&'static [u8]>, &'static [u8], Action); +} + +fn extract_error_param<'a>(err: ParseError<'a>) -> Option> { + 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>)> { + 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> { - match self.error_data() { - (_, 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))))) => - 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 error_params(self) -> (ErrorString<'a>, Option>) { + let (s, error) = self.error_data(); + extract_error_params(error).unwrap_or((ErrorString::Snippet(s), None)) } - fn to_gecko_message(&self) -> (&'static [u8], Action) { - match *self { + fn to_gecko_message(&self) -> (Option<&'static [u8]>, &'static [u8], Action) { + 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(_))))) => - (b"PEValueParsingError\0", Action::Drop), + PropertyDeclarationParseError::InvalidValue(_, ref err))))) => { + 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 followup = match action { + let (pre, name, action) = error.to_gecko_message(); + 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, - url.mBaseURI.raw::(), - followup as *const _); + location.column as u32); } } } diff --git a/tests/unit/style/stylesheets.rs b/tests/unit/style/stylesheets.rs index 4eb371317f9..16a9eca1a10 100644 --- a/tests/unit/style/stylesheets.rs +++ b/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);