1330984 - Crash in mozilla::dom::ServiceWorkerRegistrar::ReadData

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

This bug was filed from the Socorro interface and is 
report bp-f183b6a5-067f-45c0-bf2c-c5ef62170112.
=============================================================

I believe this is happening because shutdown is starting after the LoadData() runnable is dispatched, but before it runs.  So the NS_GetSpecialDirectory() call is failing.  This in turn zero's out the mProfileDir due to getter_AddRef() behavior.

Also, I think its probably just unsafe to reference mProfileDir across threads like this.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Handle mProfileDir being cleared in ServiceWorkerRegistrar. r=baku

Andrea, this patch modifies ServiceWorkerRegistrar to protect mProfileDir since its accessed on multiple threads.

I believe the cross-thread access is a problem during shutdown when getter_AddRefs() clears mProfileDir in ProfileStopped().  It may or may not re-populate it with the profile dir again.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d794907ebb15
Handle mProfileDir being cleared in ServiceWorkerRegistrar. r=baku

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

This is low enough frequency I don't think its worth uplifting after beta RC merge.  Lets just uplift to aurora.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8826745 [details] [diff] [review]
Handle mProfileDir being cleared in ServiceWorkerRegistrar. r=baku

Approval Request Comment
[Feature/Bug causing the regression]: Service workers
[User impact if declined]: Shutdown crashes
[Is this code covered by automated tests?]: Service workers are covered in automated tests, but they don't catch this corner case.
[Has the fix been verified in Nightly?]: Its a very racy crash, so its hard to manully verify.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Minimal risk
[Why is the change risky/not risky?]: This patch basically provides some additional nullptr checking and mutex locking around the pointer.  It should have very load chance of regression.
[String changes made/needed]: None

Comment 5

[Phil Ringnalda (:philor)]

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

Comment 6

[Julien Cristau [:jcristau]]

Comment on attachment 8826745 [details] [diff] [review]
Handle mProfileDir being cleared in ServiceWorkerRegistrar. r=baku

shutdown crash fix with service workers, aurora52+

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/412036d99f78afc1e68c1d7088a3ce1fa275405e

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8826745 [details] [diff] [review]
Handle mProfileDir being cleared in ServiceWorkerRegistrar. r=baku

Review of attachment 8826745 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/test/gtest/TestReadWrite.cpp
@@ +25,5 @@
>    ServiceWorkerRegistrarTest()
>    {
>      nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR,
>                                         getter_AddRefs(mProfileDir));
> +    MOZ_DIAGNOSTIC_ASSERT(NS_SUCCEEDED(rv));

This will break when reaching beta, with:

/home/worker/workspace/build/src/dom/workers/test/gtest/TestReadWrite.cpp:27:14: error: unused variable 'rv' [-Werror,-Wunused-variable]