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

RESOLVED FIXED in Firefox 57

Status

()

defect
P2
critical
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: timdream, Assigned: edenchuang)

Tracking

(Blocks 1 bug, {crash})

Trunk
mozilla58
Unspecified
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 unaffected, firefox57- fixed, firefox58+ fixed)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

[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)
Assignee

Comment 2

2 years ago
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.
Assignee

Comment 5

2 years ago
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.
Assignee

Comment 6

2 years ago
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+
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 10

2 years ago
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
Last Resolved: 2 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?
Assignee

Comment 16

2 years ago
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.