1695725 - Crash in [@ mozilla::dom::CanvasRenderingContext2D::EnsureErrorTarget]

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Chris Peterson [:cpeterson]]

Fission Crash, all 128 crashes (from 8 installs) have Fission enabled

Potential Startup Crash, more than half of the crashes happened during the first minute after launch

Browser Crash

Crash report: https://crash-stats.mozilla.org/report/index/31cfebc5-28cf-43b5-ac8f-8f42d0210301

Reason: EXCEPTION_ACCESS_VIOLATION_WRITE

Top 10 frames of crashing thread:

0 xul.dll static mozilla::dom::CanvasRenderingContext2D::EnsureErrorTarget dom/canvas/CanvasRenderingContext2D.cpp:5130
1 xul.dll mozilla::dom::CanvasRenderingContext2D::SetErrorState dom/canvas/CanvasRenderingContext2D.cpp:1386
2 xul.dll mozilla::dom::CanvasRenderingContext2D::EnsureTarget dom/canvas/CanvasRenderingContext2D.cpp:1320
3 xul.dll mozilla::dom::CanvasRenderingContext2D::FillRect dom/canvas/CanvasRenderingContext2D.cpp:2605
4 xul.dll mozilla::dom::CanvasRenderingContext2D_Binding::fillRect dom/bindings/CanvasRenderingContext2DBinding.cpp:5913
5 xul.dll mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions> dom/bindings/BindingUtils.cpp:3235
6 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:520
7 xul.dll Interpret js/src/vm/Interpreter.cpp:3243
8 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:552
9 xul.dll JS::Call js/src/jsapi.cpp:2863

Comment 1

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

All crashes are likely from the same machine: same graphics adapter, same non-en-US locale. crash-stats doesn't track users which means a Firefox update will get picked up as new install.

Comment 2

[Bob Owen (:bobowen) PTO until 13th Aug]

This must be somehow related to bug 1692894 it landed in build 20210225215504.

From the environment "D2DEnabled":false but "d2d":{"status":"available","version":"1.1"}, which is odd.
It should still just fall back to skia though.

I've tried to recreate by flipping various prefs, but I can't get a crash.

Comment 3

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Looking through the URLs of the crashes, they are mostly or entirely individual images files (JPG, PNG, etc.) which is unusual.

For instance: https://www.rt.com/static/img/og-logo-rt.png

Which is just a logo for RT.

Comment 4

[Andrew McCreight (out of office until 8/21) [:mccr8]]

It looks like at least one of the crashes is using this image blocking addon, so maybe that's related? https://addons.mozilla.org/en-US/firefox/addon/image-block/

Comment 5

[Jim Mathies [:jimm]]

Maybe this should be tracked by a fission milestone?

Comment 6

[Andrew McCreight (out of office until 8/21) [:mccr8]]

This doesn't look like a common crash, but it does look highly correlated to Fission.

Looking at the full stack in the crash report, the JS is being run inside a call to WindowGlobalChild::RecvRawMessage and JSActor::CallReceiveMessage, so I think that means we're running code for a JS window actor, if that helps narrow things down a bit.

This also appears to be a call to CanvasRenderingContext2D_Binding::fillRect().

Looking at the code, if CreateOffscreenCanvasDrawTarget returns null in CanvasRenderingContext2D::EnsureErrorTarget(), then we're going to try to addref a null pointer, which will crash. Maybe that's the issue, though the line numbers are off.

The Graphics critical error section for these crashes all have things like this: |[C0][GFX1]: Failed borrow shared and basic targets. (t=9103.68)

That looks like a failure annotation near the end of CanvasRenderingContext2D::EnsureTarget() so maybe that is related: https://searchfox.org/mozilla-central/rev/5e70cd673a0ba0ad19b662c1cf656e0823781596/dom/canvas/CanvasRenderingContext2D.cpp#1316

Comment 7

[Andrew McCreight (out of office until 8/21) [:mccr8]]

If NS_ADDREF(sErrorTarget); was changed to NS_IF_ADDREF(sErrorTarget); I think we'd avoid crashing, because AFAICT nothing else dereferences sErrorTarget, but I don't know if the resulting behavior would be reasonable or not.

Comment 8

[Chris Peterson [:cpeterson]]

Sending to gfx-triage

@ Bob, could this crash be fallout from your fix for bug 1692894 ("Stop creating Direct3D/2D devices when no longer required in the content process")? 100% of the (1172) Nightly crash reports have Fission enabled, but the ~30 crash reports from DevEdition/Beta and Release don't have Fission enabled.

This crash spiked in 88.0a1 Nightly. The earliest build ID is 20210225215504. Here is the changelog between 2021-02-24 and 2021-02-25, which includes your fix for bug 1692894:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b3eb91f0b5a7713390e5016f1f59e3a7f8d9a4f2&tochange=0dbba4cd204153adcf3b8f09c1d27db056470599

Should EnsureErrorTarget handle a null errorTarget in release builds instead of just debug MOZ_ASSERT? Is this an OOM?

https://hg.mozilla.org/mozilla-central/file/f875a4ffd653fb9a1ce1567323598057af32ebf0/dom/canvas/CanvasRenderingContext2D.cpp#l5128

The JSWindowActor message sent is "Browser:Thumbnail:ContentInfo" here:

https://searchfox.org/mozilla-central/rev/a8b75e4ba3f8ddf0e76b42681d0a7b7e78e67730/toolkit/actors/BackgroundThumbnailsChild.jsm#26

Comment 9

[Bob Owen (:bobowen) PTO until 13th Aug]

(In reply to Chris Peterson [:cpeterson] from comment #8)

Sending to gfx-triage

@ Bob, could this crash be fallout from your fix for bug 1692894 ("Stop creating Direct3D/2D devices when no longer required in the content process")? 100% of the (1172) Nightly crash reports have Fission enabled, but the ~30 crash reports from DevEdition/Beta and Release don't have Fission enabled.

Yes, it must be down to this change, I've reproduced by removing skia from gfx.canvas.azure.backends and doing a full screenshot.
I didn't get a visible notification of the crash, the screenshot just doesn't appear.

I'm not sure how this could happen without someone changing that pref.

lsalzman - is there a reason why we shouldn't always fall back to skia even if it is not in the pref?
I guess the other alternative is to just not crash (which we should probably do anyway), but whatever is using the off screen canvas will obviously still fail.

Comment 10

[Lee Salzman [:lsalzman]]

I would not be opposed to forcing mFallbackCanvasBackend to SKIA if it somehow ended up as NONE, if you want to write a patch (or maybe to mSoftwareBackend if you wanted to be generic).

Comment 11

[Bob Owen (:bobowen) PTO until 13th Aug]

(In reply to Lee Salzman [:lsalzman] from comment #10)

I would not be opposed to forcing mFallbackCanvasBackend to SKIA if it somehow ended up as NONE, if you want to write a patch (or maybe to mSoftwareBackend if you wanted to be generic).

Thanks, I'll get onto this tomorrow.

Comment 12

[Bob Owen (:bobowen) PTO until 13th Aug]

Attachment: Bug 1695725 p1: Use mSoftwareBackend if we don't have a fallback canvas backend. r=lsalzman!

Comment 13

[Bob Owen (:bobowen) PTO until 13th Aug]

Attachment: Bug 1695725 p2: Guard against a failure to create sErrorTarget in CanvasRenderingContext2D. r=lsalzman!

Depends on D112963

Comment 14

[Bob Owen (:bobowen) PTO until 13th Aug]

https://treeherder.mozilla.org/jobs?repo=try&revision=1b8f96a6537d9027f072b884707c62a1cf743660

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/055d0eaac283
p1: Use mSoftwareBackend if we don't have a fallback canvas backend. r=lsalzman
https://hg.mozilla.org/integration/autoland/rev/2c9e8d26dfcc
p2: Guard against a failure to create sErrorTarget in CanvasRenderingContext2D. r=lsalzman

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/055d0eaac283
https://hg.mozilla.org/mozilla-central/rev/2c9e8d26dfcc

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:bobowen, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 18

[Bob Owen (:bobowen) PTO until 13th Aug]

Comment on attachment 9217412 [details]
Bug 1695725 p1: Use mSoftwareBackend if we don't have a fallback canvas backend. r=lsalzman!

Beta/Release Uplift Approval Request

• User impact if declined: It's possible that this can only occur when someone misconfigures a pref, but it means certain features like snapshot can cause a tab crash.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: * remove skia from gfx.canvas.azure.backends pref and restart
• do a full screenshot
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Very simple changes to always fallback to the same software backend as content and also some additional null checks.
• String changes made/needed:

Comment 19

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Hello reproduced the issue with Firefox 88.0a1 (20210301220015) on Windows 10x64 as stated in comment 9 by following the steps from comment 18. The Firefox browser does not crash but Screenshot fails to work with full-page after removing skia from gfx.canvas.azure.backends pref.
Verified fixed with Firefox 90.0a1 (20210428215523) on Windows 10x64, macOS 11.2.3 and Ubuntu 20.04. Screenshot a full-page works as expected while skia is removed from gfx.canvas.azure.backends pref.

Comment 20

[Pascal Chevrel:pascalc]

Comment on attachment 9217412 [details]
Bug 1695725 p1: Use mSoftwareBackend if we don't have a fallback canvas backend. r=lsalzman!

Bob, we only have one crash in 89 beta with this signature so it seems to me that this is not a good candidate for an uplift in beta and that it can ride the 90 train. Don't hesitate to contact me if you think that I should reconsider my position, thanks!