1890077 - Crash in [@ mozilla::dom::CanvasRenderingContext2D::EnsureWritablePath]

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/23accbe6-ffa9-45e3-8cb0-9915f0240404

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  xul.dll  mozilla::dom::CanvasRenderingContext2D::EnsureWritablePath  dom/canvas/CanvasRenderingContext2D.cpp:3801
1  xul.dll  mozilla::dom::CanvasRenderingContext2D::MoveTo  dom/canvas/CanvasRenderingContext2D.h:374
2  xul.dll  mozilla::dom::CanvasRenderingContext2D_Binding::moveTo  dom/bindings/CanvasRenderingContext2DBinding.cpp:6231
3  xul.dll  mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>  dom/bindings/BindingUtils.cpp:3269
4  xul.dll  CallJSNative  js/src/vm/Interpreter.cpp:479
4  xul.dll  js::InternalCallOrConstruct  js/src/vm/Interpreter.cpp:573
4  xul.dll  InternalCall  js/src/vm/Interpreter.cpp:640
4  xul.dll  js::CallFromStack  js/src/vm/Interpreter.cpp:645
4  xul.dll  js::Interpret  js/src/vm/Interpreter.cpp:3060
5  xul.dll  MaybeEnterInterpreterTrampoline  js/src/vm/Interpreter.cpp:393

New-ish looking crash. Looks like a null crash on mTarget. There's an EnsureTarget call earlier in the method, but the return value is ignored, so maybe it can fail?

Comment 1

[Andrew McCreight [:mccr8]]

The volume is rather low, but it looks like it first showed up in 125.

Comment 2

[Teodor Tanasoaia [:teoxoy]]

There's an EnsureTarget call earlier in the method, but the return value is ignored, so maybe it can fail?

I'm not too familiar with the code but something looks off there.

// NOTE: IsTargetValid() may be false here (mTarget == sErrorTarget) but we
// go ahead and create a path anyway since callers depend on that.

This note underneath assumes mTarget to never be null which it could be.

The only relevant change that landed in 125 seems to be 1885365.
Andrew, could you take a look?

Comment 3

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

The bug is linked to a topcrash signature, which matches the following criterion:

• Top 10 AArch64 and ARM crashes on nightly

For more information, please visit BugBot documentation.

Comment 4

[Chris Peterson [:cpeterson]]

We're also seeing crash reports from Android:

https://crash-stats.mozilla.org/report/index/46803c1d-faf6-48e3-81bd-88c2a0240415(In reply to Andrew McCreight [:mccr8] from comment #1)

The volume is rather low, but it looks like it first showed up in 125.

Though I do see one crash report from ESR 115.7.0 on 2024-01-24. Did we backport a regressing change to ESR 115? Or did an ESR user flip a pref?

https://crash-stats.mozilla.org/report/index/938011f0-f18c-4790-8799-d058a0240124

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Timing fits bug 1887729.

Comment 6

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

Attachment: Bug 1890077 - Ensure we check for a null target when creating paths for Canvas2D.

Comment 7

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

I tried testing locally on some of the URLs from the crash reports, in particular, https://www.flightradar24.com, but I could not reproduce. It is timing sensitive where it requires the GPU process to crash, and for the site to attempt to create a path for the canvas before the recovery has completed.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7b3b51a5efea
Ensure we check for a null target when creating paths for Canvas2D. r=gfx-reviewers,lsalzman

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7b3b51a5efea

Comment 10

[Donal Meehan [:dmeehan]]

:aosmond could you add a beta and release uplift request on this?
We could include it in 126.0b2 and in a later 125 dot release

Comment 11

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

Comment on attachment 9396719 [details]
Bug 1890077 - Ensure we check for a null target when creating paths for Canvas2D.

Beta/Release Uplift Approval Request

• User impact if declined: May crash if using canvas during GPU process recovery
• 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): We are just adding state guards to do nothing during an interim state between recoveries.
• String changes made/needed:
• Is Android affected?: Yes

Comment 12

[Donal Meehan [:dmeehan]]

Comment on attachment 9396719 [details]
Bug 1890077 - Ensure we check for a null target when creating paths for Canvas2D.

Approved for 126.0b2

Comment 13

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/9ced27dab437

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9396719 [details]
Bug 1890077 - Ensure we check for a null target when creating paths for Canvas2D.

Approved for Desktop 125.0.2 / Android 125.2.0

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-release/rev/fb09eb32de7b