Closed Bug 1330984 Opened 7 years ago Closed 7 years ago

Crash in mozilla::dom::ServiceWorkerRegistrar::ReadData


(Core :: DOM: Service Workers, defect)

Windows 7
Not set



Tracking Status
firefox-esr45 --- disabled
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed


(Reporter: bkelly, Assigned: bkelly)



(Keywords: crash)

Crash Data


(1 file)

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.
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.
Attachment #8826745 - Flags: review?(amarchesini)
Attachment #8826745 - Flags: review?(amarchesini) → review+
Pushed by
Handle mProfileDir being cleared in ServiceWorkerRegistrar. r=baku
This is low enough frequency I don't think its worth uplifting after beta RC merge.  Lets just uplift to aurora.
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
Attachment #8826745 - Flags: approval-mozilla-aurora?
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8826745 [details] [diff] [review]
Handle mProfileDir being cleared in ServiceWorkerRegistrar. r=baku

shutdown crash fix with service workers, aurora52+
Attachment #8826745 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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));

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]
Depends on: 1331949
You need to log in before you can comment on or make changes to this bug.