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)
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().
Comment 1•7 years ago
|
||
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!
Blocks: ServiceWorkers-stability
Flags: needinfo?(echuang)
Assignee | ||
Comment 2•7 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)
Comment 3•7 years ago
|
||
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)
Updated•7 years ago
|
Priority: -- → P2
Comment 4•7 years ago
|
||
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•7 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•7 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 7•7 years ago
|
||
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 | ||
Comment 8•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5a399096055ee86c46a5265cc7bd837acf7c360
Assignee | ||
Comment 9•7 years ago
|
||
Update the patch according to comment 7.
Attachment #8922241 -
Attachment is obsolete: true
Attachment #8922600 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 10•7 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
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ac63e59b68b4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Updated•7 years ago
|
status-firefox-esr52:
--- → unaffected
Flags: in-testsuite+
Comment 12•7 years ago
|
||
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 13•7 years ago
|
||
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?
Comment 14•7 years ago
|
||
Note, the dependent bug 1413056 is not necessary for the uplift.
Reporter | ||
Comment 15•7 years ago
|
||
Do we really know this won't affact Fx57 or we simply don't care at this point?
Assignee | ||
Comment 16•7 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.
Reporter | ||
Comment 17•7 years ago
|
||
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.
Comment 18•7 years ago
|
||
(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+
Comment 20•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/39275addee59
You need to log in
before you can comment on or make changes to this bug.
Description
•