1199610 - various CSS parser methods incorrectly assume ParseVariant and friends consume no tokens on failure

Status: RESOLVED DUPLICATE of bug 1213076

(Core :: CSS Parsing and Computation, defect)

Description

[Tim Nguyen :ntim]

Testcase : http://jsfiddle.net/ntim/os1ev3ur/

Comment 1

[Tim Nguyen :ntim]

:heycam, can you please look at this ? Thanks.

Comment 2

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

Attachment: reporter's testcase

Comment 3

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

(I'm not sure if the "0" instead of "0px" should work with calc(), but even fixing that doesn't make the testcase work.)

Comment 4

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

Attachment: testcase with 0px

Steps to reproduce:
 1. load this testcase
 2. hover over the tabbar

Expected results:
 1. white bars between Foo | Bar | Baz are not the whole height of the red strip
 2. when hovering, white bars are now the whole height of the red strip

Actual results:
 1. shows expected results for (2)

Comment 5

[Cameron McCormack (:heycam)]

I think the issue is similar to one we've had before; when we're in the middle of parsing the calc() value, we encounter the "var(" token in the middle of ParseNonNegativeVariant, which triggers a SkipUntil(')').  The false return false of ParseNonNegativeVariant isn't treated as an error -- it's really assuming that we left the state of scanner before the value we attempted to parse.  We then arrive at the ';' token, which ExpectEndProperty up in ParseProperty takes to mean we completely parsed a successful property.

You can get the same problem without variables, with e.g.

  background-size: 10px calc(0px + rubbish);

Comment 6

[Cameron McCormack (:heycam)]

That other similar bug was bug 1099448.

Comment 7

[Cameron McCormack (:heycam)]

From code inspection (but without testing) it looks like there are other examples of this error:

CSSParserImpl::ParseGridTrackBreadth
CSSParserImpl::ParseColorStop
CSSParserImpl::ParseBoxCornerRadius
CSSParserImpl::ParseBoxCornerRadiiInternals
CSSParserImpl::ParseBoxPositionValues
CSSParserImpl::ParsePositionValue
CSSParserImpl::ParseBorderSpacing
CSSParserImpl::ParseTransformOrigin
CSSParserImpl::ParseShadowItem (for the optional radius/spread/color values)
CSSParserImpl::ParsePaint

I'm inclined to fix these by making the various ParseVariant methods be able to restore the input stream state if they encounter an error inside a calc(), rgb(), hsl(), etc., rather than copying the input position code that we added in bug 1099448 to all of the above methods.

Comment 8

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

Bug 1179078 was also similar, and when I was prompted to look at it a few days ago due to bugmail, I filed bug 1213076 on the general problem with roughly that idea, and also one alternative.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a1fd211d8ae

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6a1fd211d8ae

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6a1fd211d8ae