1330747 - Crash in RefPtr<T>::RefPtr<T> | mozilla::dom::workers::ServiceWorkerManager::InvalidateServiceWorkerRegistrationWorker

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-2ace578a-5922-4792-81f0-eab3a2170112.
=============================================================

I believe the browser is shutting down as we try to update a registration attribute here.  We need to accomodate a nullptr SWM in AsyncUpdateRegistrationStateProperties().

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Always nullptr check return value from ServiceWorkerManager::GetInstance(). r=asuth

I think its pretty clear browser shutdown can come at just about any time and as a result ServiceWorkerManager::GetInstance() can return nullptr.  We should be checking the pointer before using it in all cases.  If some code wants to guarantee it cannot hit shutdown, then it should hold its own ref to the SWM instead of using GetInstance().

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

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8826308 [details] [diff] [review]
Always nullptr check return value from ServiceWorkerManager::GetInstance(). r=asuth

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +414,5 @@
>      AssertIsOnMainThread();
>  
>      RefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
> +    if (swm) {
> +      swm->PropagateUnregister(mPrincipal, mCallback, mScope);

Note: I observe that you're removing the rv handling here and affirm that we don't actually care about the rv here and that the rest of the runnables like this one never paid attention to the rv to begin with.  (In particular these are all processed via ProcessNextEvent() and it does not care what Run() returns.)

::: dom/workers/ServiceWorkerUpdateJob.cpp
@@ +485,4 @@
>  }
>  
>  void
> +ServiceWorkerUpdateJob::Install(ServiceWorkerManager* aSWM)

off-topic argument name enjoyment: What a nice day for aSWM!

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c24603332545
Always nullptr check return value from ServiceWorkerManager::GetInstance(). r=asuth

Comment 4

[Carsten Book [:Tomcat]]

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

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8826308 [details] [diff] [review]
Always nullptr check return value from ServiceWorkerManager::GetInstance(). r=asuth

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 has a large number of automated tests.  They don't regularly trigger all the possible shutdown crashes, though.
[Has the fix been verified in Nightly?]:  It has landed on nightly, but the shutdown crashes are very timing dependent so they have not been manually verified.
[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?]: While the patch is a bit large, it basically just adds a nullptr check where we call ServiceWorkerManager::GetInstance().  These checks have very little chance of causing regressions.
[String changes made/needed]: None

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8826308 [details] [diff] [review]
Always nullptr check return value from ServiceWorkerManager::GetInstance(). r=asuth

Crash fix, may be a bit risky to put into beta at the last minute, but I'd like it for the 51 RC build.  Along with some other patches from other bugs to fix Service Worker issues.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Thanks.  I'll uplift this and the other bugs since it might need some minor rebasing.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Aurora landing required some minor rebasing:

https://hg.mozilla.org/releases/mozilla-aurora/rev/be9be2c13b4dc9038b331a91f923a4658014bae4

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Slightly more rebasing for beta:

https://hg.mozilla.org/releases/mozilla-beta/rev/7d9973f1bba7