1747176 - Crash in [@ mozilla::webgl::FromDomElem]

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect, P3)

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/c761db0c-f41d-482d-9450-c28c80211217

Reason: EXC_BAD_ACCESS / KERN_INVALID_ADDRESS

Top 10 frames of crashing thread:

0 XUL mozilla::webgl::FromDomElem dom/canvas/WebGLTextureUpload.cpp:233
1 XUL mozilla::ClientWebGLContext::TexImage const dom/canvas/ClientWebGLContext.cpp:4003
2 XUL void mozilla::ClientWebGLContext::TexImage2D<mozilla::dom::HTMLCanvasElement> const dom/canvas/ClientWebGLContext.h:1764
3 XUL mozilla::dom::WebGL2RenderingContext_Binding::texImage2D dom/bindings/WebGL2RenderingContextBinding.cpp:2028
4 XUL bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions> dom/bindings/BindingUtils.cpp:3306
5 XUL Interpret js/src/vm/Interpreter.cpp:3243
6 XUL js::RunScript js/src/vm/Interpreter.cpp:357
7 XUL InternalCall js/src/vm/Interpreter.cpp:535
8 XUL js::jit::DoCallFallback js/src/jit/BaselineIC.cpp:1595
9 None @0x00001ef858cbfe17 

Looks like this first showed up in the 20211216163155 build. Here's the commits added in that build. Nothing seems obviously related. Could this be a signature change?

Null deref, on this line:
nsIPrincipal* dstPrincipal = canvas.NodePrincipal();

It doesn't really make sense to me that there could be a null deref there, so I don't know.

Comment 1

[Chris Peterson [:cpeterson]]

This crash looks like a regression in Nightly 97. All platforms are affected, but Android's crash volume is higher than Windows' (133 vs 53).

Comment 2

[Christian Holler (:decoder)]

The crashes started a bit before OffscreenCanvas was enabled in bug 1746110, but many of the crashes on Dec 17 seem to be from the same machine and almost around the same time. Maybe someone was manually switching on that pref before?

Andrew, could this be related?

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Crashes went away when OffscreenCanvas was preffed off.

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

I wouldn't have thought this would even be reachable from worker threads, so we should probably add a check and throw if not on the main thread and we are given a DOM element:
https://searchfox.org/mozilla-central/rev/eb554e155a28ad36aca62281406757833b9c467a/dom/canvas/ClientWebGLContext.cpp#4071

As for why we crash on this line, even on the main thread:
https://searchfox.org/mozilla-central/rev/eb554e155a28ad36aca62281406757833b9c467a/dom/canvas/WebGLTextureUpload.cpp#233

canvas, the associated HTMLCanvasElement, must be null for the OffscreenCanvas. This is expected. We should check if it has a bound HTMLCanvasElement, and if not, also throw here.

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1747176 - Make ClientWebGLContext::TexImage2D with a DOM element work with OffscreenCanvas.

We cannot access DOM elements off the main thread, so any uses of that
must be a mistake. Also, if we are in use with an OffscreenCanvas, we
won't have an HTMLCanvasElement, so we need to make sure we pull the
nsIPrincipal from the OffscreenCanvas object instead.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6dcb3a75b392
Make ClientWebGLContext::TexImage2D with a DOM element work with OffscreenCanvas. r=jgilbert

Comment 7

[Cristina Cozmuta (:CrissCozmuta)]

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