1825169 - Crash in [@ mozilla::ThreadSafeWeakPtr<T>::operator=]

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

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

Crash report: https://crash-stats.mozilla.org/report/index/e79c5095-09c8-482c-a7d5-540dc0230328

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  xul.dll  RefPtr<mozilla::detail::ThreadSafeWeakReference>::assign_assuming_AddRef  mfbt/RefPtr.h:66
0  xul.dll  RefPtr<mozilla::detail::ThreadSafeWeakReference>::assign_with_AddRef  mfbt/RefPtr.h:62
0  xul.dll  RefPtr<mozilla::detail::ThreadSafeWeakReference>::operator=  mfbt/RefPtr.h:190
0  xul.dll  mozilla::ThreadSafeWeakPtr<mozilla::gfx::SourceSurface>::operator=  mfbt/ThreadSafeWeakPtr.h:218
1  xul.dll  mozilla::gfx::DrawTargetRecording::OptimizeSourceSurface const  gfx/2d/DrawTargetRecording.cpp:565
2  xul.dll  nsLayoutUtils::SurfaceFromElement  layout/base/nsLayoutUtils.cpp:7290
3  xul.dll  nsLayoutUtils::SurfaceFromElement  layout/base/nsLayoutUtils.cpp:7459
4  xul.dll  nsLayoutUtils::SurfaceFromElement  layout/base/nsLayoutUtils.h:2228
4  xul.dll  mozilla::dom::CanvasRenderingContext2D::DrawImage  dom/canvas/CanvasRenderingContext2D.cpp:4837
5  xul.dll  mozilla::dom::CanvasRenderingContext2D::DrawImage  dom/canvas/CanvasRenderingContext2D.h:256

Comment 1

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

Null pointer deref. Looks like the assert at https://searchfox.org/mozilla-central/rev/f9beb753a84aa297713d1565dcd0c5e3c66e4174/gfx/2d/DrawTargetRecording.cpp#562 is not holding true.

Comment 2

[Sotaro Ikeda [:sotaro]]

:bobowen, can you comment to this bug?

Comment 3

[Bob Owen (:bobowen)]

I stared at this for a long time, but couldn't see how this could happen.
The only way that the call to EnsureSurfaceStoredRecording couldn't add user data is if the object was already stored.
However, if the object is already stored it will already have user data.

So my guess was we were failing to remove old objects and the same memory was getting reused.
All the destructions seemed to call RemoveStoredObject properly though.

Then I realised that these two submit that removal (and recording of deletion) to the main thread, so the memory is freed and could be reused before that clean-up and recording of deletion happens.

• https://searchfox.org/mozilla-central/rev/55d5c4b9dffe5e59eb6b019c1a930ec9ada47e10/gfx/layers/CanvasDrawEventRecorder.cpp#525-527
• https://searchfox.org/mozilla-central/rev/55d5c4b9dffe5e59eb6b019c1a930ec9ada47e10/gfx/layers/ipc/CanvasChild.cpp#89-93

I can't be sure that this is what is causing this issue, but it certainly seems like a bug and could be causing other problems as well.

Comment 4

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

Jeff M and I discussed this offline. It is a thread issue. Bob, I'm happy to fix this, I already did some prototyping.

Comment 5

[Bob Owen (:bobowen)]

(In reply to Andrew Osmond [:aosmond] (he/him) from comment #4)

Jeff M and I discussed this offline. It is a thread issue. Bob, I'm happy to fix this, I already did some prototyping.

I think I have a patch.
I've made AddStoredObject virtual and added an override and also an array of pending deletions to the CanvasDrawEventRecorder.
It appends to this array instead of submitting to the main thread and then makes sure they are processed before the objects are added.

Comment 6

[Bob Owen (:bobowen)]

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

Comment 7

[Bob Owen (:bobowen)]

Attachment: Bug 1825169: Process off main thread deletions in DrawEventRecorderPrivate::AddStoredObject. r=aosmond!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ea78ac468b8
Process off main thread deletions in DrawEventRecorderPrivate::AddStoredObject. r=aosmond

Comment 9

[Cristina Horotan [:chorotan]]

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

Comment 10

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

The patch landed in nightly and beta is affected.
:bobowen, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox113 to wontfix.

For more information, please visit auto_nag documentation.

Comment 11

[Bob Owen (:bobowen)]

(In reply to Release mgmt bot [:suhaib / :marco/ :calixte] from comment #10)

The patch landed in nightly and beta is affected.
:bobowen, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox113 to wontfix.

For more information, please visit auto_nag documentation.

Given this fix involves new locking and this crash is not too common I'm inclined to let this ride the trains.
:aosmond - what do you think, not sure if the other problems that this hopefully fixes would warrant an uplift?

Comment 14

[Bob Owen (:bobowen)]

I think letting this roll out is the best policy at this stage.

Comment 15

[Bob Owen (:bobowen)]

Comment on attachment 9327513 [details]
Bug 1825169: Process off main thread deletions in DrawEventRecorderPrivate::AddStoredObject. r=aosmond!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Uplift requested for crash.
• User impact if declined: Tab crashes still experienced.
• Fix Landed on Version: 114
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Relatively simple fix that has already rolled out to release and requires no rebase.

Comment 16

[Donal Meehan [:dmeehan]]

Comment on attachment 9327513 [details]
Bug 1825169: Process off main thread deletions in DrawEventRecorderPrivate::AddStoredObject. r=aosmond!

Approved for 102.13esr.

Comment 17

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/23fb72af3558