Closed Bug 1806751 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: XPCOM, defect)

Unspecified
Android
defect

Tracking

()

RESOLVED FIXED
111 Branch
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)

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

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

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

Flags: needinfo?(nika)

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.

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

(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.

Assignee: nobody → nika
Severity: -- → S2
Flags: needinfo?(nika)
Regressed by: 1799222

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

Summary: Crash in [@ mozilla::ThreadEventTarget::Dispatch] → Crash in [@ mozilla::ThreadEventTarget::Dispatch] from storage::Service::Observe

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.

Keywords: topcrash

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

Flags: needinfo?(nika)

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.

Pushed by nlayzell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d7c4d4371b6c Perform indexeddb database maintenance on a TaskQueue, r=asuth,janv,dom-storage-reviewers
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 111 Branch
Flags: needinfo?(nika)

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.

Flags: needinfo?(nika)

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
Flags: needinfo?(nika)
Attachment #9311859 - Flags: approval-mozilla-beta?

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

Approved for 110 beta 3, thanks

Attachment #9311859 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: