Closed Bug 1077388 Opened 5 years ago Closed 5 years ago

Fix parse and computation of polygon()

Categories

(Core :: CSS Parsing and Computation, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: krit, Assigned: krit)

References

Details

Attachments

(1 file, 5 obsolete files)

There are two small issues. One in parsing and one in computation.

The parsing issue allows clip-path to parse two reference boxes or two shapes. Both is not allowed.

For computation, a comma is added to polygon() even if the fill rule is nonzero.
Blocks: 1072894
Attached patch clip-path-polygon-fix.patch (obsolete) — Splinter Review
Attachment #8499535 - Flags: review?(dbaron)
Comment on attachment 8499535 [details] [diff] [review]
clip-path-polygon-fix.patch

OK, so the spec is:
http://dev.w3.org/fxtf/css-masking-1/#the-clip-path
http://dev.w3.org/csswg/css-shapes/#basic-shape-functions
(it might be nice to mention that, given that I don't see it in bug 1072894 either.)


>-    if (!shape && !box && !eof) {
>+    if ((!shape && !box) || !eof) {
>       REPORT_UNEXPECTED_EOF(PEClipPathEOF);
>       return false;
>     }

Neither the old nor the new code here makes sense.  There's no reason to test for EOF at all in this function.

Second, the code looks like it accepts two shapes or two boxes just fine, which it shouldn't.

Third, it fails to distinguish between ParseBasicShape returning false because it consumed nothing and returning false because it consumed an invalid polygon function.  It needs to distinguish.



Beyond that, a few questions:
 * is this code fixing test failures if you run layout/style mochitests with the pref enabled?  If not, can you add tests for what it's fixing?  (In general, it helps if you answer this when requesting review.)

>-      if (i > 0 || fillRule) {
>+      if (i > 0 || fillRule == NS_STYLE_FILL_RULE_EVENODD) {

This is ok, although it might be clearer to keep a "start" boolean instead.

>diff --git a/layout/style/test/property_database.js b/layout/style/test/property_database.js


Is this change fixing test failures too?
Attachment #8499535 - Flags: review?(dbaron) → review-
(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #2)
> Comment on attachment 8499535 [details] [diff] [review]
> clip-path-polygon-fix.patch
> 
> OK, so the spec is:
> http://dev.w3.org/fxtf/css-masking-1/#the-clip-path
> http://dev.w3.org/csswg/css-shapes/#basic-shape-functions
> (it might be nice to mention that, given that I don't see it in bug 1072894
> either.)

Yes, will add it to the master bug. Thanks!

> 
> 
> >-    if (!shape && !box && !eof) {
> >+    if ((!shape && !box) || !eof) {
> >       REPORT_UNEXPECTED_EOF(PEClipPathEOF);
> >       return false;
> >     }
> 
> Neither the old nor the new code here makes sense.  There's no reason to
> test for EOF at all in this function.

This was a request of Cameron in his review of the original patch: https://bugzilla.mozilla.org/show_bug.cgi?id=1072894#c2

> 
> Second, the code looks like it accepts two shapes or two boxes just fine,
> which it shouldn't.

It does work, since we never enter ParseBasicShape if if was successfully parsed before. (That's why there is the !shape check.)

There are multiple tests in the test file that check this behavior. They failed before, since the EOF check wasn't correct. This is fixed with this patch.

> 
> Third, it fails to distinguish between ParseBasicShape returning false
> because it consumed nothing and returning false because it consumed an
> invalid polygon function.  It needs to distinguish.

We do not have the distinguish in ParseClipPath, but we report failures in the individual parse methods for each shape function.

However, one report was missing. I did not report if the user specified a function that is not a basic shape. I'll add this to the next patch.

> 
> 
> 
> Beyond that, a few questions:
>  * is this code fixing test failures if you run layout/style mochitests with
> the pref enabled?  If not, can you add tests for what it's fixing?  (In
> general, it helps if you answer this when requesting review.)
> 
> >-      if (i > 0 || fillRule) {
> >+      if (i > 0 || fillRule == NS_STYLE_FILL_RULE_EVENODD) {
> 
> This is ok, although it might be clearer to keep a "start" boolean instead.

Indeed. I added a boolean before the loop.

> 
> >diff --git a/layout/style/test/property_database.js b/layout/style/test/property_database.js
> 
> 
> Is this change fixing test failures too?

8 tests failed before this patch:

      "border-box content-box",
      "polygon(0 0) polygon(0 0)",

    both fixed with the changes to the parser in this patch.

      "polygon(30% 30%",

    fixed by adding it to the unbalanced checks.


    and various polygon(x y) checks where the computed style was polygon(, x y) because of the missing explicit evenodd check.

So all changes are covered by existing tests.
Attached patch clip-path-polygon-fix.patch (obsolete) — Splinter Review
Attachment #8499535 - Attachment is obsolete: true
Attachment #8500847 - Flags: review?(dbaron)
(In reply to Dirk Schulze from comment #3)
> > >-    if (!shape && !box && !eof) {
> > >+    if ((!shape && !box) || !eof) {
> > >       REPORT_UNEXPECTED_EOF(PEClipPathEOF);
> > >       return false;
> > >     }
> > 
> > Neither the old nor the new code here makes sense.  There's no reason to
> > test for EOF at all in this function.
> 
> This was a request of Cameron in his review of the original patch:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1072894#c2

I see "We don't need to check for EOF here for correct parsing" in that comment.

Really, all error reporting *inside* of property value parsers is optional, and only needed for errors that are particularly interesting to explain.

I stand by both conditions above being wrong (how could the new one be a condition for reporting an unexpected EOF when it doesn't require EOF), and I think you shouldn't test for EOF at all in this function.

> > Second, the code looks like it accepts two shapes or two boxes just fine,
> > which it shouldn't.
> 
> It does work, since we never enter ParseBasicShape if if was successfully
> parsed before. (That's why there is the !shape check.)

The code is:
      if (ParseBasicShape(cur->mValue) && !shape) {
which tests !shape *after* calling ParseBasicShape.

> There are multiple tests in the test file that check this behavior. They
> failed before, since the EOF check wasn't correct. This is fixed with this
> patch.

I see two:
      "border-box content-box",
      "polygon(0 0) polygon(0 0)",

Could you explain what codepah rejects these values?

> > Third, it fails to distinguish between ParseBasicShape returning false
> > because it consumed nothing and returning false because it consumed an
> > invalid polygon function.  It needs to distinguish.
> 
> We do not have the distinguish in ParseClipPath, but we report failures in
> the individual parse methods for each shape function.

The problem isn't *reporting* failures, which is for the Web console.  The problem is whether the parse actually succeeds or fails.

As far as I can tell, all of your invalid_values that are invalid because of what is inside of polygon() will be treated as valid if you stick "content-box" before or after them.

> However, one report was missing. I did not report if the user specified a
> function that is not a basic shape. I'll add this to the next patch.

Detailed error reporting in property value parsing is not necessary.

> 8 tests failed before this patch:
> 
>       "border-box content-box",
>       "polygon(0 0) polygon(0 0)",
> 
>     both fixed with the changes to the parser in this patch.

I suppose I need to add more tests to test/test_property_syntax_errors.html that test that properties are rejected even when there's something else after them.  (We currently only test syntax errors in isolation, i.e., followed by EOF.)  And probably similar for test_value_storage.html.

This change in particular:
>-    if (!shape && !box && !eof) {
>+    if ((!shape && !box) || !eof) {
makes us treat as an error any value of the property that is not terminated by end of file, which means that any value that's actually in a style sheet (as opposed to set through the CSSOM as the only value) will be rejected.
Another interesting test for the bizarre logic in that function:  I believe it will accept:

  polygon(0 0) polygon(0 0) content-box

(The first function gets parsed with i=0, and the second function and the keyword get parsed with i=1.)


Also, one of the tests:
>      "polygon(0 0) conten-box",
has a typo in it that should cause it to fail (no t in content).  That should go in the invalid_values line, and the correct thing in the other_values line.  (I don't actually see how that's passing today since the ExpectEndProperty call in CSSParserImpl::ParseProperty ought to fail.)
Attachment #8500847 - Flags: review?(dbaron) → review-
(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #5)
> (In reply to Dirk Schulze from comment #3)
> > > >-    if (!shape && !box && !eof) {
> > > >+    if ((!shape && !box) || !eof) {
> > > >       REPORT_UNEXPECTED_EOF(PEClipPathEOF);
> > > >       return false;
> > > >     }
> > > 
> > > Neither the old nor the new code here makes sense.  There's no reason to
> > > test for EOF at all in this function.
> > 
> > This was a request of Cameron in his review of the original patch:
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1072894#c2
> 
> I see "We don't need to check for EOF here for correct parsing" in that
> comment.
> 
> Really, all error reporting *inside* of property value parsers is optional,
> and only needed for errors that are particularly interesting to explain.
> 
> I stand by both conditions above being wrong (how could the new one be a
> condition for reporting an unexpected EOF when it doesn't require EOF), and
> I think you shouldn't test for EOF at all in this function.
> 
> > > Second, the code looks like it accepts two shapes or two boxes just fine,
> > > which it shouldn't.
> > 
> > It does work, since we never enter ParseBasicShape if if was successfully
> > parsed before. (That's why there is the !shape check.)
> 
> The code is:
>       if (ParseBasicShape(cur->mValue) && !shape) {
> which tests !shape *after* calling ParseBasicShape.
> 

That is true. We call ParseBasicShape twice. And you are also right that 2 polygons and a reference box causes the parser to pass. Changing the order of the !shape check and ParseBasicShape fixes this issue. Ditto for the box of course.

I am fine with not reporting EOF, however we still want to report that the box is incorrect or there isn't any correct argument at all. The EOF text currently has "<basic-shape> or reference box". This is exactly Cameron's suggested change after the quote you pasted.

> Detailed error reporting in property value parsing is not necessary.

I removed the report for invalid shape function again.

> I suppose I need to add more tests to test/test_property_syntax_errors.html that test that properties are rejected even when there's something else after them.

I added a lot of more tests with different scenarios that cover your suggested tests and a lot more.
Attached patch clip-path-polygon-fix.patch (obsolete) — Splinter Review
Attachment #8500847 - Attachment is obsolete: true
Attachment #8500877 - Flags: review?(dbaron)
Comment on attachment 8500877 [details] [diff] [review]
clip-path-polygon-fix.patch

>-    if (!shape && !box && !eof) {
>+    if ((!shape && !box) || !eof) {

This will still reject any value that's coming from an actual style sheet rather than a DOM set.

You should not be testing for EOF at all in this function.
Attachment #8500877 - Flags: review?(dbaron) → review-
(And, furthermore, it does not make sense to make an error report of "Unexpected EOF" in a block conditioned on !eof.)
(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #9)
> Comment on attachment 8500877 [details] [diff] [review]
> clip-path-polygon-fix.patch
> 
> >-    if (!shape && !box && !eof) {
> >+    if ((!shape && !box) || !eof) {
> 
> This will still reject any value that's coming from an actual style sheet
> rather than a DOM set.
> 
> You should not be testing for EOF at all in this function.

So REPORT_UNEXPECTED_EOF as the way to report an error would cause problems with stylesheets that have more data following after the current property value?

REPORT_UNEXPECTED_EOF seems to be used a lot in different places like ParseSingleFilter, ParseValueWithVariables or parsing var() and 61 other places. When is it save to use? Why doesn't it cause problems anywhere else? Or better, what is it good for?
Flags: needinfo?(dbaron)
REPORT_UNEXPECTED_EOF is to report an unexpected end-of-file (EOF) when looking for something else.  REPORT_UNEXPECTED_TOKEN is to report all other unexpected tokens (i.e., anything other than the token you were looking for, but not EOF).  The other REPORT_UNEXPECTED_* variants are for more unusual cases.
Flags: needinfo?(dbaron)
Attached patch clip-path-polygon-fix.patch (obsolete) — Splinter Review
Removed the EOF report and replaced it with REPORT_UNEXPECTED_TOKEN.
Attachment #8500877 - Attachment is obsolete: true
Attachment #8501245 - Flags: review?(dbaron)
Comment on attachment 8501245 [details] [diff] [review]
clip-path-polygon-fix.patch

>-    if (!shape && !box && !eof) {
>+    if ((!shape && !box) || !eof) {

This is still wrong, as I explained in comment 9.
Attachment #8501245 - Flags: review?(dbaron) → review-
(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #14)
> Comment on attachment 8501245 [details] [diff] [review]
> clip-path-polygon-fix.patch
> 
> >-    if (!shape && !box && !eof) {
> >+    if ((!shape && !box) || !eof) {
> 
> This is still wrong, as I explained in comment 9.

I think I do not check for EOF anymore but indeed did not change the name. Could you check the functionality please before I upload another patch?

The variable 'eof' should probably be renamed to 'parsed' or 'parsingPassed'.
Flags: needinfo?(dbaron)
A little more detail (sorry, in all day meetings yesterday, today, and tomorrow, so comments a little disjointed):

In general, parsing functions should consider what's valid rather than what's invalid.  This generally makes them more robust under future extensions.  For example, when we added @supports, property value parsing functions were suddenly used in a context where the value could be terminated by ) in addition to ;, }, or EOF.  Value parsing functions that were poorly written and looked at what they thought would be after them needed modification; value parsing functions that considered only what they allowed were fine.  (Callers of GetToken() still need to check for EOF, though, but there's no need for this function to call GetToken() at all; it's doing so solely to check for EOF.)

And, again, as I said in comment 9 and before, the change you're making breaks uses of this property that aren't terminated by EOF (i.e., those that are terminated by }, ;, or )).
(In reply to Dirk Schulze from comment #15)
> I think I do not check for EOF anymore but indeed did not change the name.
> Could you check the functionality please before I upload another patch?
> 
> The variable 'eof' should probably be renamed to 'parsed' or 'parsingPassed'.

No, you're still checking for EOF; GetToken failing happens only on EOF.  But you have no reason to call GetToken in this function at all, except to check for EOF.  (See also comment 16, which I wrote before seeing comment 15.)
Flags: needinfo?(dbaron)
Attached patch clip-path-polygon-fix.patch (obsolete) — Splinter Review
Thanks a lot for your explanation. This makes a lot of sense. After the comment I looked more into how other parsing code handles this kind of situation.

I uploaded a new patch. This is much closer to my original patch on bug 1072894.
Instead of checking for GetToken(), I take the commonly used function CheckEndProperty(). An example of a call of this function is in ParseFilter() which checks if we are at the end of the property before trying to parse the next filter function.
If we ever get into the 'else' case, this is because we either fail in ParseBasicShape and ParseEnumeration, or we entered it before.
Attachment #8501245 - Attachment is obsolete: true
Attachment #8501824 - Flags: review?(dbaron)
Comment on attachment 8501824 [details] [diff] [review]
clip-path-polygon-fix.patch

>+PEExpectedBasicShapeFunction=Expected <basic-shape> function but found '%1$S'.

Don't add an error string that you don't use.

>diff --git a/layout/style/nsCSSParser.cpp b/layout/style/nsCSSParser.cpp
>     bool shape = false, box = false;
>     nsCSSValueList* cur = value.SetListValue();
>-    bool eof = false;
>     for (int i = 0; i < 2; ++i) {
>-      if (ParseBasicShape(cur->mValue) && !shape) {
>+      if (!shape &&ParseBasicShape(cur->mValue)) {

space after &&

>         shape = true;
>-      } else if (ParseEnum(cur->mValue, nsCSSProps::kClipShapeSizingKTable) &&
>-                 !box) {
>+      } else if (!box &&
>+                 ParseEnum(cur->mValue, nsCSSProps::kClipShapeSizingKTable)) {
>         box = true;
>       } else {
>-        break;
>-      }
>-      if (!GetToken(true)) {
>-        eof = true;
>-        break;
>-      }
>-      UngetToken();
>+        REPORT_UNEXPECTED_TOKEN(PEExpectedBasicShapeOrBox);      
>+        return false;
>+      }
>+      if (CheckEndProperty()) {
>+        break;
>+      }
>       cur->mNext = new nsCSSValueList;
>       cur = cur->mNext;
>     }
>-    if (!shape && !box && !eof) {
>-      REPORT_UNEXPECTED_EOF(PEClipPathEOF);
>-      return false;
>-    }

So this works, but the code is still unnecessarily complicated.  In particular, I think you can:
 * convert the "return false" inside the loop back to a break
 * remove the CheckEndProperty() test
 * restore the if (!shape && !box) { return false } (maybe or maybe not with an error message)

(I'd actually like to remove the CheckEndProperty() function, though we're not quite there yet. Thus I'd really like to avoid adding more uses of it.)


>     if (fillRule == NS_STYLE_FILL_RULE_EVENODD) {
>       shapeFunctionString.AppendLiteral("evenodd");
>     }
>+    bool hasEvenOdd = fillRule == NS_STYLE_FILL_RULE_EVENODD;

At least move the hasEvenOdd up above this if, and then makethe if test hasEvenOdd, so that it's clearer how the conditions are related.


Otherwise I think this looks good, but I'd like to see one more revision.
Attachment #8501824 - Flags: review?(dbaron) → review-
(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #19)
> Comment on attachment 8501824 [details] [diff] [review]
> clip-path-polygon-fix.patch
> 
> >+PEExpectedBasicShapeFunction=Expected <basic-shape> function but found '%1$S'.
> 
> Don't add an error string that you don't use.

Done. Thanks.

> 
> >diff --git a/layout/style/nsCSSParser.cpp b/layout/style/nsCSSParser.cpp
> >     bool shape = false, box = false;
> >     nsCSSValueList* cur = value.SetListValue();
> >-    bool eof = false;
> >     for (int i = 0; i < 2; ++i) {
> >-      if (ParseBasicShape(cur->mValue) && !shape) {
> >+      if (!shape &&ParseBasicShape(cur->mValue)) {
> 
> space after &&

Fixed.

> 
> >         shape = true;
> >-      } else if (ParseEnum(cur->mValue, nsCSSProps::kClipShapeSizingKTable) &&
> >-                 !box) {
> >+      } else if (!box &&
> >+                 ParseEnum(cur->mValue, nsCSSProps::kClipShapeSizingKTable)) {
> >         box = true;
> >       } else {
> >-        break;
> >-      }
> >-      if (!GetToken(true)) {
> >-        eof = true;
> >-        break;
> >-      }
> >-      UngetToken();
> >+        REPORT_UNEXPECTED_TOKEN(PEExpectedBasicShapeOrBox);      
> >+        return false;
> >+      }
> >+      if (CheckEndProperty()) {
> >+        break;
> >+      }
> >       cur->mNext = new nsCSSValueList;
> >       cur = cur->mNext;
> >     }
> >-    if (!shape && !box && !eof) {
> >-      REPORT_UNEXPECTED_EOF(PEClipPathEOF);
> >-      return false;
> >-    }
> 
> So this works, but the code is still unnecessarily complicated.  In
> particular, I think you can:
>  * convert the "return false" inside the loop back to a break
>  * remove the CheckEndProperty() test
>  * restore the if (!shape && !box) { return false } (maybe or maybe not with
> an error message)
> 
> (I'd actually like to remove the CheckEndProperty() function, though we're
> not quite there yet. Thus I'd really like to avoid adding more uses of it.)

clip-path has the following syntax: <basic-shape> || <reference-box>

With one valid value and the change above, I create a new empty list item at the end of the first loop. This gives wrong results for inline style. Of course I can create a temp variable of type nsCSSValue, check at the end of the look if i > 0 and set currentListItem->mValue to the temp variable.

The problem that I have however, is what happens if the first argument is correct but the second is not, like in "border-box polygon()". The first argument is true, we set box to true and jump into the next loop. When ParseBasicShape returns false, we would check if there was a box parsed before. It is, so we go to the "else" part of the check and break. Now check if (!shape && !box). At least box is true so we should pass parsing, no? Of course we should fail here.

However, in the tests I see the following results

border-box polygon(0 0) - passes, parser correct
polygon(0 0) nonsense - fails, parser correct
border-box test(0 0) - fails, parser correct
border-box padding-box - fails, parser correct
test(0 0) border-box - fails, parser correct
border-box polygon() - passes with result "border-box", parser incorrect
polygon() padding-box - passes with result "padding-box", parser incorrect

So once we enter ParsePolygonFunction and the polygon function has invalid input, we get a wrong parsing result. Note that ParsePolygonFunction still correctly returns false. I am confused why the parser does not fail in the other scenarios but does in the last two cases.

> 
> 
> >     if (fillRule == NS_STYLE_FILL_RULE_EVENODD) {
> >       shapeFunctionString.AppendLiteral("evenodd");
> >     }
> >+    bool hasEvenOdd = fillRule == NS_STYLE_FILL_RULE_EVENODD;
> 
> At least move the hasEvenOdd up above this if, and then makethe if test
> hasEvenOdd, so that it's clearer how the conditions are related.
> 

Fixed.

> 
> Otherwise I think this looks good, but I'd like to see one more revision.
(In reply to Dirk Schulze from comment #20)
> With one valid value and the change above, I create a new empty list item at
> the end of the first loop. This gives wrong results for inline style. Of
> course I can create a temp variable of type nsCSSValue, check at the end of
> the look if i > 0 and set currentListItem->mValue to the temp variable.

Ah, indeed, to do it this way you'd need to append later.

> The problem that I have however, is what happens if the first argument is
> correct but the second is not, like in "border-box polygon()". The first
> argument is true, we set box to true and jump into the next loop. When
> ParseBasicShape returns false, we would check if there was a box parsed
> before. It is, so we go to the "else" part of the check and break. Now check
> if (!shape && !box). At least box is true so we should pass parsing, no? Of
> course we should fail here.

Oh, this is the problem that I pointed out in comment 2 and comment 6 and then forgot about.  Given the current structure of the code, ParseBasicShape needs to distinguish between consuming nothing and consuming something and then failing.

However, there's a better way, which is the way we normally handle constructs like this:

You should get rid of the loop construct and do things more like other code that parses properties of that syntax.  That is, structure the code more like CSSParserImpl::ParseBorderImageSlice:  call ParseEnum, call ParseBasicShape, and then if ParseEnum didn't succeed the first time try calling ParseEnum again.  This is the way we generally handle this sort of thing, and it avoids this problem, and avoids needing to call CheckEndProperty.
I took ParseBorderImageSlice as example and added an argument aConsumedTokens to ParseBasicShape. I added more tests to test the argument. I replaced the loop with the suggested Enum, BasicShape, Enum testing.
Attachment #8501824 - Attachment is obsolete: true
Attachment #8503046 - Flags: review?(dbaron)
Comment on attachment 8503046 [details] [diff] [review]
clip-path-polygon-fix.patch

>+    // We need to preserve the specified order of arguments for inline style.

I'm not sure this is actually true; there are many similar properties where we don't preserve order.

r=dbaron, and sorry for all the iterations here
Attachment #8503046 - Flags: review?(dbaron) → review+
Thanks a lot for the reviews!

The try bots on Linux Opt already pass all tests: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d5aaa9976c9b
Keywords: checkin-needed
Blocks: 1074522
Assignee: nobody → krit
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9fa1a711dc7

Please make sure your future patches are generated per the guidelines below and that all commit information is present when requesting checkin.
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c9fa1a711dc7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.