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

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: TYLin, Assigned: TYLin)

Tracking

(Blocks 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(2 attachments)

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.
Blocks: basic-shape
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 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 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.
Attachment #8820195 - Flags: review?(dholbert) → review+
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 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!
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6fb85d407c4f
Fix integer overflow in CreateClipPathCircle(). r=dholbert
https://hg.mozilla.org/mozilla-central/rev/6fb85d407c4f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.