Crash with certain CSS border-* values and D2D on

RESOLVED FIXED in Firefox 50

Status

()

Core
Layout
--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Oriol, Assigned: bas)

Tracking

({crash, regression})

50 Branch
mozilla51
crash, regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox50 fixed, firefox51 fixed)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8784016 [details]
testcase.htm

Reported in bug 382721 comment 208.

Just open the attached testcase and Firefox crashes.
(Reporter)

Updated

2 years ago
status-firefox49: --- → unaffected
status-firefox50: --- → affected
status-firefox51: --- → affected
Flags: needinfo?(arai.unmht)
Keywords: crash, regression
I cannot reproduce the crash, on Firefox 50.0a2 (2016-08-23) on Windows 10 32bit.


here's the crash report from bug 382721 comment 208
https://crash-stats.mozilla.com/report/index/55a48e5d-3d2f-4244-8c7f-628f02160819

https://hg.mozilla.org/mozilla-unified/annotate/0b2e1f4c1af3/layout/base/nsCSSRenderingBorders.cpp#l2407
>       RefPtr<Path> path = builder->Finish();
>       mDrawTarget->Fill(path, ColorPattern(ToDeviceColor(borderColor)));

we don't check return value of `builder->Finish()` in any place,
and the crash report says `path` is nullptr, so I guess it hits this path.
https://dxr.mozilla.org/mozilla-central/rev/24763f58772d45279a935790f732d80851924b46/gfx/2d/PathD2D.cpp#368-372
> already_AddRefed<Path>
> PathBuilderD2D::Finish()
> {
> ...
>   HRESULT hr = mSink->Close();
>   if (FAILED(hr)) {
>     gfxCriticalNote << "Failed to close PathSink. Code: " << hexa(hr);
>     return nullptr;
>   }

jrmuizel, what do we do when `builder->Finish()` fails?
Flags: needinfo?(arai.unmht) → needinfo?(jmuizelaar)
(Reporter)

Comment 3

2 years ago
Created attachment 8784044 [details]
testcase2.htm

Maybe you will be able to reproduce the crash when changing borderWidth with JS instead of CSS animations.
Still cannot reproduce, tested on Windows 7 32bit.
(Reporter)

Comment 5

2 years ago
If I disable hardware acceleration or set gfx.direct2d.disabled=true, then there is no crash.

Might it be that you don't have D2D and thus can't reproduce?
could be.
I don't have any windows PC that can use hardware acceleration :/
Bas, should be able to say useful things about what to do when `builder->Finish()` fails?
Flags: needinfo?(jmuizelaar) → needinfo?(bas)
(Reporter)

Comment 8

2 years ago
Created attachment 8785148 [details]
testcase3.htm

In fact neither JS nor CSS animations are needed.
Attachment #8784016 - Attachment is obsolete: true
Attachment #8784044 - Attachment is obsolete: true
(Reporter)

Updated

2 years ago
Summary: Crash when animating border with border-radius → Crash with certain CSS border-* values and D2D on
Crash volume for signature 'mozilla::gfx::DrawTargetD2D1::Fill':
 - nightly (version 51): 18 crashes from 2016-08-01.
 - aurora  (version 50): 14 crashes from 2016-08-01.
 - beta    (version 49): 1 crash from 2016-08-02.
 - release (version 48): 2 crashes from 2016-07-25.
 - esr     (version 45): 0 crashes from 2016-05-02.

Crash volume on the last weeks (Week N is from 08-22 to 08-28):
            W. N-1  W. N-2  W. N-3
 - nightly       4       0       0
 - aurora       14       0       0
 - beta          1       0       0
 - release       0       0       0
 - esr           0       0       0

Affected platform: Windows

Crash rank on the last 7 days:
           Browser   Content     Plugin
 - nightly #60       #606
 - aurora            #53
 - beta
 - release #8078
 - esr
status-firefox48: --- → affected
status-firefox49: unaffected → affected
(Assignee)

Updated

2 years ago
Assignee: nobody → bas
Status: NEW → ASSIGNED
Flags: needinfo?(bas)
Comment hidden (mozreview-request)

Comment 11

2 years ago
mozreview-review
Comment on attachment 8785935 [details]
Bug 1297427: Discount border dots with negative radii.

https://reviewboard.mozilla.org/r/74954/#review72814

::: layout/base/nsCSSRenderingBorders.cpp:2410
(Diff revision 1)
>        segmentCount = 0;
>      }
>  
>      DottedCornerFinder::Result result = finder.Next();
>  
> -    if (marginedDirtyRect.Contains(result.C)) {
> +    if (marginedDirtyRect.Contains(result.C) && result.r > 0) {

Should we be taking the absolute value of the radius instead of dropping them completely?

::: layout/base/nsCSSRenderingBorders.cpp:2413
(Diff revision 1)
>      DottedCornerFinder::Result result = finder.Next();
>  
> -    if (marginedDirtyRect.Contains(result.C)) {
> +    if (marginedDirtyRect.Contains(result.C) && result.r > 0) {
>        entered = true;
>        builder->MoveTo(Point(result.C.x + result.r, result.C.y));
>        builder->Arc(result.C, result.r, 0, Float(2.0 * M_PI));

It would be good to add an assert to Arc for negative radii if it can't handle them.

::: layout/generic/crashtests/crashtests.list:638
(Diff revision 1)
>  load 1278007.html
>  load large-border-radius-dashed.html
>  load large-border-radius-dashed2.html
>  load large-border-radius-dotted.html
>  load large-border-radius-dotted2.html
> +load 1297427.html

let's call this 1297427-negative-radius.html
(In reply to Jeff Muizelaar [:jrmuizel] from comment #11)
> Comment on attachment 8785935 [details]
> Bug 1297427: Discount border dots with negative radii.
> 
> https://reviewboard.mozilla.org/r/74954/#review72814
> 
> ::: layout/base/nsCSSRenderingBorders.cpp:2410
> (Diff revision 1)
> >        segmentCount = 0;
> >      }
> >  
> >      DottedCornerFinder::Result result = finder.Next();
> >  
> > -    if (marginedDirtyRect.Contains(result.C)) {
> > +    if (marginedDirtyRect.Contains(result.C) && result.r > 0) {
> 
> Should we be taking the absolute value of the radius instead of dropping
> them completely?

Tooru, what makes the most sense here?
Flags: needinfo?(arai.unmht)
Thank you for finding out the issue :)

The case that r < 0 may happen if the input of CalculateDistanceToEllipticArc doesn't satisfy the assumption.
will check the detail shortly.
The actual issue was that the center curve doesn't share the origin with the outer curve and the inner curve in DottedCornerFinder,
but I reused the center curve's origin as the inner curve's origin, for CalculateDistanceToEllipticArc call,
that results in the wrong offset for the ellipse, and the point becomes inside of the ellipse.
will fix shortly.
Flags: needinfo?(arai.unmht)
Created attachment 8786099 [details] [diff] [review]
Part 2: Use calculate curve origin instead of reusing center curve origin in DottedCornerFinder.

Added mInnerCurveOrigin, in addition to rename mCurveOrigin to mCenterCurveOrigin.
they can be different and we should calculate them separately.

While we need to fix the calculation in DottedCornerFinder, it would be better also applying Bas's patch too, to avoid D2D crash on unknown case.
If r is negative, the result should be ignored, so skipping it would be reasonable.
We could still catch the wrong case in debug build, by the assertion in CalculateDistanceToEllipticArc.
Attachment #8786099 - Flags: review?(jmuizelaar)
status-firefox48: affected → ---
status-firefox49: affected → ---
(Assignee)

Comment 16

2 years ago
Comment on attachment 8786099 [details] [diff] [review]
Part 2: Use calculate curve origin instead of reusing center curve origin in DottedCornerFinder.

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

Hrm, how come we're not getting the inner and the outer origin here? Rather than inner and center?
(Assignee)

Comment 17

2 years ago
Comment on attachment 8786099 [details] [diff] [review]
Part 2: Use calculate curve origin instead of reusing center curve origin in DottedCornerFinder.

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

Never mind, I was being thick. This makes sense.
Attachment #8786099 - Flags: review?(jmuizelaar) → review+
Comment hidden (mozreview-request)

Comment 19

2 years ago
mozreview-review
Comment on attachment 8785935 [details]
Bug 1297427: Discount border dots with negative radii.

https://reviewboard.mozilla.org/r/74954/#review73646

::: layout/generic/crashtests/crashtests.list:638
(Diff revision 2)
>  load 1278007.html
>  load large-border-radius-dashed.html
>  load large-border-radius-dashed2.html
>  load large-border-radius-dotted.html
>  load large-border-radius-dotted2.html
> +load 1297427-non-equal-centers.html

Looks like the test wasn't included in the patch?

Comment 20

2 years ago
mozreview-review
Comment on attachment 8785935 [details]
Bug 1297427: Discount border dots with negative radii.

https://reviewboard.mozilla.org/r/74954/#review73706
Attachment #8785935 - Flags: review?(jmuizelaar) → review+

Comment 21

2 years ago
mozreview-review
Comment on attachment 8785935 [details]
Bug 1297427: Discount border dots with negative radii.

https://reviewboard.mozilla.org/r/74954/#review73708

And please include the test.
Is this ready to land?
Flags: needinfo?(bas)
Flags: needinfo?(arai.unmht)
I'll land mine after testing on latest m-i.
Flags: needinfo?(arai.unmht)
Keywords: leave-open
https://hg.mozilla.org/integration/mozilla-inbound/rev/c39042e6ec52f3ff6d7bc5401fc5c4b42bed570b
Bug 1297427 - Use calculate curve origin instead of reusing center curve origin in DottedCornerFinder. r=bas
Comment hidden (mozreview-request)

Comment 27

2 years ago
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1c72b311d2c6
Discount border dots with negative radii. r=jrmuizel
(Assignee)

Updated

2 years ago
Flags: needinfo?(bas)
Comment on attachment 8786099 [details] [diff] [review]
Part 2: Use calculate curve origin instead of reusing center curve origin in DottedCornerFinder.

Approval Request Comment
[Feature/regressing bug #]: bug 1285658
[User impact if declined]: Crashes with certain borders
[Describe test coverage new/current, TreeHerder]: Has been on nightly for a while and includes a tests case.
[Risks and why]: Limited.
Attachment #8786099 - Flags: approval-mozilla-aurora?
Comment on attachment 8786099 [details] [diff] [review]
Part 2: Use calculate curve origin instead of reusing center curve origin in DottedCornerFinder.

Crash fix, Aurora50+
Attachment #8786099 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Does this need to be open still?
Flags: needinfo?(bas)
(Assignee)

Comment 33

2 years ago
It does not.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(bas)
Keywords: leave-open
Resolution: --- → FIXED
status-firefox51: affected → fixed
Target Milestone: --- → mozilla51

Comment 34

2 years ago
this signature is reappearing in bigger volume again on the beta channel since 51.0b12...
Flags: needinfo?(bas)
(Reporter)

Comment 35

2 years ago
(In reply to [:philipp] from comment #34)
> this signature is reappearing in bigger volume again on the beta channel
> since 51.0b12...

For most crash reports, this signature comes from CanvasRenderingContext2D::Fill, and thus seems not related with this bug, which was about borders.
Right, tracked in bug 1331274 instead.
Flags: needinfo?(bas)
You need to log in before you can comment on or make changes to this bug.