1324713 - clip-path: circle() with large reference box and percentage radius does not render correctly

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: test-clip-path-circle.html

Open the test case attached with "layout.css.clip-path-shapes.enabled" set to true. 

Expected result:
A green circle.

Actual result:
A green box.

Comment 1

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1324713 - Fix integer overflow in CreateClipPathCircle().

The sum of the squares is prone to overflow, resulting |referenceLength| a
NaN. Convert them to float before taking squares. Without this patch,
clip-path-circle-021.html will be rendered as a rectangle.

Review commit: https://reviewboard.mozilla.org/r/99722/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99722/

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8820195 [details]
Bug 1324713 - Fix integer overflow in CreateClipPathCircle().

https://reviewboard.mozilla.org/r/99722/#review100318

::: layout/svg/nsCSSClipPathInstance.cpp:253
(Diff revision 1)
> -  float referenceLength = sqrt((aRefBox.width * aRefBox.width +
> -                                aRefBox.height * aRefBox.height) / 2.0);
> +  float referenceLength = std::sqrt((float(aRefBox.width) * aRefBox.width +
> +                                     float(aRefBox.height) * aRefBox.height) / 2.0);

Let's use our standard NS_hypot() function here, from nsMathUtils.h. (It's implemented in terms of something that can be hardware-accelerated when possible.)

I think that means this should be something like:

  constexpr double sqrt2 = std::sqrt(2.0);
  double referenceLength = NS_hypot(aRefBox.width, aRefBox.height) / sqrt2;

Might be worth dropping a spec link to https://www.w3.org/TR/css-shapes/#funcdef-circle while you're here, to explain this slightly-odd math (the sqrt2 in particular).

ALSO: while you're here, we should move this line down to the one specific clause where it's used -- in the "else" clause for the subsequent if-check.

ALSO: Looks like the one usage of this variable -- a call to ComputeCoordPercentCalc -- expects a nscoord, not a double/float. So, we should wrap this variable in NSToCoordRound() when we pass it into that function.

::: layout/svg/nsCSSClipPathInstance.cpp:255
(Diff revision 1)
>  
>    const nsTArray<nsStyleCoord>& coords = basicShape->Coordinates();
>    MOZ_ASSERT(coords.Length() == 1, "wrong number of arguments");
> -  float referenceLength = sqrt((aRefBox.width * aRefBox.width +
> -                                aRefBox.height * aRefBox.height) / 2.0);
> +  float referenceLength = std::sqrt((float(aRefBox.width) * aRefBox.width +
> +                                     float(aRefBox.height) * aRefBox.height) / 2.0);
> +  MOZ_ASSERT(referenceLength >= 0, "Reference length must not be negative!");

Is there a reason you're adding this MOZ_ASSERT?

I can see that it happens to be valid (because sqrt is guaranteed to return something nonnegative), but I don't see any code that strongly depends on referenceLength being nonnegative to avoid crashing or anything else egregious.  So I'm not clear what the value is of asserting about this here.

(I might be missing something, though.)

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8820195 [details]
Bug 1324713 - Fix integer overflow in CreateClipPathCircle().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99722/diff/1-2/

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8820195 [details]
Bug 1324713 - Fix integer overflow in CreateClipPathCircle().

https://reviewboard.mozilla.org/r/99722/#review100522

::: layout/svg/nsCSSClipPathInstance.cpp:253
(Diff revision 1)
> -  float referenceLength = sqrt((aRefBox.width * aRefBox.width +
> -                                aRefBox.height * aRefBox.height) / 2.0);
> +  float referenceLength = std::sqrt((float(aRefBox.width) * aRefBox.width +
> +                                     float(aRefBox.height) * aRefBox.height) / 2.0);

Thanks for pointing NS_hypot() to me! Updated my patch to address all the comments.

::: layout/svg/nsCSSClipPathInstance.cpp:255
(Diff revision 1)
>  
>    const nsTArray<nsStyleCoord>& coords = basicShape->Coordinates();
>    MOZ_ASSERT(coords.Length() == 1, "wrong number of arguments");
> -  float referenceLength = sqrt((aRefBox.width * aRefBox.width +
> -                                aRefBox.height * aRefBox.height) / 2.0);
> +  float referenceLength = std::sqrt((float(aRefBox.width) * aRefBox.width +
> +                                     float(aRefBox.height) * aRefBox.height) / 2.0);
> +  MOZ_ASSERT(referenceLength >= 0, "Reference length must not be negative!");

I added MOZ_ASSERT() in case that `sqrt` returns NaN when the input value is nagative. Now we use `NS_hypot`, so I don't think it'll happen. I dropped the assert.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8820195 [details]
Bug 1324713 - Fix integer overflow in CreateClipPathCircle().

https://reviewboard.mozilla.org/r/99722/#review100532

r=me, with nits addressed.

First, a commit message nit -- this bit of the extended commit message isn't really clear:
> The sum of the squares is prone to overflow, resulting |referenceLength| a
> NaN


I think you want to say something like the following:
====
Before this patch, we did a sum-of-squares operation with nscoord variables,
which could overflow (to a negative value), and that would then produce NaN
when sqrt()'ed.  We'll now avoid this by using 'double' variables & NS_hypot.
====


Please clarify with something along those lines.

::: layout/svg/nsCSSClipPathInstance.cpp:266
(Diff revision 2)
> -    r = nsRuleNode::ComputeCoordPercentCalc(coords[0], referenceLength);
> +    // Definition of <shape-radius> of circle():
> +    // https://drafts.csswg.org/css-shapes/#funcdef-circle
> +    constexpr double sqrt2 = std::sqrt(2.0);

This comment is a bit too vague.  (It should mention percents, since that's what we're specifically getting set up to handle in this clause.)

Please reword to something like:
    // We resolve percent <shape-radius> vals for circle() as defined here:
    // https://drafts.csswg.org/css-shapes/#funcdef-circle

::: layout/svg/nsCSSClipPathInstance.cpp:269
(Diff revision 2)
>      }
>    } else {
> -    r = nsRuleNode::ComputeCoordPercentCalc(coords[0], referenceLength);
> +    // Definition of <shape-radius> of circle():
> +    // https://drafts.csswg.org/css-shapes/#funcdef-circle
> +    constexpr double sqrt2 = std::sqrt(2.0);
> +    double referenceLength = NS_hypot(aRefBox.width, aRefBox.height) / sqrt2;

Please add...
 #include "nsMathUtils.h"
...at the top of this file, to provide the definition of NS_hypot.

Comment 6

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8820195 [details]
Bug 1324713 - Fix integer overflow in CreateClipPathCircle().

https://reviewboard.mozilla.org/r/99722/#review100544

::: layout/svg/nsCSSClipPathInstance.cpp:268
(Diff revision 2)
>        r = horizontal < vertical ? horizontal : vertical;
>      }
>    } else {
> -    r = nsRuleNode::ComputeCoordPercentCalc(coords[0], referenceLength);
> +    // Definition of <shape-radius> of circle():
> +    // https://drafts.csswg.org/css-shapes/#funcdef-circle
> +    constexpr double sqrt2 = std::sqrt(2.0);

Try doesn't like `constexpr` since `std::sqrt` might not be defined as a `constexpr` function. I'll use `const` here.

Comment 7

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8820195 [details]
Bug 1324713 - Fix integer overflow in CreateClipPathCircle().

https://reviewboard.mozilla.org/r/99722/#review100532

> Please add...
>  #include "nsMathUtils.h"
> ...at the top of this file, to provide the definition of NS_hypot.

Nice catch. Thanks!

Comment 8

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8820195 [details]
Bug 1324713 - Fix integer overflow in CreateClipPathCircle().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99722/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6fb85d407c4f
Fix integer overflow in CreateClipPathCircle(). r=dholbert

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6fb85d407c4f