Crash in [@ mozilla::gfx::DrawTargetD2D1::CreateBrushForPattern]

RESOLVED FIXED in Firefox 66

Status

()

defect
P1
critical
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: philipp, Assigned: bas.schouten)

Tracking

({crash, regression})

66 Branch
mozilla67
All
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox65 unaffected, firefox66blocking fixed, firefox67+ fixed)

Details

(crash signature)

Attachments

(3 attachments)

Reporter

Description

3 months ago

This bug is for crash report bp-8b436030-f2df-49bb-b712-298950190207.

Top 10 frames of crashing thread:

0 xul.dll struct already_AddRefed<ID2D1Brush> mozilla::gfx::DrawTargetD2D1::CreateBrushForPattern gfx/2d/DrawTargetD2D1.cpp:2065
1 xul.dll mozilla::gfx::DrawTargetD2D1::FillRect gfx/2d/DrawTargetD2D1.cpp:563
2 xul.dll void gfxSurfaceDrawable::DrawInternal gfx/thebes/gfxDrawable.cpp
3 xul.dll gfxSurfaceDrawable::Draw gfx/thebes/gfxDrawable.cpp:64
4 xul.dll static struct already_AddRefed<gfxDrawable> CreateSamplingRestrictedDrawable gfx/thebes/gfxUtils.cpp:321
5 xul.dll gfxUtils::DrawPixelSnapped gfx/thebes/gfxUtils.cpp:545
6 xul.dll mozilla::image::ImgDrawResult mozilla::image::RasterImage::DrawInternal image/RasterImage.cpp:1367
7 xul.dll mozilla::image::RasterImage::Draw image/RasterImage.cpp:1429
8 xul.dll static mozilla::image::ImgDrawResult DrawImageInternal layout/base/nsLayoutUtils.cpp:6584
9 xul.dll nsLayoutUtils::DrawBackgroundImage layout/base/nsLayoutUtils.cpp:6798

there are still a number of [@ mozilla::gfx::DrawTargetD2D1::CreateBrushForPattern] content crashes after the patches from the "see also" bugs have gone into firefox 66.0b5 (the volume has noticeably reduced though).

the stack from this particular report is the most common remaining one, but there are others too: https://bit.ly/2TxLYWW

Assignee

Updated

3 months ago
Assignee: nobody → bas
Status: NEW → ASSIGNED
Reporter

Updated

3 months ago
See Also: 15224151524554
Priority: -- → P1

From the channel meeting today: this signature = 2.2% of beta content crashes.

Assignee

Comment 2

3 months ago

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #1)

From the channel meeting today: this signature = 2.2% of beta content crashes.

The proper fix to this is probably not great to uplift. I can create a bandaid patch that probably addresses this, but in a less desirable manner, that we can uplift, how important do we believe it is to ship a patch for this in beta?

Flags: needinfo?(lhenry)

Still pretty important as the crash volume is quite high for beta and i expect it would be our top crash on release.

Flags: needinfo?(lhenry) → needinfo?(bas)
Assignee

Comment 4

3 months ago

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #3)

Still pretty important as the crash volume is quite high for beta and i expect it would be our top crash on release.

It should be noted that beta tends to have more device resets than release. However it will very likely still be top 5, we can make an upliftable patch.

Flags: needinfo?(bas)

Comment 6

3 months ago
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1aef51083749
Avoid invalid DrawTargets when not using OMTP. r=rhunt

Comment 7

3 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Assignee

Comment 8

3 months ago

I still found one crash on nightly with this patch. We'll have to uplift it to beta if we want to have an idea of how much this reduces the crashes.

Assignee

Comment 9

3 months ago

Comment on attachment 9044669 [details]
Bug 1526045: Avoid invalid DrawTargets when not using OMTP. r=rhunt

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: Bug 1519760
  • User impact if declined: Crashes
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Just an extra verification of surface validity.
  • String changes made/needed: None
Attachment #9044669 - Flags: approval-mozilla-beta?

Comment on attachment 9044669 [details]
Bug 1526045: Avoid invalid DrawTargets when not using OMTP. r=rhunt

Trying to bring down this crash volume. OK for beta 11 uplift.

Attachment #9044669 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee

Comment 12

3 months ago

I'm still not happy with the crashrates here on nightly, not sure about beta yet but speculatively I'm going to deliver another, more extensive fix.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 13

3 months ago

Preferrably CreateOffscreenContentDrawTarget would create Capture DrawTargets when we intend to use OMTP. This is not the case at the moment though and changing this would likely introduce more unforseen issues. For now all of these calls basically mean a DrawTarget will be used on the main thread and we should use a no-op ClearRect to ensure that the DrawTarget is actually initialized. Since IsValid for the moment won't do this for DrawTargetD2D. (See bug 1521368)

Comment 14

3 months ago
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e6973ba9fe5
Part 2: Verify all DrawTargets created through CreateOffscreenContentDrawTarget. r=rhunt

Comment 16

3 months ago
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0659ccd37ee7
Part 2: Verify all DrawTargets created through CreateOffscreenContentDrawTarget. r=rhunt

Comment 17

3 months ago
bugherder
Status: REOPENED → RESOLVED
Last Resolved: 3 months ago3 months ago
Resolution: --- → FIXED

The volume is still so high for beta that I'm marking this as a release blocker for 66. (With hopes we can uplift these patches and keep bringing the crash volume down.)

Want to go ahead and request uplift? As with the other patches it may just be hard to tell the results from Nightly.

Assignee

Comment 20

3 months ago

Comment on attachment 9046405 [details]
Bug 1526045 - Part 2: Verify all DrawTargets created through CreateOffscreenContentDrawTarget. r=rhunt

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: Bug 1519760
  • User impact if declined: Crashes
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Just an additional verification step.
  • String changes made/needed: None
Flags: needinfo?(bas)
Attachment #9046405 - Flags: approval-mozilla-beta?
Assignee

Comment 21

3 months ago

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #19)

Want to go ahead and request uplift? As with the other patches it may just be hard to tell the results from Nightly.

It certainly looks promising, 3 nightlies now with 0 of these crashes.

Comment on attachment 9046405 [details]
Bug 1526045 - Part 2: Verify all DrawTargets created through CreateOffscreenContentDrawTarget. r=rhunt

Another mitigation for big topcrash in beta.
Let's try to land this for beta 12.

Attachment #9046405 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 24

3 months ago
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a57aaf355b64
Part 2 - Addition: Fix merge error. r=rhunt
Assignee

Comment 26

3 months ago

It looks like this has reduced the volume considerably, there's two obvious crashes left from what I can see, one has a patch coming up on this bug. This other is a case of bug 1524554.

Comment 28

3 months ago
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c61045cda2f
Part 3: Ensure DrawTarget validity inside gfxBlur::InitDrawTarget. r=rhunt
Assignee

Comment 30

3 months ago

Comment on attachment 9048119 [details]
Bug 1526045 - Part 3: Ensure DrawTarget validity insite gfxBlur::InitDrawTarget. r=rhunt

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: Bug 1519760
  • User impact if declined: Crashes
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Additional verification.
  • String changes made/needed:
Attachment #9048119 - Flags: approval-mozilla-beta?

Comment on attachment 9048119 [details]
Bug 1526045 - Part 3: Ensure DrawTarget validity insite gfxBlur::InitDrawTarget. r=rhunt

Yet another crash fix. Let's take it!

Attachment #9048119 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee

Comment 33

2 months ago

Based on the crash reports that are left the only remaining bug is bug 1524554. Investigating that further.

You need to log in before you can comment on or make changes to this bug.