In most cases, CSSParserImpl::ParseVariant only consumes non-whitespace tokens when it returns true. However, this doesn't always hold for functional syntax, since functions consist of multiple tokens. This leads to errors like bug 1179078 (which isn't the first one). We should probably either fix this in ParseVariant generally, or have better error reporting from ParseVariant so that it's clearer that callers need to handle such cases. (We could potentially split the functional mask items into a separate function that has additional parameters.)

Also see bug 1199610 and related bugs linked from there.

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #0) > We should probably either fix this in ParseVariant generally, or have better > error reporting from ParseVariant so that it's clearer that callers need to > handle such cases. (We could potentially split the functional mask items > into a separate function that has additional parameters.) Either of those approaches sound OK to me. I like the idea of not burdening callers that expect only single-token variants with needing to handle the "failed and consumed tokens" case, even though we'd need one for each of ParseVariant, ParseVariantWithRestrictions, ParseNonNegativeVariant and ParseOneOrLargerVariant.

That said, if the state restoration is cheap enough (which I think it ought to be -- though not sure if it is), we may as well just use it in all the cases that need it.

FWIW saving state in total involves copying an nsAutoString, a float, 8 (u)int32_ts, an enum, and three bools.

I decided to report back from relevant functions whether a parse error consumed tokens or not, rather than save/restore scanner state. I added helpers for the various Parse*Variant methods that work only for single-token values, to avoid cluttering all callers to those methods with an "== CSSParseResult::Ok" check. In ParseSingleValueProperty I do the "save line/col and compare after" thing to avoid having to adjust all ~20 property-specific parsing functions it calls. https://treeherder.mozilla.org/#/jobs?repo=try&revision=df02fa430376

Comment on attachment 8674066 [details] [diff] [review] patch Is there a way to make CSSParseResult converting to a boolean be an error? (Maybe a private/deleted operator bool?) If so, that would be nice. (Also the reverse? Maybe harder?) Or is it already an error because it's an enum class? AssertNextTokenAt is a little bit dangerous, since it does do a GetToken() / UngetToken() pair in DEBUG only, which is a little bit dangerous. I guess it's ok, but it could at least use a comment. Tests: Could you add the new invalid_values items you added for animation to transition as well? It might be nice to add a test for a valid linear-gradient position with calc(), actually. I don't think your new transform-origin test catches the case that used to fail (though it's worth keeping). You should add "0px 0px calc(0px + rubbish)", though may as well keep the current one. >+ "-moz-element(#a rubbish), 10, 50%, 30%, 0) transparent", Could you make this look a little closer to a valid value for the property? Same problem for your box-shadow test as for transform-origin; the problem was a bad calc in the third or fourth position, your test has it in second. Could you repeat that test with an extra "3px" and an extra "3px 3px" at the start? (text-shadow is ok, though.) The content garbage test needs an additional \"\" to make a non-empty value with the function dropped, I think. Some other tests that it's probably worth adding: to exercise ParseChoice: border-left shorthand with a bad calc() column shorthand with a bad calc() text-decoration shorthand with a bad rgb() bad calc() in background-size bad calc() in circle or ellipse function bad cubic-bezier() in transition/animation shorthand (with another value) bad rgb() in optional shadow color Maybe file a followup on converting ParseBoxPropertyVariant to CSSParseResult? r=dbaron with that

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #9) > Is there a way to make CSSParseResult converting to a boolean be an > error? (Maybe a private/deleted operator bool?) If so, that would > be nice. (Also the reverse? Maybe harder?) Or is it already an error > because it's an enum class? Yes, an enum class doesn't convert to bool, and the reverse shouldn't work either. > AssertNextTokenAt is a little bit dangerous, since it does do a > GetToken() / UngetToken() pair in DEBUG only, which is a little bit > dangerous. I guess it's ok, but it could at least use a comment. Fair point. > I don't think your new transform-origin test catches the case that used > to fail (though it's worth keeping). You should add > "0px 0px calc(0px + rubbish)", though may as well keep the current one. The test I added did fail (inside ParseBoxPositionValues), but we should test failure of the third argument too. > >+ "-moz-element(#a rubbish), 10, 50%, 30%, 0) transparent", > > Could you make this look a little closer to a valid value for the > property? Er, that is a copypaste error leaving too much from the previous line. > Same problem for your box-shadow test as for transform-origin; the > problem was a bad calc in the third or fourth position, your test has it > in second. Could you repeat that test with an extra "3px" and an extra > "3px 3px" at the start? (text-shadow is ok, though.) I'll do one for the shadow colour argument, too. > Some other tests that it's probably worth adding: > > to exercise ParseChoice: ... > bad calc() in background-size Especially given that was the problem the test reporter had... > bad cubic-bezier() in transition/animation shorthand (with another value) This one I already have. > Maybe file a followup on converting ParseBoxPropertyVariant to > CSSParseResult? Filed bug 1215424.

This incorrectly landed with bug 1199610 as bug number in the commit message. Changesets: - mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/6a1fd211d8ae - mozilla-central: https://hg.mozilla.org/mozilla-central/rev/6a1fd211d8ae