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

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[[:philipp]]

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

Comment 1

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 2

[Bas Schouten (:bas.schouten)]

(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?

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 4

[Bas Schouten (:bas.schouten)]

(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.

Comment 5

[Bas Schouten (:bas.schouten)]

Attachment: Bug 1526045: Avoid invalid DrawTargets when not using OMTP. r=rhunt

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1aef51083749
Avoid invalid DrawTargets when not using OMTP. r=rhunt

Comment 7

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/1aef51083749

Comment 8

[Bas Schouten (:bas.schouten)]

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.

Comment 9

[Bas Schouten (:bas.schouten)]

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

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/91995baa7317

Comment 12

[Bas Schouten (:bas.schouten)]

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.

Comment 13

[Bas Schouten (:bas.schouten)]

Attachment: Bug 1526045 - Part 2: Verify all DrawTargets created through CreateOffscreenContentDrawTarget. r=rhunt

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e6973ba9fe5
Part 2: Verify all DrawTargets created through CreateOffscreenContentDrawTarget. r=rhunt

Comment 15

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 0e6973ba9fe5 (Bug 1526045) for build bustages at gfxPlatform.cpp

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/1de4e8186b7fdbbb35a2dbd3f0b934be31166814

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&selectedJob=230480852&revision=0e6973ba9fe579d79bd2845fabb5e17179b33af7

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=230480852&repo=mozilla-inbound&lineNumber=29133

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0659ccd37ee7
Part 2: Verify all DrawTargets created through CreateOffscreenContentDrawTarget. r=rhunt

Comment 17

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/0659ccd37ee7

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

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.)

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 20

[Bas Schouten (:bas.schouten)]

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

Comment 21

[Bas Schouten (:bas.schouten)]

(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 22

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 23

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/releases/mozilla-beta/rev/46f0f4418edd

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a57aaf355b64
Part 2 - Addition: Fix merge error. r=rhunt

Comment 25

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/a57aaf355b64

Comment 26

[Bas Schouten (:bas.schouten)]

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 27

[Bas Schouten (:bas.schouten)]

Attachment: Bug 1526045 - Part 3: Ensure DrawTarget validity insite gfxBlur::InitDrawTarget. r=rhunt

Depends on D21899

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c61045cda2f
Part 3: Ensure DrawTarget validity inside gfxBlur::InitDrawTarget. r=rhunt

Comment 29

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/6c61045cda2f

Comment 30

[Bas Schouten (:bas.schouten)]

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:

Comment 31

[Liz Henry (:lizzard) (relman/hg->git project)]

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!

Comment 32

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5892e926a68c

Comment 33

[Bas Schouten (:bas.schouten)]

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

Comment 34

[Firefox Bug Husbandry Bot]

Please specify a root cause for this bug. See :tmaity for more information.