1099448 - properties parsed with CSSParserImpl::ParseBoxProperties incorrectly accept values with errors inside functions

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Brian Grinstead [:bgrins]]

I was trying to set a shorthand margin property using a css variable and bumped into something that looks like a bug with variables used inside of calc().  Test case here: http://jsbin.com/livafoxama/2/edit?html,css,output

  <div class="foo"> foo </div>
  <div class="bar"> bar </div>
  <style>
  :root {
    --pad: 40px;
  }

  .foo {
    padding: 0 calc(0px + var(--pad));
  }

  .bar {
    padding-left: calc(0px + var(--pad));
    padding-right: calc(0px + var(--pad));
  }
  </style>

Notice that the bar element is padded on the sides, but the foo element isn't

Comment 1

[Brian Grinstead [:bgrins]]

Cam, am I missing something or is this an issue with calc and/or css variables?

Comment 2

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

Attachment: testcase

(without all the jsbin gunk)

Interestingly, it works without the "0 " at the beginning of the padding value.

Comment 3

[Cameron McCormack (:heycam)]

An issue with CSS variables.  Given it works without the "0 ", I wonder if we're accidentally parsing the stored token stream as one of the component longhand properties, rather than parsing it as the shorthand and then getting the corresponding longhand property that was set.

Comment 4

[Cameron McCormack (:heycam)]

Attachment: test without variables

Actually, turns out there is an underlying problem with parsing shorthand properties like 'padding' that use ParseBoxProperties.  Here's a test that doesn't use variables.  Notice that padding is applied even though the property should have failed to parse.

ParseBoxProperties calls ParseSingleValueProperty which will end up calling ParseVariant for each of the four non-negative values it expects.  ParseVariant, when encountering a "calc(" with a syntax error inside it, will SkipUtil(')') and return false.  But ParseBoxProperties just uses the false return value of ParseSingleValueProperty to break the NS_FOR_CSS_SIDES loop, and seeing that we have one valid value, will return true.  Then, back up in ParseProperty (which called ParsePropertyByFunction for 'padding') it calls ExpectEndProperty, which returns true because we're now at the ';'.

Comment 5

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

I guess ParseBoxProperties needs to distinguish between ParseSingleValueProperty consuming something and failing versus consuming nothing and failing.

(I guess prior to calc() there wasn't a way for it to consume something and fail for the properties that ParseBoxProperties uses, so this is a regression from implementing calc().)

Comment 6

[Cameron McCormack (:heycam)]

Looks like there was:

  <div style="border: 8px solid green; border-color: red rgb(nonsense)">hello</div>

shows a red border.

Comment 7

[Cameron McCormack (:heycam)]

This also applies to ParseGroupedBoxProperty but none of the properties parsed with that accept calc() values at the moment.

Comment 8

[Cameron McCormack (:heycam)]

Attachment: patch

I fixed ParseGroupedBoxProperty too and tested locally by allowing calc() values in border-image-width and border-image-outset.  I put invalid values using calc() in property_database.js for when we end up supporting calc() in these properties.

Comment 9

[Cameron McCormack (:heycam)]

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

Comment 10

[Cameron McCormack (:heycam)]

Attachment: patch v2

Update test expectations.

Comment 11

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

Comment on attachment 8523629 [details] [diff] [review]
patch v2

Instead of writing all this new code to see if we consumed tokens, why 
not just use GetNextTokenLocation both before and after the call to
ParseSingleValueProperty or ParseNonNegativeVariant and see if they're
different? 

Also, maybe add some rgb(nonsense) to the tests for properties that take 
colors, including both box properties and others?


Sorry again for the delay getting to this.

Comment 12

[Cameron McCormack (:heycam)]

Attachment: patch v3

(In reply to David Baron [:dbaron] ⏰UTC-7 (mostly busy, back June 29) from comment #11)
> Comment on attachment 8523629 [details] [diff] [review]
> patch v2
> 
> Instead of writing all this new code to see if we consumed tokens, why 
> not just use GetNextTokenLocation both before and after the call to
> ParseSingleValueProperty or ParseNonNegativeVariant and see if they're
> different? 

Yeah, that works.

Comment 13

[Cameron McCormack (:heycam)]

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

Comment 14

[Brian Grinstead [:bgrins]]

Attachment: border-image-variable.html

Just bumped into a similar problem in Bug 1049012, and I'm curious if this fix will cover it or if I should file a new bug?  See the attached testcase, bug basically vars don't seem to work in border-image properties.

Comment 15

[Tim Nguyen :ntim]

Seems more like a problem in linear-gradients rather than border-image.

Comment 16

[Cameron McCormack (:heycam)]

(In reply to Brian Grinstead [:bgrins] from comment #14)
> Just bumped into a similar problem in Bug 1049012, and I'm curious if this
> fix will cover it or if I should file a new bug?  See the attached testcase,
> bug basically vars don't seem to work in border-image properties.

No it doesn't seem to help with this.  Please do file a new bug.

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8627962 [details]
border-image-variable.html

(In reply to Cameron McCormack (:heycam) from comment #16)
> (In reply to Brian Grinstead [:bgrins] from comment #14)
> > Just bumped into a similar problem in Bug 1049012, and I'm curious if this
> > fix will cover it or if I should file a new bug?  See the attached testcase,
> > bug basically vars don't seem to work in border-image properties.
> 
> No it doesn't seem to help with this.  Please do file a new bug.

Alright, filed Bug 1179078

Comment 18

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

Comment on attachment 8625597 [details] [diff] [review]
patch v3

I'd prefer if CSSParserImpl::ParseBoxPropertyVariant set aConsumedTokens
to true when returning true, rather than setting aConsumedTokens to false
in that case.  (It shouldn't need to check the token location.)

CSSParserImpl::ParseBoxProperty should also assert that
!nsCSSProps::PropHasFlags(aPropID, CSS_PROPERTY_VALUE_PARSER_FUNCTION)

r=dbaron with that

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da921ebcc14f

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/da921ebcc14f