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

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[Bob Owen (:bobowen)]

+++ This bug was initially created as a clone of Bug #1825169 +++

Cloning because this crash is still happening, but the fix from bug 1825169 was a genuine issue, fixed other bugs, had follow-ups and was uplifted, so I think re-opening will cause more confusion.

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

[Bob Owen (:bobowen)]

I've spotted another issue, hopefully this is the real one.
We don't process pending deletions in HasStoredObject.

I could just add a call in there, but it will result in multiple calls to ProcessPendingDeletions (which involves locking) in some code paths, so a little bit of a refactor is needed to remove that.

Comment 2

[Bob Owen (:bobowen)]

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

Comment 3

[Bob Owen (:bobowen)]

Attachment: Bug 1838792: Add TryAddStoredObject to DrawEventRecorderPrivate and use in DrawTargetRecording. r=aosmond!

This means we can do a combination of HasStoredObject and AddStoredObject with
only one call to ProcessPendingDeletions, which uses locking.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f0a3370d3d44
Add TryAddStoredObject to DrawEventRecorderPrivate and use in DrawTargetRecording. r=aosmond

Comment 5

[Natalia Csoregi [:nataliaCs]]

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

Comment 6

[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-firefox115 to wontfix.

For more information, please visit BugBot documentation.

Comment 7

[Bob Owen (:bobowen)]

(In reply to BugBot [:suhaib / :marco/ :calixte] from comment #6)

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-firefox115 to wontfix.

For more information, please visit BugBot documentation.

I think we're too close to release to get it in 115 but maybe it could go in a dot release.
Either way I'll uplift to ESR115 once we've seen the impact.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Is this ready for an ESR115 uplift request?

Comment 9

[Bob Owen (:bobowen)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #8)

Is this ready for an ESR115 uplift request?

Yes, looks like this has solved the problem.

Comment 10

[Bob Owen (:bobowen)]

Comment on attachment 9340918 [details]
Bug 1838792: Add TryAddStoredObject to DrawEventRecorderPrivate and use in DrawTargetRecording. r=aosmond!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Medium crash rate that was made worse just before start of ESR115.
• User impact if declined: Users still experience tab crashes with some sites using canvas.
• Fix Landed on Version: 116
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Not putting low because the changes required some refactoring, so they are fairly simple but not trivial.

Comment 11

[Dianna Smith [:diannaS]]

Comment on attachment 9340918 [details]
Bug 1838792: Add TryAddStoredObject to DrawEventRecorderPrivate and use in DrawTargetRecording. r=aosmond!

Approved for 115.1esr

Comment 12

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/5a6e1e04029a

Comment 13

[Ryan VanderMeulen [:RyanVM]]

This seems to be spiking like crazy on Release 115.0.2 in the last few days. I guess it's not surprising given the lack of backport there, but just FYI that we probably should keep an eye on this next cycle still to verify that the crash rate looks good once this fix goes out.

Comment 14

[Bob Owen (:bobowen)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #13)

This seems to be spiking like crazy on Release 115.0.2 in the last few days. I guess it's not surprising given the lack of backport there, but just FYI that we probably should keep an eye on this next cycle still to verify that the crash rate looks good once this fix goes out.

No crashes for a week, so looks like I finally fixed the problem.