Closed Bug 941300 Opened 6 years ago Closed 6 years ago

Bad circle radial-gradient parsed as an ellipse

Categories

(Core :: CSS Parsing and Computation, defect)

26 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: stearns, Assigned: emk)

References

Details

Attachments

(2 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131104182142

Steps to reproduce:

Give a circle radial-gradient two radii:

background-image: radial-gradient(circle 175px 20px, black, white);


Actual results:

Firefox will evaluate it as an ellipse. It appears that the circle/ellipse parameter of the function is dropped.


Expected results:

Other browsers (Chrome, Opera 12) result in an invalid declaration.
Component: Untriaged → Layout
Product: Firefox → Core
GetRadialShape() and GetRadiusX() shares the storage, so RadialShape will be broken after parsing RadiusX.
https://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSValue.h?rev=22498ff939e0#1082
We will have to save the shape value before parsing RadiusX.
Blocks: 752187
Status: UNCONFIRMED → NEW
Component: Layout → CSS Parsing and Computation
Ever confirmed: true
OS: Mac OS X → All
Hardware: x86 → All
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #8336541 - Flags: review?(dbaron)
(In reply to Masatoshi Kimura [:emk] from comment #1)
> GetRadialShape() and GetRadiusX() shares the storage, so RadialShape will be
> broken after parsing RadiusX.

Can we enforce this storage-usage more strictly? (i.e. once we're treating the storage as "GetRadiusX", fail an assertion if GetRadialShape is ever called, and vice versa)

It looks like perhaps we could condition this on mIsExplicitSize -- I think (?) that's what tells us what the storage is being used for? (not sure)

(If we set mIsExplicitSize a little bit earlier -- maybe right after your patch's GetRadialShape call -- then I think we might be able to add strict checks on that bool to all of these getters.)
We can't determine whether we should set mIsExplicitSize or not until we get the haveSize value. We have to parse RadiusX to get the haveSize value (that is, before setting mIsExplicitSize).
We need to resolve this chicken-and-egg problem to add an assertion to GetRadiusX().
It would be easier to add an assertion to GetRadialShape().
(In reply to Masatoshi Kimura [:emk] from comment #4)
> We can't determine whether we should set mIsExplicitSize or not until we get
> the haveSize value. We have to parse RadiusX to get the haveSize value (that
> is, before setting mIsExplicitSize).

Hmm... I see what you mean, but I think it's actually trickier than that (and it brings up another source of trouble here).

The trouble is that even when ParseNonNegativeVariant fails, it has likely stomped on its outparam (GetRadiusX, in this case).  (See the first 3 "return false" statements in ParseNonNegativeVariant, for example [1] -- they return false after determining that they've *already* put something undesirable into the outparam.)

So even when ParseNonNegativeVariant(cssGradient->GetRadiusX(), ...) returns false, it may have corrupted our storage value already, and we shouldn't trust it to be a valid Shape anymore.

Perhaps we should really be calling ParseNonNegativeVariant() with a temporary nsCSSValue, and then *only* if it succeeds, we can set mIsExplicitSize and copy that temporary value into GetRadiusX(). (If we go this strategy, we could make all the getters strictly check mIsExplicitSize as I suggested in comment 3.)

[1] http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSParser.cpp?rev=f30dfa0f184d&mark=4995-4997,5001-5003,5006-5008#4979
(though maybe we don't care whether that particular storage value has been stomped on in that case, if e.g. the code below is going to put something else into it in that case anyway?)

If in fact we *don't* care that the storage is corrupt, we could still "optimistically" set cssGradient->mIsExplicitSize to true before we call ParseNonNegativeVariant (which would allow us to have strict assertions in all of these getters).

e.g.
  // Let's try parsing an explicit size (and tell our storage that
  // that's what we're using it for now):

  cssGradient->mIsExplicitSize = true;
  haveSize = ParseNonNegativeVariant(cssGradient->GetRadiusX(), ...);
  if (!haveSize) {
    // Oh well, I guess it's not an explicit size after all.
    // Note that ParseNonNegativeVariant may have put something
    // invalid into our storage, but it's OK because [explanation]
    cssGradient->mIsExplicitSize = false;
  } else {
    // [everything that you have in your existing
    //  'if (haveSize)' clause goes here]
 ]
(In reply to Daniel Holbert [:dholbert] from comment #5)
> The trouble is that even when ParseNonNegativeVariant fails, it has likely
> stomped on its outparam (GetRadiusX, in this case).  (See the first 3
> "return false" statements in ParseNonNegativeVariant, for example [1] --
> they return false after determining that they've *already* put something
> undesirable into the outparam.)

It doesn't matter because the declaration will be dropped anyway if we encounter a negative value here. So we can take your comment #6 approach.
Added assertions.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=2e57020dc34c
Attachment #8336541 - Attachment is obsolete: true
Attachment #8336541 - Flags: review?(dbaron)
Attachment #8337251 - Flags: review?(dbaron)
(In reply to Masatoshi Kimura [:emk] from comment #7)
> It doesn't matter because the declaration will be dropped anyway if we
> encounter a negative value here. So we can take your comment #6 approach.

I don't immediately see where that happens (not knowing this function super-well), but I'll take your word for it.

Still, why not just bail immediately in that case? That seems safer than depending on logic later in the function to react to some other bit of state and avoid touching our now-invalid |cssGradient| object.
(Also: by "if we encounter a negative value", do you mean we end up reading & reacting to the value that ParseNonNegativeVariant() stuck into cssGradient->GetRadiusX(), in situations where ParseNonNegativeVariant failed? That seems undesirable... It seems like we shouldn't depend on ParseNonNegativeVariant to have done anything in particular with the outparam, in cases where it fails. Its false return value should tell us not to trust its outparam.)
(In reply to Daniel Holbert [:dholbert] from comment #9)
> Still, why not just bail immediately in that case?

Because ParseNonNegativeVariant() can not distinguish nagative values (which should fail) from non-numeric values (which should not fail).

(In reply to Daniel Holbert [:dholbert] from comment #10)
> (Also: by "if we encounter a negative value", do you mean we end up reading
> & reacting to the value that ParseNonNegativeVariant() stuck into
> cssGradient->GetRadiusX(), in situations where ParseNonNegativeVariant
> failed?

No, GetRadiusX() will never be called. Now |mIsExplicitSize = false;| will ensure that.
(In reply to Masatoshi Kimura [:emk] from comment #11)
> > (Also: by "if we encounter a negative value", do you mean we end up reading
> > & reacting to the value that ParseNonNegativeVariant() stuck into
> > cssGradient->GetRadiusX()
> 
> No, GetRadiusX() will never be called. Now |mIsExplicitSize = false;| will
> ensure that.

Sure -- but the (bogus) value could feasibly still be accessed via e.g. GetRadialShape() or something, though. (I initially misread comment 7 as implying that something like that might happen, and bail out if the value was out-of-bounds). But as long as we never read that value again, I guess it doesn't matter much.
Comment on attachment 8337251 [details] [diff] [review]
Make circle radial-gradient invalid if two radii values are given

>+      // Note that ParseNonNegativeVariant may have put something
>+      // invalid into our storage, but it's OK because the declaration
>+      // will be dropped anyway in that case.

I think this comment should be clearer.  I'd say:

       // Note that ParseNonNegativeVariant may have put something
       // invalid into our storage, but only in the case where it was
       // rejected only for being negative.  Since this means the token
       // was a length or a percentage, we know it's not valid syntax
       // (which must be a comma, the 'at' keyword, or a color), so we
       // know this value will be dropped.  This means it doesn't matter
       // that we have something invalid in our storage.

(It took me a while to understand why the comment was true, so I think
it needs to be clearer.  I'm not crazy about the code, either, but I
guess it's ok.)

Please add a second test with the <size> and <ending-shape> in the
opposite order, i.e., "radial-gradient(175px 20px circle, black, white)".

r=dbaron with that, and sorry for the delay getting to this
Attachment #8337251 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #13)
> Please add a second test with the <size> and <ending-shape> in the
> opposite order, i.e., "radial-gradient(175px 20px circle, black, white)".

Ugh, that test found another bug. |shapeValue| had not been read at all. I also added some more tests.
Attachment #8337251 - Attachment is obsolete: true
Attachment #8346496 - Flags: review?(dbaron)
Comment on attachment 8346496 [details] [diff] [review]
Make circle radial-gradient invalid if two radii values are given

>+        if (haveShape) {
>+            shape = shapeValue.GetIntValue();
>+        }

2-space indent rather than 4, please.


Also, maybe add a test that a single percentage is rejected, both with and without the circle keyword, e.g., by adding values to invalid_values like:

  radial-gradient(50%, red, blue)
  radial-gradient(circle 50%, red, blue)
  radial-gradient(50% circle, red, blue)

r=dbaron
Attachment #8346496 - Flags: review?(dbaron) → review+
https://hg.mozilla.org/mozilla-central/rev/8bb981d3522b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.