Closed Bug 1330747 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: DOM: Service Workers, defect)

Unspecified
Windows 8
defect
Not set
critical

Tracking

()

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

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

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().
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
Attachment #8826308 - Flags: review?(bugmail)
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!
Attachment #8826308 - Flags: review?(bugmail) → review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c24603332545
Always nullptr check return value from ServiceWorkerManager::GetInstance(). r=asuth
https://hg.mozilla.org/mozilla-central/rev/c24603332545
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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
Attachment #8826308 - Flags: approval-mozilla-beta?
Attachment #8826308 - Flags: approval-mozilla-aurora?
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.
Attachment #8826308 - Flags: approval-mozilla-beta?
Attachment #8826308 - Flags: approval-mozilla-beta+
Attachment #8826308 - Flags: approval-mozilla-aurora?
Attachment #8826308 - Flags: approval-mozilla-aurora+
Thanks.  I'll uplift this and the other bugs since it might need some minor rebasing.
Crash Signature: [@ RefPtr<T>::RefPtr<T> | mozilla::dom::workers::ServiceWorkerManager::InvalidateServiceWorkerRegistrationWorker] → [@ RefPtr<T>::RefPtr<T> | mozilla::dom::workers::ServiceWorkerManager::InvalidateServiceWorkerRegistrationWorker] [@ @0x0 | mozilla::dom::workers::ServiceWorkerManager::InterceptionReleaseHandle::~InterceptionReleaseHandle ]
Crash Signature: [@ RefPtr<T>::RefPtr<T> | mozilla::dom::workers::ServiceWorkerManager::InvalidateServiceWorkerRegistrationWorker] [@ @0x0 | mozilla::dom::workers::ServiceWorkerManager::InterceptionReleaseHandle::~InterceptionReleaseHandle ] → [@ RefPtr<T>::RefPtr<T> | mozilla::dom::workers::ServiceWorkerManager::InvalidateServiceWorkerRegistrationWorker] [@ @0x0 | mozilla::dom::workers::ServiceWorkerManager::InterceptionReleaseHandle::~InterceptionReleaseHandle ] [@ mozilla::dom::workers::Se…
Crash Signature: mozilla::dom::workers::ServiceWorkerManager::InvalidateServiceWorkerRegistrationWorker ] → mozilla::dom::workers::ServiceWorkerManager::InvalidateServiceWorkerRegistrationWorker ] [@ mozilla::dom::workers::ServiceWorkerManager::ReportToAllClients ]
You need to log in before you can comment on or make changes to this bug.