Crash in [@ mozilla::ThreadEventTarget::Dispatch] from storage::Service::Observe
Categories
(Core :: XPCOM, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr102 | --- | unaffected |
firefox108 | --- | unaffected |
firefox109 | --- | unaffected |
firefox110 | + | fixed |
firefox111 | --- | fixed |
People
(Reporter: mccr8, Assigned: nika)
References
(Regression)
Details
(Keywords: crash, regression, topcrash)
Crash Data
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details |
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
Reporter | ||
Comment 1•2 years ago
|
||
Nika, could this be a regression from bug 1799222? Thanks.
Reporter | ||
Comment 2•2 years ago
|
||
I dunno if this is a dupe of bug 1806332 or something new.
Reporter | ||
Comment 3•2 years ago
|
||
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•2 years ago
|
||
The first three functions in both crashes look like they could be removed from the signature to tell crashes apart.
Assignee | ||
Comment 5•2 years ago
|
||
(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•2 years ago
|
||
Set release status flags based on info from the regressing bug 1799222
Reporter | ||
Updated•2 years ago
|
Reporter | ||
Updated•2 years ago
|
Comment 7•2 years ago
|
||
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.
Updated•2 years ago
|
Comment 8•2 years ago
|
||
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
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 9•2 years ago
|
||
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•2 years ago
|
||
Comment 11•2 years ago
|
||
bugherder |
Updated•2 years ago
|
Comment 12•2 years ago
|
||
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
towontfix
.
For more information, please visit auto_nag documentation.
Assignee | ||
Comment 13•2 years ago
|
||
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•2 years ago
|
||
Comment on attachment 9311859 [details]
Bug 1806751 - Perform indexeddb database maintenance on a TaskQueue, r=asuth!
Approved for 110 beta 3, thanks
Comment 15•2 years ago
|
||
bugherder uplift |
Description
•