Closed Bug 1324713 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

test-clip-path-circle.html
177 bytes, text/html
Details
Bug 1324713 - Fix integer overflow in CreateClipPathCircle().
58 bytes, text/x-review-board-request
dholbert
: review+
Details 
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: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: