Closed Bug 1408734 Opened 7 years ago Closed 7 years ago

Crash in mozilla::dom::ServiceWorkerRegistrationMainThread::UpdateViaCache const

Categories

(Core :: DOM: Service Workers, defect, P2)

Unspecified
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 - fixed
firefox58 + fixed

People

(Reporter: timdream, Assigned: edenchuang)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 1 obsolete file)

[Tracking Requested - why for this release]: Reproducible crash.

This bug was filed from the Socorro interface and is 
report bp-685a1b9a-0a47-498c-9de8-582230171015.
=============================================================

Reproducible crash of content process.

1. Go to an website with a service worker
2. Run the following reduced test case in DevTools

navigator.serviceWorker.getRegistration('/')
  .then(reg => Promise.all([reg, reg.unregister()]))
  .then(([reg,]) => console.log(reg));

3. Crash in Fx57 and Nightly!

Note: crash won't reproduce without the console.log().
We need to handle the case where a Dom registration object is alive, but the backing registration has been removed.  Eden, do you think you could convert the code triggering this crash to do a runtime check instead of an assert?

Also, should be easy to turn the code in comment 0 into a mochitest regression testing.

Thanks!
Flags: needinfo?(echuang)
Yes, I can convert the code to do a runtime checking and also add a mochitest for the comment 0 case.

Could you help to review the patch? or suggest a reviewer for me, if you are busy on other things.
Assignee: nobody → echuang
Status: NEW → ASSIGNED
Flags: needinfo?(echuang) → needinfo?(bkelly)
Sure.  Feel free to flag me for review.

And now that I'm back at my laptop I can link the assertion I was referencing in comment:

https://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerRegistration.cpp#157

I guess if those conditions are false we should throw NS_ERROR_DOM_INVALID_STATE_ERR.  To do that you'll need to add [Throws] to the webidl here so you get an ErrorResult passed in:

https://searchfox.org/mozilla-central/source/dom/webidl/ServiceWorkerRegistration.webidl#20

Thanks!
Flags: needinfo?(bkelly)
Priority: -- → P2
Looks like an uncommon crash, though since it is reproducible and may be a new regression, maybe you want to request uplift to 57 if you land a fix for 58.
Sorry for late response, since I just finished my WebPayment work. Now I am going to start to work on ServiceWorker.

After study the reproduce case, I found that if we access ServiceWorkerRegistration::updateViaCache after the unregister() finishes, we meet the assertion. So I will create the test by this reduced case.

Besides throwing the InvalidStateError, I think ServiceWorkerRegistration::updateViaCache should return ServiceWorkerUpdateViaCache::None under the situation.
This patch implements followings                                             

1. Adding extended attribute [Throws] on ServiceWorkerRegistration::updateViaCache attribute in ServiceWorkerRegistration.webidl.                                                
2. Instead of calling MOZ_ASSERT, returning InvalidStateError when fail to get the registration in ServiceWorkerRegistration::GetUpdateViaCache().   
3. Adding a new mochitest test_bug1408734.html to reproduce the bug introduced by accessing ServiceWorkerRegistration::updateViaCache after unregister() finishes.
Attachment #8922241 - Flags: review?(bkelly)
Comment on attachment 8922241 [details] [diff] [review]
Bug 1408734 - Returning InvalidStateError when accessing ServiceWorkerRegistration::updateViaCache after unregister. r?bkelly

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

Thanks!

::: dom/workers/ServiceWorkerRegistration.cpp
@@ +154,5 @@
>      nsCOMPtr<nsIServiceWorkerRegistrationInfo> registration;
>      nsresult rv = swm->GetRegistrationByPrincipal(doc->NodePrincipal(), mScope,
>                                                    getter_AddRefs(registration));
> +
> +    if (!(NS_SUCCEEDED(rv) && registration)) {

nit: I think this would be easier to read as `NS_FAILED(rv) || !registration`.

@@ +155,5 @@
>      nsresult rv = swm->GetRegistrationByPrincipal(doc->NodePrincipal(), mScope,
>                                                    getter_AddRefs(registration));
> +
> +    if (!(NS_SUCCEEDED(rv) && registration)) {
> +      aRv = NS_ERROR_DOM_INVALID_STATE_ERR;

Please add a comment that this is not strictly correct.  We should probably store the `updateViaCache` value on the ServiceWorkerRegistration object and update it when necessary.  We don't have a good way to reach all ServiceWorkerRegistration objects from the ServiceWorkerRegistratinInfo right now, though.  So this is a short term fix to avoid crashing.

::: dom/workers/test/serviceworkers/test_bug1408734.html
@@ +52,5 @@
> +      ok(false,
> +         "Expected InvalidStateError when accessing registration.updateViaCache after unregister()");
> +    }
> +  } catch (err) {
> +    is(err.name, "InvalidStateError", "Expected InvalidStateError.");

Maybe add a comment that we really care that we don't crash.  In the future we will fix updateViaCache to be accessible after unregister().
Attachment #8922241 - Flags: review?(bkelly) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac63e59b68b4
Return InvalidStateError when accessing ServiceWorkerRegistration::updateViaCache after unregister. r=bkelly
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ac63e59b68b4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Actually, I think we should fix this for FF57.  Its a crash that can be easily reproduced by script and the fix is not complicated.

I'll do the uplift flag request.
Comment on attachment 8922600 [details] [diff] [review]
Bug 1408734 - Returning InvalidStateError when accessing ServiceWorkerRegistration::updateViaCache after unregister. r=bkelly

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1353636
[User impact if declined]: If a page unregisters a service worker and then tries to access the updateViaCache attribute the browser will crash.
[Is this code covered by automated tests?]: Yes, included in this test.
[Has the fix been verified in Nightly?]: Its been in nightly for a few days now.
[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 simply checks for nullptr and throws a JS exception in that case instead of crashing.
[String changes made/needed]: None
Attachment #8922600 - Flags: approval-mozilla-beta?
Note, the dependent bug 1413056 is not necessary for the uplift.
Do we really know this won't affact Fx57 or we simply don't care at this point?
Tim, if you are asking bug 1413056, it won't affect Fx57, because the patch only updates the test code to make sure the behavior is correct.
I am sorry. I was asking about this bug: it was marked as unaffected on Fx56 and I am not sure if that's correct.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #17)
> I am sorry. I was asking about this bug: it was marked as unaffected on Fx56
> and I am not sure if that's correct.

The `updateViaCache` property was added in FF57.  See bug 1353636.

Bug 1413056 is not necessary to uplift because the test only fails with some newer code I am landing in FF58 that makes us check some service worker spec constraints more strictly.
Blocks: 1353636
Comment on attachment 8922600 [details] [diff] [review]
Bug 1408734 - Returning InvalidStateError when accessing ServiceWorkerRegistration::updateViaCache after unregister. r=bkelly

Recent regression in 57, Beta57+
Attachment #8922600 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: