Closed Bug 1199610 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1213076

People

(Reporter: ntim, Assigned: heycam)

References

Details

Attachments

(2 files)

:heycam, can you please look at this ? Thanks.
Flags: needinfo?(cam)
(I'm not sure if the "0" instead of "0px" should work with calc(), but even fixing that doesn't make the testcase work.)
Attached file 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)
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);
Assignee: nobody → cam
Status: NEW → ASSIGNED
Flags: needinfo?(cam)
Summary: CSS variables don't work with calc() → background-size does not handle parse errors inside calc() expressions correctly
That other similar bug was bug 1099448.
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.
Summary: background-size does not handle parse errors inside calc() expressions correctly → various CSS parser methods incorrectly assume ParseVariant and friends consume no tokens on failure
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.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1213076
Duplicate of this bug: 1221877
You need to log in before you can comment on or make changes to this bug.