1682639 - Crash in [@ mozilla::dom::(anonymous namespace)::PrepareDatastoreOp::NestedRun]

Status: RESOLVED FIXED

(Core :: Storage: localStorage & sessionStorage, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

[Tracking Requested - why for this release]:

Looks like it will be frequent for Firefox 85 beta 1. Failed once for Nightly 85 (build ID 20201203094726).

Crash report: https://crash-stats.mozilla.org/report/index/ded2cacc-9b68-449e-856b-c70370201215

MOZ_CRASH Reason: MOZ_DIAGNOSTIC_ASSERT(isSome())

Top 10 frames of crashing thread:

0 XUL mozilla::dom:: dom/localstorage/ActorsParent.cpp:7829
1 XUL mozilla::dom:: dom/localstorage/ActorsParent.cpp:6774
2 XUL nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1200
3 XUL mozilla::ipc::MessagePumpForNonMainThreads::Run ipc/glue/MessagePump.cpp:302
4 XUL MessageLoop::Run ipc/chromium/src/base/message_loop.cc:309
5 XUL nsThread::ThreadFunc xpcom/threads/nsThread.cpp:441
6 libnss3.dylib _pt_root nsprpub/pr/src/pthreads/ptthread.c:201
7 libsystem_pthread.dylib _pthread_body 
8 libsystem_pthread.dylib _pthread_start 
9 libsystem_pthread.dylib thread_start 

Comment 1

[Julien Cristau [:jcristau] (back August 18)]

#1 top browser crash so far in early 85.0b1 rollout.

Comment 3

[Simon Giesecke [:sg] [he/him]]

Stack trace with inline frames:

[Inline Frame] xul.dll!mozilla::Maybe<unsigned long long>::value() Line 771
	at /builds/worker/workspace/obj-build/dist/include/mozilla/Maybe.h(771)
[Inline Frame] xul.dll!mozilla::dom::`anonymous namespace'::PrepareDatastoreOp::DatabaseWork() Line 7412
	at /builds/worker/checkouts/gecko/dom/localstorage/ActorsParent.cpp(7412)
xul.dll!mozilla::dom::`anonymous namespace'::PrepareDatastoreOp::NestedRun() Line 7837
	at /builds/worker/checkouts/gecko/dom/localstorage/ActorsParent.cpp(7837)
xul.dll!mozilla::dom::`anonymous namespace'::LSRequestBase::Run() Line 6774
	at /builds/worker/checkouts/gecko/dom/localstorage/ActorsParent.cpp(6774)
xul.dll!nsThread::ProcessNextEvent(bool aMayWait, bool * aResult=0x000000dcf279f6c0) Line 1205
	at /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp(1205)
[Inline Frame] xul.dll!NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait) Line 548
	at /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp(548)
xul.dll!mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate * aDelegate=0x000000dcf279f810) Line 302
	at /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp(302)
[Inline Frame] xul.dll!MessageLoop::RunInternal() Line 334
	at /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc(334)
xul.dll!MessageLoop::RunHandler() Line 328
	at /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc(328)
xul.dll!MessageLoop::Run() Line 310
	at /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc(310)
xul.dll!nsThread::ThreadFunc(void * aArg=0x000000dcf251f418) Line 443
	at /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp(443)
nss3.dll!_PR_NativeRunThread(void * arg=0x00000259d3b5a800) Line 421
	at /builds/worker/checkouts/gecko/nsprpub/pr/src/threads/combined/pruthr.c(421)
nss3.dll!pr_root(void * arg) Line 140
	at /builds/worker/checkouts/gecko/nsprpub/pr/src/md/windows/w95thred.c(140)

So the crash is triggered from https://searchfox.org/mozilla-central/rev/8883276967d39918e2ce64e873afdd432fb406ca/dom/localstorage/ActorsParent.cpp#7408 and is likely to be a regression from Bug 1671369, specifically https://hg.mozilla.org/mozilla-central/rev/868577854548c7442c15ab9b1ad1213af187e01e

Comment 4

[Simon Giesecke [:sg] [he/him]]

Apparently, we are in a situation where alreadyExisted but !hasUsage. In a debug build, the assertion at https://searchfox.org/mozilla-central/rev/8883276967d39918e2ce64e873afdd432fb406ca/dom/localstorage/ActorsParent.cpp#74087 would have failed (MOZ_ASSERT(hasUsage);) in this situation.

Before the patch this would have silently worked in a non-debug build, but it would have assigned the undefined value of the unitialized usage local variable to mUsage, with unclear follow-up effects.

A quick fix for this is to change mUsage = usageInfo.DatabaseUsage().value(); to mUsage = usageInfo.DatabaseUsage().valueOr(0);.

But eventually, either the assertion should be removed because it's not necessarily true, or the logic leading to this situation must be fixed.

Jan, do you see what's the right thing to do here? Should we land a quick fix as a workaround to remove the beta top blocker?

Comment 5

[Julien Cristau [:jcristau] (back August 18)]

FWIW I'd like to get a fix or backout in beta today or tomorrow, so we don't leave this crash over the holiday break.

Comment 6

[Jan Varga [:janv]]

(In reply to Simon Giesecke [:sg] [he/him] from comment #4)

A quick fix for this is to change mUsage = usageInfo.DatabaseUsage().value(); to mUsage = usageInfo.DatabaseUsage().valueOr(0);.

valueOr(0) for now is ok

But eventually, either the assertion should be removed because it's not necessarily true, or the logic leading to this situation must be fixed.

Yeah, I remember there was a problem with this, it's probably an inconsistency between the data on disk on the cached information about that.

Comment 7

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1682639 - Fix diagnostic assertion failure in PrepareDatastoreOp::DatabaseWork. r=janv,#dom-workers-and-storage

Comment 8

[Simon Giesecke [:sg] [he/him]]

Comment on attachment 9193707 [details]
Bug 1682639 - Fix diagnostic assertion failure in PrepareDatastoreOp::DatabaseWork. r=janv,#dom-workers-and-storage

Beta/Release Uplift Approval Request

• User impact if declined: Exposure to top-crashing diagnostic assertion failure.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• 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): While this situation is not covered by automated tests, the change is minimal and only replaces a diagnostic assertion failure by a defined 0 value.
• String changes made/needed:

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7b8c33214d9b
Fix diagnostic assertion failure in PrepareDatastoreOp::DatabaseWork. r=janv,dom-workers-and-storage-reviewers

Comment 10

[Julien Cristau [:jcristau] (back August 18)]

Comment on attachment 9193707 [details]
Bug 1682639 - Fix diagnostic assertion failure in PrepareDatastoreOp::DatabaseWork. r=janv,#dom-workers-and-storage

approved for 85.0b3

Thanks Simon and Jan!

Comment 11

[Julien Cristau [:jcristau] (back August 18)]

https://hg.mozilla.org/releases/mozilla-beta/rev/4bf49331ca74

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7b8c33214d9b