1502841 - Crash in shutdownhang | RtlpWaitCouldDeadlock | mozilla::detail::MutexImpl::lock | `anonymous namespace'::DataStorageSharedThread::Initialize

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-34874eca-430c-456d-bfa1-328720181028.
=============================================================

Top 10 frames of crashing thread:

0 ntdll.dll KiFastSystemCallRet 
1 ntdll.dll ZwWaitForKeyedEvent 
2 ntdll.dll RtlpWaitCouldDeadlock 
3 mozglue.dll mozilla::detail::MutexImpl::lock mozglue/misc/Mutex_windows.cpp:27
4 xul.dll static nsresult `anonymous namespace'::DataStorageSharedThread::Initialize security/manager/ssl/DataStorage.cpp:86
5 xul.dll mozilla::DataStorage::Init security/manager/ssl/DataStorage.cpp:379
6 xul.dll mozilla::DataStorage::GetAllChildProcessData security/manager/ssl/DataStorage.cpp:286
7 xul.dll void mozilla::dom::ContentParent::InitInternal dom/ipc/ContentParent.cpp:2487
8 xul.dll bool mozilla::dom::ContentParent::LaunchSubprocess dom/ipc/ContentParent.cpp:2274
9 xul.dll mozilla::dom::ContentParent::GetNewOrUsedBrowserProcess dom/ipc/ContentParent.cpp:888

=============================================================

shutdownhangs with this signature or similar start to show up with firefox 63 on windows in low-to-mid volume, likely related to the changes from bug 1470918.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: bug 1502841 - fix a deadlock by not holding the DataStorageSharedThread lock while shutting the thread down r?jcj

Judging by some stack traces we've received in crash reports, while shutting
down the DataStorageSharedThread, it is possible to process an event on that
thread that causes an attempt to re-initialize DataStorage. This wouldn't be a
problem because we have a shutdown sentinel boolean and we exit early if it is
true. However, checking the boolean involves acquiring the static lock for the
thread, which means we can't be holding the lock while we're shutting down the
thread.

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a41d63e2f67a38447a3b9bdc01d5aed37548ee26

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c3dd82a036ef
fix a deadlock by not holding the DataStorageSharedThread lock while shutting the thread down r=jcj

Comment 4

[Cristina Coroiu [:ccoroiu]]

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

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Given the crash reports we've seen so far, I don't expect we're going to get any validation of this fix's effectiveness until it makes it to Beta. It grafts cleanly, so please nominate for approval when you get a chance.

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: bug 1502841 - follow-up to ensure reentering DataStorageSharedThread::Shutdown() doesn't cause problems r?jcj

We shouldn't reenter DataStorageSharedThread::Shutdown(), but it may be
possible. To guard against potentially attempting to shut down the shared thread
more than once, we can check gDataStorageSharedThreadShutDown first.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler]]

I noticed a potential issue with the first patch, so let's fix that and then uplift.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f38aa27974ee
follow-up to ensure reentering DataStorageSharedThread::Shutdown() doesn't cause problems r=jcj

Comment 9

[Andreea Pavel [:apavel]]

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

Comment 10

[[:philipp]]

is everything in place for an uplift to beta now?

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 9024601 [details]
bug 1502841 - fix a deadlock by not holding the DataStorageSharedThread lock while shutting the thread down r?jcj

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1470918

User impact if declined: Potential shutdown hangs

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: the other patch in this bug

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The fix is small, localized, and relatively conservative.

String changes made/needed: none

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 9025427 [details]
bug 1502841 - follow-up to ensure reentering DataStorageSharedThread::Shutdown() doesn't cause problems r?jcj

(see comment 11)

Comment 13

[Julien Cristau [:jcristau]]

Comment on attachment 9024601 [details]
bug 1502841 - fix a deadlock by not holding the DataStorageSharedThread lock while shutting the thread down r?jcj

PSM deadlock fix, approved for 64.0b14

Comment 14

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

https://hg.mozilla.org/releases/mozilla-beta/rev/ce690bc50558
https://hg.mozilla.org/releases/mozilla-beta/rev/140ce47ccf2b