Closed Bug 1157627 Opened 5 years ago Closed 5 years ago

After unregistering a SW from about:serviceworkers, the new registrations are not shown

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: baku, Assigned: baku)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch r.patch (obsolete) — 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)
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)
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)
> 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)
Attached patch r.patch (obsolete) — Splinter Review
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+
Attached patch 241731.diffSplinter Review
Attachment #8596499 - Attachment is obsolete: true
Attachment #8599527 - Attachment is obsolete: true
Keywords: checkin-needed
Should this have a test?
Flags: needinfo?(amarchesini)
Flags: in-testsuite?
https://hg.mozilla.org/mozilla-central/rev/2db8c3341c29
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
We don't have tests for about:serviceworkers. I'll check if we have some tests for about:* pages.
Flags: needinfo?(amarchesini)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.