1213076 - CSSParserImpl::ParseVariant sometimes consumes non-whitespace input and returns false

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also see bug 1199610 and related bugs linked from there.

Comment 3

[Cameron McCormack (:heycam)]

(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.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 5

[Cameron McCormack (:heycam)]

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

Comment 6

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bda827d458eb

Comment 7

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff0c1ffc547f

Comment 8

[Cameron McCormack (:heycam)]

Attachment: patch

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 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 10

[Cameron McCormack (:heycam)]

(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.

Comment 11

[Tim Nguyen :ntim]

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