1903387 - Crash in [@ mozilla::dom::quota::DirectoryLockImpl::~DirectoryLockImpl]

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, defect, P1)

Description

[Jan Varga [:janv]]

Crash report: https://crash-stats.mozilla.org/report/index/555a800f-980b-4923-84ec-0a0bf0240618

MOZ_CRASH Reason: MOZ_DIAGNOSTIC_ASSERT(!mRegistered)

Top 10 frames:

0  xul.dll  mozilla::dom::quota::DirectoryLockImpl::~DirectoryLockImpl()  dom/quota/DirectoryLockImpl.cpp:57
1  xul.dll  mozilla::dom::quota::DirectoryLockImpl::Release()  dom/quota/DirectoryLockImpl.h:179
2  xul.dll  mozilla::RefPtrTraits<mozilla::dom::quota::ClientDirectoryLock>::Release(mozi...  mfbt/RefPtr.h:49
2  xul.dll  RefPtr<mozilla::dom::quota::ClientDirectoryLock>::ConstRemovingRefPtrTraits<m...  mfbt/RefPtr.h:409
2  xul.dll  RefPtr<mozilla::dom::quota::ClientDirectoryLock>::~RefPtr()  mfbt/RefPtr.h:80
2  xul.dll  mozilla::detail::VariantImplementation<unsigned char, 1, RefPtr<mozilla::dom:...  mfbt/Variant.h:240
2  xul.dll  mozilla::detail::VariantImplementation<unsigned char, 0, mozilla::Nothing, Re...  mfbt/Variant.h:242
2  xul.dll  mozilla::Variant<mozilla::Nothing, RefPtr<mozilla::dom::quota::ClientDirector...  mfbt/Variant.h:688
2  xul.dll  mozilla::MozPromise<RefPtr<mozilla::dom::quota::ClientDirectoryLock>, nsresul...  xpcom/threads/MozPromise.h:181
2  xul.dll  mozilla::MozPromise<RefPtr<mozilla::dom::quota::ClientDirectoryLock>, nsresul...  xpcom/threads/MozPromise.h:1307

Comment 1

[Jan Varga [:janv]]

The full stack trace is a bit confusing, especially the part with FileDescriptor, but after a quick audit I found at least one place where we can hit the assertion and end up with the same top frames:

0:25.85 pid:24470 [24470] Assertion failure: !mRegistered, at /Users/jvarga/Projects/Mozilla14/src/dom/quota/DirectoryLockImpl.cpp:57
 0:25.85 pid:24470 #01: mozilla::dom::quota::DirectoryLockImpl::~DirectoryLockImpl() (/Users/jvarga/Projects/Mozilla14/src/dom/quota/DirectoryLockImpl.cpp:0)
 0:25.85 pid:24470 #02: mozilla::dom::quota::DirectoryLockImpl::Release() (/Users/jvarga/Projects/Mozilla14/src/dom/quota/DirectoryLockImpl.h:179)
 0:25.85 pid:24470 #03: mozilla::MozPromise<RefPtr<mozilla::dom::quota::ClientDirectoryLock>, nsresult, true>::~MozPromise() (/Users/jvarga/Projects/Mozilla14/obj-ff-nightly-debug-optimized/dist/include/mozilla/MozPromise.h:1307)
 0:25.85 pid:24470 #04: mozilla::MozPromise<RefPtr<mozilla::dom::quota::ClientDirectoryLock>, nsresult, true>::Private::~Private() (/Users/jvarga/Projects/Mozilla14/obj-ff-nightly-debug-optimized/dist/include/mozilla/MozPromise.h:257)
 0:25.85 pid:24470 #05: mozilla::MozPromise<RefPtr<mozilla::dom::quota::ClientDirectoryLock>, nsresult, true>::ThenValueBase::ResolveOrRejectRunnable::Run() (/Users/jvarga/Projects/Mozilla14/obj-ff-nightly-debug-optimized/dist/include/mozilla/MozPromise.h:491)
 0:25.85 pid:24470 #06: nsThread::ProcessNextEvent(bool, bool*) (/Users/jvarga/Projects/Mozilla14/src/xpcom/threads/nsThread.cpp:1199)
 0:25.85 pid:24470 #07: NS_ProcessNextEvent(nsIThread*, bool) (/Users/jvarga/Projects/Mozilla14/src/xpcom/threads/nsThreadUtils.cpp:480)
 0:25.85 pid:24470 #08: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) (/Users/jvarga/Projects/Mozilla14/src/ipc/glue/MessagePump.cpp:300)
 0:25.85 pid:24470 #09: MessageLoop::RunInternal() (/Users/jvarga/Projects/Mozilla14/src/ipc/chromium/src/base/message_loop.cc:0)
 0:25.85 pid:24470 #10: MessageLoop::Run() (/Users/jvarga/Projects/Mozilla14/src/ipc/chromium/src/base/message_loop.cc:346)
 0:25.85 pid:24470 #11: nsThread::ThreadFunc(void*) (/Users/jvarga/Projects/Mozilla14/src/xpcom/threads/nsThread.cpp:372)
 0:25.85 pid:24470 #12: _pt_root (/Users/jvarga/Projects/Mozilla14/src/nsprpub/pr/src/pthreads/ptthread.c:204)
 0:25.85 pid:24470 #13: _pthread_start (/usr/lib/system/libsystem_pthread.dylib + 0x6f94)

PrepareDatastoreOp::DirectoryLockAcquired does not always take ownership of the passed directory lock and that causes that the lock will get released without Drop being called prior that.

I'll submit a patch in a bit and will do more investigation.

Comment 2

[Jan Varga [:janv]]

Attachment: Bug 1903387 - Always take ownership of the passed directory lock in PrepareDatastoreOp::DirectoryLockAcquired; r=#dom-storage

If we don't always take ownership of the directory lock then it can happen that
the directory lock will get released without Drop being called prior that.
If we are shutting down or the actor is destroyed the directory lock will get
dropped and relased in PrepareDatastoreOp::Cleanup.

Comment 3

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

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

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4baefc2716d4
Always take ownership of the passed directory lock in PrepareDatastoreOp::DirectoryLockAcquired; r=dom-storage-reviewers,asuth

Comment 5

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/4baefc2716d4

Comment 6

[Jan Varga [:janv]]

It seems the pace of new crash reports has slowed down after landing https://phabricator.services.mozilla.com/D214168

I identified another place which needs to be fixed and is probably causing reports like this:
https://crash-stats.mozilla.org/report/index/b804d98f-17f8-4c46-a979-514510240619

Comment 7

[Jan Varga [:janv]]

Attachment: Bug 1903387 - Always check and eventually drop directory lock in Maintenance::Finish; r=#dom-storage

It can happen that DirectoryOpen call in Maintenance::DirectoryLockAcquired
fails or Maintenance::DirectoryWork method fails or
Maintenance::BeginDatabaseMaintenance method fails which results in
Maintenance::Finish being called with not null mDirectoryLock and that
the directory lock hasn't been dropped in Maintenance::BeginDatabaseMaintenance.
Maintenance::Finish should always check and eventually drop the directory lock
instead of relying on Maintenance::BeginDatabaseMaintenance to drop it.
We still want to drop it as soon as possible in
Maintenance::BeginDatabaseMaintenance so other storage operations are not
blocked by the directory lock covering all idb folders for all repositories.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe859802fa07
Always check and eventually drop directory lock in Maintenance::Finish; r=dom-storage-reviewers,jstutte

Comment 9

[Cristian Tuns]

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

Comment 10

[Jan Varga [:janv]]

This should be now fixed.

Comment 11

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

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

Comment 12

[Liz Henry (:lizzard)]

janv, there are a few crashes with this signature now showing up in Nightly 133. If you have time to take a look at the new reports, do you think it is worth filing a new bug or re-opening this one? Thanks!

Comment 13

[Jan Varga [:janv]]

(In reply to Liz Henry (:lizzard) (relman/hg->git project) from comment #12)

janv, there are a few crashes with this signature now showing up in Nightly 133. If you have time to take a look at the new reports, do you think it is worth filing a new bug or re-opening this one? Thanks!

There's already a new bug 1923056.
I have a fix, I'll be submitting it for a review soon.