897094 - Mismatched parenthesis in some CSS functions do not prevent parsing of subsequent CSS properties

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Max Vujovic]

Attachment: Test

Please see attached test for an example. The test fails in FF nightly.

Affected properties include 'transform', 'font-variant-alternates', and 'filter' (when bug 895182 lands).

We need to call SkipUntil("(") in ParseFunctionInternals.

Comment 1

[Max Vujovic]

Attachment: Patch v1

This patch passes mochitests locally.

Comment 2

[Max Vujovic]

Comment on attachment 780690 [details] [diff] [review]
Patch v1

Requesting review from heycam.

Comment 3

[Cameron McCormack (:heycam)]

Comment on attachment 780690 [details] [diff] [review]
Patch v1

Review of attachment 780690 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Can you change the comment above CSSParserImpl::ParseFunctionInternals to mention that it consumes the ')'?  And also fix its two typos ("meant" and "called") while you're there.  r=me with comments addressed.

::: layout/style/test/test_css_function_mismatched_parenthesis.html
@@ +6,5 @@
> +This test verifies that:
> +(1) Mismatched parenthesis in a CSS function prevent parsing of subsequent CSS
> +properties.
> +(2) Properly matched parenthesis do not prevent parsing of subsequent CSS
> +properties.

parentheses

Comment 4

[Max Vujovic]

Attachment: Patch v2

Comment 5

[Max Vujovic]

Comment on attachment 780690 [details] [diff] [review]
Patch v1

Review of attachment 780690 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the review!

Updated the comment and fixed the typos.

::: layout/style/test/test_css_function_mismatched_parenthesis.html
@@ +6,5 @@
> +This test verifies that:
> +(1) Mismatched parenthesis in a CSS function prevent parsing of subsequent CSS
> +properties.
> +(2) Properly matched parenthesis do not prevent parsing of subsequent CSS
> +properties.

Done.

Comment 6

[Max Vujovic]

https://tbpl.mozilla.org/?tree=Try&rev=a67e7f011777

Comment 7

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

The code part of the patch looks great.  I'm a little concerned about the test -- I think it could be easier to understand and more maintainable if it were all run using script rather than having big style and HTML blocks.  That is, you could have a function something like:

function check_parens(declaration, parens_are_balanced) {
  var elt = document.getElementById("...");
  elt.setAttribute("style",
                   "background-color: " +
                     (parens_are_balanced ? "red" : "green") +
                     "; " + declaration + "; background-color: " +
                     (parens_are_balanced ? "green" : "red");
  is(elt.style.getPropertyValue("background-color"), "green",
     "parenthesis balancing within " + declaration);
}

and then have all the tests simply be calls to this function, such as:
  check_parens("transform: scale()", true);
  check_parens("transform: scale(,)", true);
  check_parens("transform: scale(", false);
  check_parens("transform: scale(,", false);

This also avoids going through style computation, which is unnecessary here, and thus should be faster.

Comment 8

[Max Vujovic]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #7)
> The code part of the patch looks great.  I'm a little concerned about the
> test -- I think it could be easier to understand and more maintainable if it
> were all run using script rather than having big style and HTML blocks. 
> That is, you could have a function something like:
> 
> function check_parens(declaration, parens_are_balanced) {
>   var elt = document.getElementById("...");
>   elt.setAttribute("style",
>                    "background-color: " +
>                      (parens_are_balanced ? "red" : "green") +
>                      "; " + declaration + "; background-color: " +
>                      (parens_are_balanced ? "green" : "red");
>   is(elt.style.getPropertyValue("background-color"), "green",
>      "parenthesis balancing within " + declaration);
> }
> 
> and then have all the tests simply be calls to this function, such as:
>   check_parens("transform: scale()", true);
>   check_parens("transform: scale(,)", true);
>   check_parens("transform: scale(", false);
>   check_parens("transform: scale(,", false);
> 
> This also avoids going through style computation, which is unnecessary here,
> and thus should be faster.

I like that function! I tried to avoid script to make the test easier to read, but I'm not sure it helped. This way is definitely more maintainable. I'll change the test. Thanks!

Comment 9

[Max Vujovic]

Attachment: Patch v3

New patch with dbaron's feedback.

Comment 10

[Max Vujovic]

https://tbpl.mozilla.org/?tree=Try&rev=10b2c210a7b5

Push new try job with Mochitests only. I didn't change any code since the last patch, only the test.

Comment 11

[Cameron McCormack (:heycam)]

Great; once you're happy with the try results, set the checkin-needed keyword.

Comment 12

[Max Vujovic]

(In reply to Cameron McCormack (:heycam) from comment #11)
> Great; once you're happy with the try results, set the checkin-needed
> keyword.

Thanks! Try results look good with only unrelated failures; ready for checkin.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/80be1fb00dc5

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/80be1fb00dc5