Closed
Bug 1157627
Opened 9 years ago
Closed 9 years ago
After unregistering a SW from about:serviceworkers, the new registrations are not shown
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: baku, Assigned: baku)
Details
Attachments
(1 file, 2 obsolete files)
1.11 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Attachment #8596499 -
Flags: review?(nsm.nikhil)
Comment on attachment 8596499 [details] [diff] [review] r.patch Review of attachment 8596499 [details] [diff] [review]: ----------------------------------------------------------------- Could you explain why you are reseting the registration when one already exists?
Attachment #8596499 -
Flags: review?(nsm.nikhil)
Assignee | ||
Comment 2•9 years ago
|
||
Because we already send the unregister message to the actor and ServiceWorkerRegistrar is out-of-sync: https://mxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#1796
Flags: needinfo?(nsm.nikhil)
Assignee | ||
Updated•9 years ago
|
Attachment #8596499 -
Flags: review?(nsm.nikhil)
Comment on attachment 8596499 [details] [diff] [review] r.patch Review of attachment 8596499 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, I am still confused. about:serviceworkers seems to call swm.unregister() which will, if the registration is not controlling any page, remove the registration, and if it is controlling, it will be marked as pending uninstall. In either case, if the same scope is register()ed again, either a new registration is created entirely from scratch, or it enters this if (mRegistration) {} case, in which case we don't need to create a new one. One thing i noticed in the about:serviceworker implementation was that it uses the SWM in the same process. Could that be the problem on e10s due to which the registrar is removing the entry but the content SWM still has it? (I am assuming about: pages run in the parent since they seem to have privileges).
Flags: needinfo?(nsm.nikhil) → needinfo?(amarchesini)
Assignee | ||
Comment 4•9 years ago
|
||
> Sorry, I am still confused. about:serviceworkers seems to call > swm.unregister() which will, if the registration is not controlling any > page, remove the registration, and if it is controlling, it will be marked > as pending uninstall. Correct. And at this point the SWM calls mActor->SendUnregisterServiceWorker(). This removes the SW from the list of known ServiceWorkers in ServiceWorkerRegistrar. In either case, if the same scope is register()ed > again, either a new registration is created entirely from scratch, or it > enters this if (mRegistration) {} case, in which case we don't need to > create a new one. Sure. But the ServiceWorkerRegistar doesn't know about this 'new' ServiceWorker and the .txt file will go out-of-sync. > One thing i noticed in the about:serviceworker implementation was that it > uses the SWM in the same process. Could that be the problem on e10s due to > which the registrar is removing the entry but the content SWM still has it? > (I am assuming about: pages run in the parent since they seem to have > privileges). about:serviceworkers gets data from the ServiceWorkerRegistrarar. This uses IPDL and it runs in the main-process.
Flags: needinfo?(amarchesini) → needinfo?(nsm.nikhil)
Comment on attachment 8596499 [details] [diff] [review] r.patch Review of attachment 8596499 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerManager.cpp @@ +573,5 @@ > if (mRegistration) { > nsRefPtr<ServiceWorkerInfo> newest = mRegistration->Newest(); > if (newest && mScriptSpec.Equals(newest->ScriptSpec()) && > mScriptSpec.Equals(mRegistration->mScriptSpec)) { > + mRegistration = swm->CreateNewRegistration(mScope, mPrincipal); There is a call to swm->StoreRegistration(...) a few lines later which gets skipped in this branch. I think what you want here is to call swm->StoreRegistration(mRegistration) before the return.
Attachment #8596499 -
Flags: review?(nsm.nikhil)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8599527 -
Flags: review?(nsm.nikhil)
Comment on attachment 8599527 [details] [diff] [review] r.patch Review of attachment 8599527 [details] [diff] [review]: ----------------------------------------------------------------- r=me with one fix. ::: dom/workers/ServiceWorkerManager.cpp @@ +582,5 @@ > mScriptSpec.Equals(mRegistration->mScriptSpec)) { > mRegistration->mPendingUninstall = false; > Succeed(); > Done(NS_OK); > + swm->StoreRegistration(mPrincipal, mRegistration); Just move this to before Succeed().
Attachment #8599527 -
Flags: review?(nsm.nikhil) → review+
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8596499 -
Attachment is obsolete: true
Attachment #8599527 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2db8c3341c29
Keywords: checkin-needed
Comment 11•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2db8c3341c29
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Assignee | ||
Comment 12•9 years ago
|
||
We don't have tests for about:serviceworkers. I'll check if we have some tests for about:* pages.
Flags: needinfo?(amarchesini)
Flags: needinfo?(nsm.nikhil)
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•