1806751 - Crash in [@ mozilla::ThreadEventTarget::Dispatch] from storage::Service::Observe

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/3d672fc1-1127-40de-9fa8-95f3f0221220

MOZ_CRASH Reason: MOZ_DIAGNOSTIC_ASSERT(false) (Attempt to dispatch to thread which does not usually process dispatched runnables until shutdown)

Top 10 frames of crashing thread:

0  libxul.so  mozilla::ThreadEventTarget::Dispatch  xpcom/threads/ThreadEventTarget.cpp
0  libxul.so  nsThread::Dispatch  xpcom/threads/nsThread.cpp:676
1  libxul.so  nsIEventTarget::Dispatch  dist/include/nsIEventTarget.h:42
1  libxul.so  mozilla::storage::Service::minimizeMemory  storage/mozStorageService.cpp:303
1  libxul.so  mozilla::storage::Service::Observe  storage/mozStorageService.cpp:683
2  libxul.so  {virtual override thunk}  storage/mozStorageService.cpp
3  libxul.so  nsObserverList::NotifyObservers  xpcom/ds/nsObserverList.cpp:70
3  libxul.so  nsObserverService::NotifyObservers  xpcom/ds/nsObserverService.cpp:291
4  libxul.so  GeckoAppShellSupport::NotifyObservers  widget/android/nsAppShell.cpp:248
5  libxul.so  mozilla::jni::detail::ProxyNativeCall<GeckoAppShellSupport, mozilla::java::GeckoAppShell, true, false, mozilla::jni::StringParam const&, mozilla::jni::StringParam const&>::Call<true, false,  const  widget/android/jni/Natives.h:1200

Comment 1

[Andrew McCreight [:mccr8]]

Nika, could this be a regression from bug 1799222? Thanks.

Comment 2

[Andrew McCreight [:mccr8]]

I dunno if this is a dupe of bug 1806332 or something new.

Comment 3

[Andrew McCreight [:mccr8]]

In addition to the storage::Service::Observe, there's a different looking one happening on a CanvasWorkers thread for the "GradientCache::DestroyRemovedGradientStops" runnable, involving some graphics code: bp-8227801f-9f65-4c99-8bf6-676de0221219

I don't know if that should get filed separately.

Comment 4

[Gabriele Svelto [:gsvelto] (PTO)]

The first three functions in both crashes look like they could be removed from the signature to tell crashes apart.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

(In reply to Andrew McCreight [:mccr8] from comment #2)

I dunno if this is a dupe of bug 1806332 or something new.

This is a crash on a different dispatch not coming from the same code, so I would say it is not the same bug.

The main crash appears to becoming from a crash here: https://searchfox.org/mozilla-central/rev/57527d50ef5d3df412caa5d99536f0709399be6f/storage/mozStorageService.cpp#303. This is trying to dispatch to the event target which the storage::Connection was opened on, which is fetched as the current serial event target here: https://searchfox.org/mozilla-central/rev/57527d50ef5d3df412caa5d99536f0709399be6f/storage/mozStorageConnection.cpp#431

It appears that in some case we will attempt to create a storage connection when on a threadpool or similar thread, which will then note down the thread it was created from, and attempt to dispatch back to it in the future.

I wonder a bit if we should make GetCurrentSerialEventTarget() assert when called on a threadpool with no TaskQueue present or something like that so that we can catch errors like these closer to the source?

Taking for now to at least investigate it more.

(In reply to Andrew McCreight [:mccr8] from comment #3)

In addition to the storage::Service::Observe, there's a different looking one happening on a CanvasWorkers thread for the "GradientCache::DestroyRemovedGradientStops" runnable, involving some graphics code: bp-8227801f-9f65-4c99-8bf6-676de0221219

I don't know if that should get filed separately.

This should be filed separately. It looks like there is some code which uses NS_DispatchToCurrentThread when on a threadpool, which is definitely broken, and is probably best fixed by someone on the graphics team figuring out what the actual behaviour it should have is.

Comment 6

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

Set release status flags based on info from the regressing bug 1799222

Comment 7

[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 auto_nag documentation.

Comment 8

[Pascal Chevrel:pascalc (PTO until April 26)]

Nika, the volume of crashes on Android Nightly is important, will you have a fix before the Monday merge or should we plan an uplift in beta? Thanks

Comment 9

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1806751 - Perform indexeddb database maintenance on a TaskQueue, r=asuth!

After adding some assertions to GetCurrentSerialEventTarget and running tests,
I was able to find this caller which could lead to the crashes here. By running
database maintenance on a TaskQueue the captured event target will be one which
accepts and processes dispatches.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7c4d4371b6c
Perform indexeddb database maintenance on a TaskQueue, r=asuth,janv,dom-storage-reviewers

Comment 11

[Marian-Vasile Laza]

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

Comment 12

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

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

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

For more information, please visit auto_nag documentation.

Comment 13

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 9311859 [details]
Bug 1806751 - Perform indexeddb database maintenance on a TaskQueue, r=asuth!

Beta/Release Uplift Approval Request

• User impact if declined: Potential unnecessary crashes when under memory pressure
• Is this code covered by automated tests?: No
• 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): Small change with little to no semantic impact, which allows other edge-case functionality to function correctly.
• String changes made/needed: None
• Is Android affected?: Yes

Comment 14

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9311859 [details]
Bug 1806751 - Perform indexeddb database maintenance on a TaskQueue, r=asuth!

Approved for 110 beta 3, thanks

Comment 15

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/56646818f453