From f94612cfeb8ae4e7c3a91c00d9b91453d5204e11 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Mon, 28 Aug 2017 17:18:45 -0700 Subject: [PATCH 1/7] Allow null parameters for Gecko error reporter (bug 1384216). --- ports/geckolib/error_reporter.rs | 42 +++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/ports/geckolib/error_reporter.rs b/ports/geckolib/error_reporter.rs index 28d7648d2f0..583a9db6734 100644 --- a/ports/geckolib/error_reporter.rs +++ b/ports/geckolib/error_reporter.rs @@ -194,7 +194,7 @@ enum Action { trait ErrorHelpers<'a> { fn error_data(self) -> (CowRcStr<'a>, ParseError<'a>); - fn error_params(self) -> (ErrorString<'a>, Option>); + fn error_params(self) -> ErrorParams<'a>; fn to_gecko_message(&self) -> (Option<&'static [u8]>, &'static [u8], Action); } @@ -236,17 +236,29 @@ fn extract_value_error_param<'a>(err: ValueParseError<'a>) -> ErrorString<'a> { } } +struct ErrorParams<'a> { + prefix_param: Option>, + main_param: Option>, +} + /// 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 { +fn extract_error_params<'a>(err: ParseError<'a>) -> Option> { + let (main, prefix) = match err { CssParseError::Custom(SelectorParseError::Custom( StyleParseError::PropertyDeclaration( PropertyDeclarationParseError::InvalidValue(property, Some(e))))) => - Some((ErrorString::Snippet(property.into()), Some(extract_value_error_param(e)))), + (Some(ErrorString::Snippet(property.into())), Some(extract_value_error_param(e))), - err => extract_error_param(err).map(|e| (e, None)), - } + err => match extract_error_param(err) { + Some(e) => (Some(e), None), + None => return None, + } + }; + Some(ErrorParams { + main_param: main, + prefix_param: prefix, + }) } impl<'a> ErrorHelpers<'a> for ContextualParseError<'a> { @@ -273,9 +285,12 @@ impl<'a> ErrorHelpers<'a> for ContextualParseError<'a> { } } - fn error_params(self) -> (ErrorString<'a>, Option>) { + fn error_params(self) -> ErrorParams<'a> { let (s, error) = self.error_data(); - extract_error_params(error).unwrap_or((ErrorString::Snippet(s), None)) + extract_error_params(error).unwrap_or_else(|| ErrorParams { + main_param: Some(ErrorString::Snippet(s)), + prefix_param: None + }) } fn to_gecko_message(&self) -> (Option<&'static [u8]>, &'static [u8], Action) { @@ -340,17 +355,20 @@ impl ParseErrorReporter for ErrorReporter { Action::Skip => b"PEDeclSkipped\0".as_ptr(), Action::Drop => b"PEDeclDropped\0".as_ptr(), }; - let (param, pre_param) = error.error_params(); - let param = param.into_str(); + let params = error.error_params(); + let param = params.main_param; + let pre_param = params.prefix_param; + let param = param.map(|p| p.into_str()); let pre_param = pre_param.map(|p| p.into_str()); + let param_ptr = param.as_ref().map_or(ptr::null(), |p| p.as_ptr()); 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 { Gecko_ReportUnexpectedCSSError(self.0, name.as_ptr() as *const _, - param.as_ptr() as *const _, - param.len() as u32, + param_ptr as *const _, + param.as_ref().map_or(0, |p| p.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, From 8e20a6a110c8dcd09747c7f9201bf77c380f6fae Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Mon, 28 Aug 2017 17:19:28 -0700 Subject: [PATCH 2/7] Report unknown pseudos (bug 1384216). --- components/selectors/parser.rs | 14 +++++++------- components/style/gecko/selector_parser.rs | 12 ++++++------ ports/geckolib/error_reporter.rs | 13 +++++++++++-- 3 files changed, 24 insertions(+), 15 deletions(-) diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index bbc1dc4a246..6d15a741ca4 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -58,7 +58,7 @@ pub enum SelectorParseError<'i, T> { UnexpectedTokenInAttributeSelector, PseudoElementExpectedColon, PseudoElementExpectedIdent, - UnsupportedPseudoClass, + UnsupportedPseudoClassOrElement(CowRcStr<'i>), UnexpectedIdent(CowRcStr<'i>), ExpectedNamespace(CowRcStr<'i>), Custom(T), @@ -136,7 +136,7 @@ pub trait Parser<'i> { fn parse_non_ts_pseudo_class(&self, name: CowRcStr<'i>) -> Result<::NonTSPseudoClass, ParseError<'i, SelectorParseError<'i, Self::Error>>> { - Err(ParseError::Custom(SelectorParseError::UnexpectedIdent(name))) + Err(ParseError::Custom(SelectorParseError::UnsupportedPseudoClassOrElement(name))) } fn parse_non_ts_functional_pseudo_class<'t> @@ -144,20 +144,20 @@ pub trait Parser<'i> { -> Result<::NonTSPseudoClass, ParseError<'i, SelectorParseError<'i, Self::Error>>> { - Err(ParseError::Custom(SelectorParseError::UnexpectedIdent(name))) + Err(ParseError::Custom(SelectorParseError::UnsupportedPseudoClassOrElement(name))) } fn parse_pseudo_element(&self, name: CowRcStr<'i>) -> Result<::PseudoElement, ParseError<'i, SelectorParseError<'i, Self::Error>>> { - Err(ParseError::Custom(SelectorParseError::UnexpectedIdent(name))) + Err(ParseError::Custom(SelectorParseError::UnsupportedPseudoClassOrElement(name))) } fn parse_functional_pseudo_element<'t> (&self, name: CowRcStr<'i>, _arguments: &mut CssParser<'i, 't>) -> Result<::PseudoElement, ParseError<'i, SelectorParseError<'i, Self::Error>>> { - Err(ParseError::Custom(SelectorParseError::UnexpectedIdent(name))) + Err(ParseError::Custom(SelectorParseError::UnsupportedPseudoClassOrElement(name))) } fn default_namespace(&self) -> Option<::NamespaceUrl> { @@ -1503,9 +1503,9 @@ fn parse_compound_selector<'i, 't, P, E, Impl>( }; let pseudo_class = - P::parse_non_ts_pseudo_class(parser, name)?; + P::parse_non_ts_pseudo_class(parser, name.clone())?; if !p.supports_pseudo_class(&pseudo_class) { - return Err(SelectorParseError::UnsupportedPseudoClass.into()); + return Err(SelectorParseError::UnsupportedPseudoClassOrElement(name).into()); } state_selectors.push(Component::NonTSPseudoClass(pseudo_class)); } diff --git a/components/style/gecko/selector_parser.rs b/components/style/gecko/selector_parser.rs index 21c12305e6a..8000c0af5f4 100644 --- a/components/style/gecko/selector_parser.rs +++ b/components/style/gecko/selector_parser.rs @@ -308,7 +308,7 @@ impl<'a, 'i> ::selectors::Parser<'i> for SelectorParser<'a> { keyword: [$(($k_css:expr, $k_name:ident, $k_gecko_type:tt, $k_state:tt, $k_flags:tt),)*]) => { match_ignore_ascii_case! { &name, $($css => NonTSPseudoClass::$name,)* - _ => return Err(::selectors::parser::SelectorParseError::UnexpectedIdent( + _ => return Err(::selectors::parser::SelectorParseError::UnsupportedPseudoClassOrElement( name.clone()).into()) } } @@ -317,7 +317,7 @@ impl<'a, 'i> ::selectors::Parser<'i> for SelectorParser<'a> { if self.is_pseudo_class_enabled(&pseudo_class) { Ok(pseudo_class) } else { - Err(SelectorParseError::UnexpectedIdent(name).into()) + Err(SelectorParseError::UnsupportedPseudoClassOrElement(name).into()) } } @@ -354,7 +354,7 @@ impl<'a, 'i> ::selectors::Parser<'i> for SelectorParser<'a> { } NonTSPseudoClass::MozAny(selectors.into_boxed_slice()) } - _ => return Err(SelectorParseError::UnexpectedIdent(name.clone()).into()) + _ => return Err(SelectorParseError::UnsupportedPseudoClassOrElement(name.clone()).into()) } } } @@ -362,13 +362,13 @@ impl<'a, 'i> ::selectors::Parser<'i> for SelectorParser<'a> { if self.is_pseudo_class_enabled(&pseudo_class) { Ok(pseudo_class) } else { - Err(SelectorParseError::UnexpectedIdent(name).into()) + Err(SelectorParseError::UnsupportedPseudoClassOrElement(name).into()) } } fn parse_pseudo_element(&self, name: CowRcStr<'i>) -> Result> { PseudoElement::from_slice(&name, self.in_user_agent_stylesheet()) - .ok_or(SelectorParseError::UnexpectedIdent(name.clone()).into()) + .ok_or(SelectorParseError::UnsupportedPseudoClassOrElement(name.clone()).into()) } fn parse_functional_pseudo_element<'t>(&self, name: CowRcStr<'i>, @@ -392,7 +392,7 @@ impl<'a, 'i> ::selectors::Parser<'i> for SelectorParser<'a> { return Ok(pseudo); } } - Err(SelectorParseError::UnexpectedIdent(name.clone()).into()) + Err(SelectorParseError::UnsupportedPseudoClassOrElement(name.clone()).into()) } fn default_namespace(&self) -> Option { diff --git a/ports/geckolib/error_reporter.rs b/ports/geckolib/error_reporter.rs index 583a9db6734..d487f649fef 100644 --- a/ports/geckolib/error_reporter.rs +++ b/ports/geckolib/error_reporter.rs @@ -250,6 +250,9 @@ fn extract_error_params<'a>(err: ParseError<'a>) -> Option> { PropertyDeclarationParseError::InvalidValue(property, Some(e))))) => (Some(ErrorString::Snippet(property.into())), Some(extract_value_error_param(e))), + CssParseError::Custom(SelectorParseError::UnsupportedPseudoClassOrElement(p)) => + (None, Some(ErrorString::Ident(p))), + err => match extract_error_param(err) { Some(e) => (Some(e), None), None => return None, @@ -325,8 +328,14 @@ impl<'a> ErrorHelpers<'a> for ContextualParseError<'a> { _, CssParseError::Custom(SelectorParseError::Custom( StyleParseError::UnexpectedTokenWithinNamespace(_)))) => (b"PEAtNSUnexpected\0", Action::Nothing), - ContextualParseError::InvalidRule(..) => - (b"PEBadSelectorRSIgnored\0", Action::Nothing), + ContextualParseError::InvalidRule(_, ref err) => { + let prefix = match *err { + CssParseError::Custom(SelectorParseError::UnsupportedPseudoClassOrElement(_)) => + Some(&b"PEPseudoSelUnknown\0"[..]), + _ => None, + }; + return (prefix, b"PEBadSelectorRSIgnored\0", Action::Nothing); + } ContextualParseError::UnsupportedRule(..) => (b"PEDeclDropped\0", Action::Nothing), ContextualParseError::UnsupportedViewportDescriptorDeclaration(..) | From 73e903ad3cc78f7bfb4c2d3989e071c1494067c6 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Mon, 28 Aug 2017 17:20:17 -0700 Subject: [PATCH 3/7] Report unexpected attribute selector tokens (bug 1384216). --- components/selectors/parser.rs | 59 +++++++++++++------------------- ports/geckolib/error_reporter.rs | 5 +++ 2 files changed, 28 insertions(+), 36 deletions(-) diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index 6d15a741ca4..de87ae3cc4b 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -55,7 +55,7 @@ pub enum SelectorParseError<'i, T> { NegationSelectorComponentNotLocalName, EmptySelector, NonSimpleSelectorInNegation, - UnexpectedTokenInAttributeSelector, + UnexpectedTokenInAttributeSelector(Token<'i>), PseudoElementExpectedColon, PseudoElementExpectedIdent, UnsupportedPseudoClassOrElement(CowRcStr<'i>), @@ -1292,10 +1292,7 @@ fn parse_attribute_selector<'i, 't, P, E, Impl>(parser: &P, input: &mut CssParse } } - let operator; - let value; - let never_matches; - match input.next() { + let operator = match input.next() { // [foo] Err(_) => { let local_name_lower = to_ascii_lowercase(&local_name).as_ref().into(); @@ -1317,43 +1314,33 @@ fn parse_attribute_selector<'i, 't, P, E, Impl>(parser: &P, input: &mut CssParse } // [foo=bar] - Ok(&Token::Delim('=')) => { - value = input.expect_ident_or_string()?.clone(); - never_matches = false; - operator = AttrSelectorOperator::Equal; - } + Ok(&Token::Delim('=')) => AttrSelectorOperator::Equal, // [foo~=bar] - Ok(&Token::IncludeMatch) => { - value = input.expect_ident_or_string()?.clone(); - never_matches = value.is_empty() || value.contains(SELECTOR_WHITESPACE); - operator = AttrSelectorOperator::Includes; - } + Ok(&Token::IncludeMatch) => AttrSelectorOperator::Includes, // [foo|=bar] - Ok(&Token::DashMatch) => { - value = input.expect_ident_or_string()?.clone(); - never_matches = false; - operator = AttrSelectorOperator::DashMatch; - } + Ok(&Token::DashMatch) => AttrSelectorOperator::DashMatch, // [foo^=bar] - Ok(&Token::PrefixMatch) => { - value = input.expect_ident_or_string()?.clone(); - never_matches = value.is_empty(); - operator = AttrSelectorOperator::Prefix; - } + Ok(&Token::PrefixMatch) => AttrSelectorOperator::Prefix, // [foo*=bar] - Ok(&Token::SubstringMatch) => { - value = input.expect_ident_or_string()?.clone(); - never_matches = value.is_empty(); - operator = AttrSelectorOperator::Substring; - } + Ok(&Token::SubstringMatch) => AttrSelectorOperator::Substring, // [foo$=bar] - Ok(&Token::SuffixMatch) => { - value = input.expect_ident_or_string()?.clone(); - never_matches = value.is_empty(); - operator = AttrSelectorOperator::Suffix; + Ok(&Token::SuffixMatch) => AttrSelectorOperator::Suffix, + Ok(t) => return Err(SelectorParseError::UnexpectedTokenInAttributeSelector(t.clone()).into()) + }; + + let value = input.expect_ident_or_string()?.clone(); + let never_matches = match operator { + AttrSelectorOperator::Equal | + AttrSelectorOperator::DashMatch => false, + + AttrSelectorOperator::Includes => { + value.is_empty() || value.contains(SELECTOR_WHITESPACE) } - _ => return Err(SelectorParseError::UnexpectedTokenInAttributeSelector.into()) - } + + AttrSelectorOperator::Prefix | + AttrSelectorOperator::Substring | + AttrSelectorOperator::Suffix => value.is_empty() + }; let mut case_sensitivity = parse_attribute_flags(input)?; diff --git a/ports/geckolib/error_reporter.rs b/ports/geckolib/error_reporter.rs index d487f649fef..f82c41c8562 100644 --- a/ports/geckolib/error_reporter.rs +++ b/ports/geckolib/error_reporter.rs @@ -250,6 +250,9 @@ fn extract_error_params<'a>(err: ParseError<'a>) -> Option> { PropertyDeclarationParseError::InvalidValue(property, Some(e))))) => (Some(ErrorString::Snippet(property.into())), Some(extract_value_error_param(e))), + CssParseError::Custom(SelectorParseError::UnexpectedTokenInAttributeSelector(t)) => + (None, Some(ErrorString::UnexpectedToken(t))), + CssParseError::Custom(SelectorParseError::UnsupportedPseudoClassOrElement(p)) => (None, Some(ErrorString::Ident(p))), @@ -330,6 +333,8 @@ impl<'a> ErrorHelpers<'a> for ContextualParseError<'a> { (b"PEAtNSUnexpected\0", Action::Nothing), ContextualParseError::InvalidRule(_, ref err) => { let prefix = match *err { + CssParseError::Custom(SelectorParseError::UnexpectedTokenInAttributeSelector(_)) => + Some(&b"PEAttSelUnexpected\0"[..]), CssParseError::Custom(SelectorParseError::UnsupportedPseudoClassOrElement(_)) => Some(&b"PEPseudoSelUnknown\0"[..]), _ => None, From 408c34a76d8c86297a7f0bec9d58b36362b22282 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Mon, 28 Aug 2017 17:20:45 -0700 Subject: [PATCH 4/7] Report more specific invalid attribute selectors (bug 1384216). --- components/selectors/parser.rs | 54 ++++++++++++++++++++++---------- ports/geckolib/error_reporter.rs | 25 ++++++++++----- 2 files changed, 55 insertions(+), 24 deletions(-) diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index de87ae3cc4b..08ad801781d 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -49,7 +49,7 @@ fn to_ascii_lowercase(s: &str) -> Cow { #[derive(Clone, Debug, PartialEq)] pub enum SelectorParseError<'i, T> { PseudoElementInComplexSelector, - NoQualifiedNameInAttributeSelector, + NoQualifiedNameInAttributeSelector(Token<'i>), TooManyCompoundSelectorComponentsInNegation, NegationSelectorComponentNotNamespace, NegationSelectorComponentNotLocalName, @@ -61,6 +61,9 @@ pub enum SelectorParseError<'i, T> { UnsupportedPseudoClassOrElement(CowRcStr<'i>), UnexpectedIdent(CowRcStr<'i>), ExpectedNamespace(CowRcStr<'i>), + ExpectedBarInAttr(Token<'i>), + BadValueInAttr(Token<'i>), + InvalidQualNameInAttr(Token<'i>), Custom(T), } @@ -1107,8 +1110,8 @@ fn parse_type_selector<'i, 't, P, E, Impl, S>(parser: &P, input: &mut CssParser< S: Push>, { match parse_qualified_name(parser, input, /* in_attr_selector = */ false)? { - None => Ok(false), - Some((namespace, local_name)) => { + OptionalQName::None(_) => Ok(false), + OptionalQName::Some(namespace, local_name) => { match namespace { QNamePrefix::ImplicitAnyNamespace => {} QNamePrefix::ImplicitDefaultNamespace(url) => { @@ -1176,13 +1179,19 @@ enum QNamePrefix { ExplicitNamespace(Impl::NamespacePrefix, Impl::NamespaceUrl), // `prefix|foo` } +enum OptionalQName<'i, Impl: SelectorImpl> { + Some(QNamePrefix, Option>), + None(Token<'i>), +} + /// * `Err(())`: Invalid selector, abort -/// * `Ok(None)`: Not a simple selector, could be something else. `input` was not consumed. -/// * `Ok(Some((namespace, local_name)))`: `None` for the local name means a `*` universal selector +/// * `Ok(None(token))`: Not a simple selector, could be something else. `input` was not consumed, +/// but the token is still returned. +/// * `Ok(Some(namespace, local_name))`: `None` for the local name means a `*` universal selector fn parse_qualified_name<'i, 't, P, E, Impl> (parser: &P, input: &mut CssParser<'i, 't>, in_attr_selector: bool) - -> Result, Option>)>, + -> Result, ParseError<'i, SelectorParseError<'i, E>>> where P: Parser<'i, Impl=Impl, Error=E>, Impl: SelectorImpl { @@ -1191,17 +1200,18 @@ fn parse_qualified_name<'i, 't, P, E, Impl> Some(url) => QNamePrefix::ImplicitDefaultNamespace(url), None => QNamePrefix::ImplicitAnyNamespace, }; - Ok(Some((namespace, local_name))) + Ok(OptionalQName::Some(namespace, local_name)) }; let explicit_namespace = |input: &mut CssParser<'i, 't>, namespace| { match input.next_including_whitespace() { Ok(&Token::Delim('*')) if !in_attr_selector => { - Ok(Some((namespace, None))) + Ok(OptionalQName::Some(namespace, None)) }, Ok(&Token::Ident(ref local_name)) => { - Ok(Some((namespace, Some(local_name.clone())))) + Ok(OptionalQName::Some(namespace, Some(local_name.clone()))) }, + Ok(t) if in_attr_selector => Err(SelectorParseError::InvalidQualNameInAttr(t.clone()).into()), Ok(t) => Err(ParseError::Basic(BasicParseError::UnexpectedToken(t.clone()))), Err(e) => Err(ParseError::Basic(e)), } @@ -1223,7 +1233,7 @@ fn parse_qualified_name<'i, 't, P, E, Impl> _ => { input.reset(&after_ident); if in_attr_selector { - Ok(Some((QNamePrefix::ImplicitNoNamespace, Some(value)))) + Ok(OptionalQName::Some(QNamePrefix::ImplicitNoNamespace, Some(value))) } else { default_namespace(Some(value)) } @@ -1241,7 +1251,7 @@ fn parse_qualified_name<'i, 't, P, E, Impl> input.reset(&after_star); if in_attr_selector { match result { - Ok(t) => Err(ParseError::Basic(BasicParseError::UnexpectedToken(t))), + Ok(t) => Err(SelectorParseError::ExpectedBarInAttr(t).into()), Err(e) => Err(ParseError::Basic(e)), } } else { @@ -1253,9 +1263,13 @@ fn parse_qualified_name<'i, 't, P, E, Impl> Ok(Token::Delim('|')) => { explicit_namespace(input, QNamePrefix::ExplicitNoNamespace) } - _ => { + Ok(t) => { input.reset(&start); - Ok(None) + Ok(OptionalQName::None(t)) + } + Err(e) => { + input.reset(&start); + Err(e.into()) } } } @@ -1269,9 +1283,10 @@ fn parse_attribute_selector<'i, 't, P, E, Impl>(parser: &P, input: &mut CssParse let namespace; let local_name; match parse_qualified_name(parser, input, /* in_attr_selector = */ true)? { - None => return Err(ParseError::Custom(SelectorParseError::NoQualifiedNameInAttributeSelector)), - Some((_, None)) => unreachable!(), - Some((ns, Some(ln))) => { + OptionalQName::None(t) => + return Err(ParseError::Custom(SelectorParseError::NoQualifiedNameInAttributeSelector(t))), + OptionalQName::Some(_, None) => unreachable!(), + OptionalQName::Some(ns, Some(ln)) => { local_name = ln; namespace = match ns { QNamePrefix::ImplicitNoNamespace | @@ -1328,7 +1343,12 @@ fn parse_attribute_selector<'i, 't, P, E, Impl>(parser: &P, input: &mut CssParse Ok(t) => return Err(SelectorParseError::UnexpectedTokenInAttributeSelector(t.clone()).into()) }; - let value = input.expect_ident_or_string()?.clone(); + let value = match input.expect_ident_or_string() { + Ok(t) => t.clone(), + Err(BasicParseError::UnexpectedToken(t)) => + return Err(SelectorParseError::BadValueInAttr(t.clone()).into()), + Err(e) => return Err(e.into()), + }; let never_matches = match operator { AttrSelectorOperator::Equal | AttrSelectorOperator::DashMatch => false, diff --git a/ports/geckolib/error_reporter.rs b/ports/geckolib/error_reporter.rs index f82c41c8562..0518750271b 100644 --- a/ports/geckolib/error_reporter.rs +++ b/ports/geckolib/error_reporter.rs @@ -214,9 +214,6 @@ fn extract_error_param<'a>(err: ParseError<'a>) -> Option> { 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)))) => @@ -250,9 +247,16 @@ fn extract_error_params<'a>(err: ParseError<'a>) -> Option> { PropertyDeclarationParseError::InvalidValue(property, Some(e))))) => (Some(ErrorString::Snippet(property.into())), Some(extract_value_error_param(e))), - CssParseError::Custom(SelectorParseError::UnexpectedTokenInAttributeSelector(t)) => + CssParseError::Custom(SelectorParseError::UnexpectedTokenInAttributeSelector(t)) | + CssParseError::Custom(SelectorParseError::BadValueInAttr(t)) | + CssParseError::Custom(SelectorParseError::ExpectedBarInAttr(t)) | + CssParseError::Custom(SelectorParseError::NoQualifiedNameInAttributeSelector(t)) | + CssParseError::Custom(SelectorParseError::InvalidQualNameInAttr(t)) => (None, Some(ErrorString::UnexpectedToken(t))), + CssParseError::Custom(SelectorParseError::ExpectedNamespace(namespace)) => + (None, Some(ErrorString::Ident(namespace))), + CssParseError::Custom(SelectorParseError::UnsupportedPseudoClassOrElement(p)) => (None, Some(ErrorString::Ident(p))), @@ -324,9 +328,6 @@ impl<'a> ErrorHelpers<'a> for ContextualParseError<'a> { (b"PEKeyframeBadName\0", Action::Nothing), ContextualParseError::UnsupportedKeyframePropertyDeclaration(..) => (b"PEBadSelectorKeyframeRuleIgnored\0", Action::Nothing), - ContextualParseError::InvalidRule( - _, CssParseError::Custom(SelectorParseError::ExpectedNamespace(_))) => - (b"PEUnknownNamespacePrefix\0", Action::Nothing), ContextualParseError::InvalidRule( _, CssParseError::Custom(SelectorParseError::Custom( StyleParseError::UnexpectedTokenWithinNamespace(_)))) => @@ -335,6 +336,16 @@ impl<'a> ErrorHelpers<'a> for ContextualParseError<'a> { let prefix = match *err { CssParseError::Custom(SelectorParseError::UnexpectedTokenInAttributeSelector(_)) => Some(&b"PEAttSelUnexpected\0"[..]), + CssParseError::Custom(SelectorParseError::ExpectedBarInAttr(_)) => + Some(&b"PEAttSelNoBar\0"[..]), + CssParseError::Custom(SelectorParseError::BadValueInAttr(_)) => + Some(&b"PEAttSelBadValue\0"[..]), + CssParseError::Custom(SelectorParseError::NoQualifiedNameInAttributeSelector(_)) => + Some(&b"PEAttributeNameOrNamespaceExpected\0"[..]), + CssParseError::Custom(SelectorParseError::InvalidQualNameInAttr(_)) => + Some(&b"PEAttributeNameExpected\0"[..]), + CssParseError::Custom(SelectorParseError::ExpectedNamespace(_)) => + Some(&b"PEUnknownNamespacePrefix\0"[..]), CssParseError::Custom(SelectorParseError::UnsupportedPseudoClassOrElement(_)) => Some(&b"PEPseudoSelUnknown\0"[..]), _ => None, From 9a7cceb0a1eafb28607e1f7a7773bf342108a8b9 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Mon, 28 Aug 2017 17:21:07 -0700 Subject: [PATCH 5/7] Report invalid selectors (bug 1384216). --- components/selectors/builder.rs | 6 ++++++ components/selectors/parser.rs | 35 +++++++++++++++++++++----------- ports/geckolib/error_reporter.rs | 24 ++++++++++++++++++++-- 3 files changed, 51 insertions(+), 14 deletions(-) diff --git a/components/selectors/builder.rs b/components/selectors/builder.rs index 7195d63f14c..99a8f086915 100644 --- a/components/selectors/builder.rs +++ b/components/selectors/builder.rs @@ -90,6 +90,12 @@ impl SelectorBuilder { self.simple_selectors.is_empty() } + /// Returns true if combinators have ever been pushed to this builder. + #[inline(always)] + pub fn has_combinators(&self) -> bool { + !self.combinators.is_empty() + } + /// Consumes the builder, producing a Selector. #[inline(always)] pub fn build(&mut self, parsed_pseudo: bool) -> ThinArc> { diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index 08ad801781d..ac5d184cb9b 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -54,16 +54,19 @@ pub enum SelectorParseError<'i, T> { NegationSelectorComponentNotNamespace, NegationSelectorComponentNotLocalName, EmptySelector, + DanglingCombinator, NonSimpleSelectorInNegation, UnexpectedTokenInAttributeSelector(Token<'i>), - PseudoElementExpectedColon, - PseudoElementExpectedIdent, + PseudoElementExpectedColon(Token<'i>), + PseudoElementExpectedIdent(Token<'i>), + NoIdentForPseudo(Token<'i>), UnsupportedPseudoClassOrElement(CowRcStr<'i>), UnexpectedIdent(CowRcStr<'i>), ExpectedNamespace(CowRcStr<'i>), ExpectedBarInAttr(Token<'i>), BadValueInAttr(Token<'i>), InvalidQualNameInAttr(Token<'i>), + ExplicitNamespaceUnexpectedToken(Token<'i>), Custom(T), } @@ -1044,7 +1047,12 @@ fn parse_selector<'i, 't, P, E, Impl>( let mut parsed_pseudo_element; 'outer_loop: loop { // Parse a sequence of simple selectors. - parsed_pseudo_element = parse_compound_selector(parser, input, &mut builder)?; + parsed_pseudo_element = match parse_compound_selector(parser, input, &mut builder) { + Ok(result) => result, + Err(ParseError::Custom(SelectorParseError::EmptySelector)) if builder.has_combinators() => + return Err(SelectorParseError::DanglingCombinator.into()), + Err(e) => return Err(e), + }; if parsed_pseudo_element { break; } @@ -1109,9 +1117,10 @@ fn parse_type_selector<'i, 't, P, E, Impl, S>(parser: &P, input: &mut CssParser< Impl: SelectorImpl, S: Push>, { - match parse_qualified_name(parser, input, /* in_attr_selector = */ false)? { - OptionalQName::None(_) => Ok(false), - OptionalQName::Some(namespace, local_name) => { + match parse_qualified_name(parser, input, /* in_attr_selector = */ false) { + Err(ParseError::Basic(BasicParseError::EndOfInput)) | + Ok(OptionalQName::None(_)) => Ok(false), + Ok(OptionalQName::Some(namespace, local_name)) => { match namespace { QNamePrefix::ImplicitAnyNamespace => {} QNamePrefix::ImplicitDefaultNamespace(url) => { @@ -1160,6 +1169,7 @@ fn parse_type_selector<'i, 't, P, E, Impl, S>(parser: &P, input: &mut CssParser< } Ok(true) } + Err(e) => Err(e) } } @@ -1212,7 +1222,7 @@ fn parse_qualified_name<'i, 't, P, E, Impl> Ok(OptionalQName::Some(namespace, Some(local_name.clone()))) }, Ok(t) if in_attr_selector => Err(SelectorParseError::InvalidQualNameInAttr(t.clone()).into()), - Ok(t) => Err(ParseError::Basic(BasicParseError::UnexpectedToken(t.clone()))), + Ok(t) => Err(SelectorParseError::ExplicitNamespaceUnexpectedToken(t.clone()).into()), Err(e) => Err(ParseError::Basic(e)), } }; @@ -1499,14 +1509,15 @@ fn parse_compound_selector<'i, 't, P, E, Impl>( match input.next_including_whitespace() { Ok(&Token::Colon) => {}, Ok(&Token::WhiteSpace(_)) | Err(_) => break, - _ => return Err(SelectorParseError::PseudoElementExpectedColon.into()), + Ok(t) => + return Err(SelectorParseError::PseudoElementExpectedColon(t.clone()).into()), } // TODO(emilio): Functional pseudo-classes too? // We don't need it for now. - let name = match input.next_including_whitespace() { - Ok(&Token::Ident(ref name)) => name.clone(), - _ => return Err(SelectorParseError::PseudoElementExpectedIdent.into()), + let name = match input.next_including_whitespace()? { + &Token::Ident(ref name) => name.clone(), + t => return Err(SelectorParseError::NoIdentForPseudo(t.clone()).into()), }; let pseudo_class = @@ -1626,7 +1637,7 @@ fn parse_one_simple_selector<'i, 't, P, E, Impl>(parser: &P, let (name, is_functional) = match next_token { Token::Ident(name) => (name, false), Token::Function(name) => (name, true), - t => return Err(ParseError::Basic(BasicParseError::UnexpectedToken(t))), + t => return Err(SelectorParseError::PseudoElementExpectedIdent(t).into()), }; let is_pseudo_element = !is_single_colon || P::is_pseudo_element_allows_single_colon(&name); diff --git a/ports/geckolib/error_reporter.rs b/ports/geckolib/error_reporter.rs index 0518750271b..1f2c683aca3 100644 --- a/ports/geckolib/error_reporter.rs +++ b/ports/geckolib/error_reporter.rs @@ -161,7 +161,7 @@ fn token_to_str<'a>(t: Token<'a>) -> String { format!("{}{}", i, escape_css_ident(&*unit)), Token::Dimension { value, ref unit, .. } => format!("{}{}", value, escape_css_ident(&*unit)), - Token::WhiteSpace(_) => "whitespace".into(), + Token::WhiteSpace(s) => s.into(), Token::Comment(_) => "comment".into(), Token::Colon => ":".into(), Token::Semicolon => ";".into(), @@ -251,7 +251,11 @@ fn extract_error_params<'a>(err: ParseError<'a>) -> Option> { CssParseError::Custom(SelectorParseError::BadValueInAttr(t)) | CssParseError::Custom(SelectorParseError::ExpectedBarInAttr(t)) | CssParseError::Custom(SelectorParseError::NoQualifiedNameInAttributeSelector(t)) | - CssParseError::Custom(SelectorParseError::InvalidQualNameInAttr(t)) => + CssParseError::Custom(SelectorParseError::InvalidQualNameInAttr(t)) | + CssParseError::Custom(SelectorParseError::ExplicitNamespaceUnexpectedToken(t)) | + CssParseError::Custom(SelectorParseError::PseudoElementExpectedIdent(t)) | + CssParseError::Custom(SelectorParseError::NoIdentForPseudo(t)) | + CssParseError::Custom(SelectorParseError::PseudoElementExpectedColon(t)) => (None, Some(ErrorString::UnexpectedToken(t))), CssParseError::Custom(SelectorParseError::ExpectedNamespace(namespace)) => @@ -260,6 +264,10 @@ fn extract_error_params<'a>(err: ParseError<'a>) -> Option> { CssParseError::Custom(SelectorParseError::UnsupportedPseudoClassOrElement(p)) => (None, Some(ErrorString::Ident(p))), + CssParseError::Custom(SelectorParseError::EmptySelector) | + CssParseError::Custom(SelectorParseError::DanglingCombinator) => + (None, None), + err => match extract_error_param(err) { Some(e) => (Some(e), None), None => return None, @@ -344,10 +352,22 @@ impl<'a> ErrorHelpers<'a> for ContextualParseError<'a> { Some(&b"PEAttributeNameOrNamespaceExpected\0"[..]), CssParseError::Custom(SelectorParseError::InvalidQualNameInAttr(_)) => Some(&b"PEAttributeNameExpected\0"[..]), + CssParseError::Custom(SelectorParseError::ExplicitNamespaceUnexpectedToken(_)) => + Some(&b"PETypeSelNotType\0"[..]), CssParseError::Custom(SelectorParseError::ExpectedNamespace(_)) => Some(&b"PEUnknownNamespacePrefix\0"[..]), + CssParseError::Custom(SelectorParseError::EmptySelector) => + Some(&b"PESelectorGroupNoSelector\0"[..]), + CssParseError::Custom(SelectorParseError::DanglingCombinator) => + Some(&b"PESelectorGroupExtraCombinator\0"[..]), CssParseError::Custom(SelectorParseError::UnsupportedPseudoClassOrElement(_)) => Some(&b"PEPseudoSelUnknown\0"[..]), + CssParseError::Custom(SelectorParseError::PseudoElementExpectedColon(_)) => + Some(&b"PEPseudoSelEndOrUserActionPC\0"[..]), + CssParseError::Custom(SelectorParseError::NoIdentForPseudo(_)) => + Some(&b"PEPseudoClassArgNotIdent\0"[..]), + CssParseError::Custom(SelectorParseError::PseudoElementExpectedIdent(_)) => + Some(&b"PEPseudoSelBadName\0"[..]), _ => None, }; return (prefix, b"PEBadSelectorRSIgnored\0", Action::Nothing); From 4016371e1d608d7c3414a02c36f5834189e4ed38 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Mon, 28 Aug 2017 17:21:57 -0700 Subject: [PATCH 6/7] Report more invalid selectors (bug 1384216). --- components/selectors/parser.rs | 17 +++++++++++------ ports/geckolib/error_reporter.rs | 8 ++++++++ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index ac5d184cb9b..8cacbe6f3a2 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -50,9 +50,6 @@ fn to_ascii_lowercase(s: &str) -> Cow { pub enum SelectorParseError<'i, T> { PseudoElementInComplexSelector, NoQualifiedNameInAttributeSelector(Token<'i>), - TooManyCompoundSelectorComponentsInNegation, - NegationSelectorComponentNotNamespace, - NegationSelectorComponentNotLocalName, EmptySelector, DanglingCombinator, NonSimpleSelectorInNegation, @@ -67,6 +64,8 @@ pub enum SelectorParseError<'i, T> { BadValueInAttr(Token<'i>), InvalidQualNameInAttr(Token<'i>), ExplicitNamespaceUnexpectedToken(Token<'i>), + ClassNeedsIdent(Token<'i>), + EmptyNegation, Custom(T), } @@ -1446,13 +1445,19 @@ fn parse_negation<'i, 't, P, E, Impl>(parser: &P, // Get exactly one simple selector. The parse logic in the caller will verify // that there are no trailing tokens after we're done. - if !parse_type_selector(parser, input, &mut sequence)? { + let is_type_sel = match parse_type_selector(parser, input, &mut sequence) { + Ok(result) => result, + Err(ParseError::Basic(BasicParseError::EndOfInput)) => + return Err(SelectorParseError::EmptyNegation.into()), + Err(e) => return Err(e.into()), + }; + if !is_type_sel { match parse_one_simple_selector(parser, input, /* inside_negation = */ true)? { Some(SimpleSelectorParseResult::SimpleSelector(s)) => { sequence.push(s); }, None => { - return Err(ParseError::Custom(SelectorParseError::EmptySelector)); + return Err(ParseError::Custom(SelectorParseError::EmptyNegation)); }, Some(SimpleSelectorParseResult::PseudoElement(_)) => { return Err(ParseError::Custom(SelectorParseError::NonSimpleSelectorInNegation)); @@ -1622,7 +1627,7 @@ fn parse_one_simple_selector<'i, 't, P, E, Impl>(parser: &P, let class = Component::Class(class.as_ref().into()); Ok(Some(SimpleSelectorParseResult::SimpleSelector(class))) } - ref t => Err(ParseError::Basic(BasicParseError::UnexpectedToken(t.clone()))), + ref t => Err(SelectorParseError::ClassNeedsIdent(t.clone()).into()), } } Ok(Token::SquareBracketBlock) => { diff --git a/ports/geckolib/error_reporter.rs b/ports/geckolib/error_reporter.rs index 1f2c683aca3..67afdb0dfba 100644 --- a/ports/geckolib/error_reporter.rs +++ b/ports/geckolib/error_reporter.rs @@ -255,6 +255,7 @@ fn extract_error_params<'a>(err: ParseError<'a>) -> Option> { CssParseError::Custom(SelectorParseError::ExplicitNamespaceUnexpectedToken(t)) | CssParseError::Custom(SelectorParseError::PseudoElementExpectedIdent(t)) | CssParseError::Custom(SelectorParseError::NoIdentForPseudo(t)) | + CssParseError::Custom(SelectorParseError::ClassNeedsIdent(t)) | CssParseError::Custom(SelectorParseError::PseudoElementExpectedColon(t)) => (None, Some(ErrorString::UnexpectedToken(t))), @@ -268,6 +269,9 @@ fn extract_error_params<'a>(err: ParseError<'a>) -> Option> { CssParseError::Custom(SelectorParseError::DanglingCombinator) => (None, None), + CssParseError::Custom(SelectorParseError::EmptyNegation) => + (None, Some(ErrorString::Snippet(")".into()))), + err => match extract_error_param(err) { Some(e) => (Some(e), None), None => return None, @@ -368,6 +372,10 @@ impl<'a> ErrorHelpers<'a> for ContextualParseError<'a> { Some(&b"PEPseudoClassArgNotIdent\0"[..]), CssParseError::Custom(SelectorParseError::PseudoElementExpectedIdent(_)) => Some(&b"PEPseudoSelBadName\0"[..]), + CssParseError::Custom(SelectorParseError::ClassNeedsIdent(_)) => + Some(&b"PEClassSelNotIdent\0"[..]), + CssParseError::Custom(SelectorParseError::EmptyNegation) => + Some(&b"PENegationBadArg\0"[..]), _ => None, }; return (prefix, b"PEBadSelectorRSIgnored\0", Action::Nothing); From 549cdb27d9ce5849c9e212a2b5c305d6f227c5be Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Mon, 28 Aug 2017 17:22:24 -0700 Subject: [PATCH 7/7] Report invalid at-rules (bug 1384216). --- ports/geckolib/error_reporter.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/ports/geckolib/error_reporter.rs b/ports/geckolib/error_reporter.rs index 67afdb0dfba..09875015ed6 100644 --- a/ports/geckolib/error_reporter.rs +++ b/ports/geckolib/error_reporter.rs @@ -203,7 +203,9 @@ fn extract_error_param<'a>(err: ParseError<'a>) -> Option> { CssParseError::Basic(BasicParseError::UnexpectedToken(t)) => ErrorString::UnexpectedToken(t), - CssParseError::Basic(BasicParseError::AtRuleInvalid(i)) => + CssParseError::Basic(BasicParseError::AtRuleInvalid(i)) | + CssParseError::Custom(SelectorParseError::Custom( + StyleParseError::UnsupportedAtRule(i))) => ErrorString::Snippet(format!("@{}", escape_css_ident(&i)).into()), CssParseError::Custom(SelectorParseError::Custom( @@ -344,6 +346,12 @@ impl<'a> ErrorHelpers<'a> for ContextualParseError<'a> { _, CssParseError::Custom(SelectorParseError::Custom( StyleParseError::UnexpectedTokenWithinNamespace(_)))) => (b"PEAtNSUnexpected\0", Action::Nothing), + ContextualParseError::InvalidRule( + _, CssParseError::Basic(BasicParseError::AtRuleInvalid(_))) | + ContextualParseError::InvalidRule( + _, CssParseError::Custom(SelectorParseError::Custom( + StyleParseError::UnsupportedAtRule(_)))) => + (b"PEUnknownAtRule\0", Action::Nothing), ContextualParseError::InvalidRule(_, ref err) => { let prefix = match *err { CssParseError::Custom(SelectorParseError::UnexpectedTokenInAttributeSelector(_)) =>