Closed Bug 1297427 Opened 7 years ago Closed 7 years ago

Crash with certain CSS border-* values and D2D on

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Version:
50 Branch
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: Oriol, Assigned: bas.schouten)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(3 files, 2 obsolete files)

testcase3.htm
222 bytes, text/html
Details
Bug 1297427: Discount border dots with negative radii.
58 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Part 2: Use calculate curve origin instead of reusing center curve origin in DottedCornerFinder.
5.86 KB, patch
bas.schouten
: review+
ritu
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Attached file testcase.htm (obsolete) — 
Reported in bug 382721 comment 208.

Just open the attached testcase and Firefox crashes.
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)
Attached file testcase2.htm (obsolete) — 
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.
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)
Attached file testcase3.htm — 
In fact neither JS nor CSS animations are needed.
Attachment #8784016 - Attachment is obsolete: true
Attachment #8784044 - Attachment is obsolete: true
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: nobody → bas
Status: NEW → ASSIGNED
Flags: needinfo?(bas)
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)
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)
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?
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 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 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 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
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1c72b311d2c6
Discount border dots with negative radii. r=jrmuizel
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)
It does not.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: needinfo?(bas)
Keywords: leave-open
Resolution: --- → FIXED
status-firefox51: affected → fixed
Target Milestone: --- → mozilla51
this signature is reappearing in bigger volume again on the beta channel since 51.0b12...
Flags: needinfo?(bas)
(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.