Re: [w3c/ServiceWorker] Define Purge Service Worker Registrations (#1506)

jakearchibald requested changes on this pull request.

This is a great step in the right direction. However, it feels like we need to do *something* about the currently running job.

Right now, this PR dequeues the job and rejects the promise, but it might still be half-way through the update algorithm. As a result, it might start trying to modify the registration after unregistration.

I tried to spec this in detail last year, but ended up feeling like I would have to rewrite the whole spec to accommodate every algorithm potentially aborting at any point. Maybe a slightly hand-wavey way would be better?

Maybe we could have an *aborted* flag on job, then each algorithm could have a bit of prose like:

If, at any point, job's *aborted* flag is set, abort this algorithm immediately, abort any ongoing fetches created in steps 3.4, 7, 9.5, and terminate the worker created in step 5.

Would that work?

> @@ -3230,6 +3233,36 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe

           * |registration|'s [=active worker=] is null or the result of running [=Service Worker Has No Pending Events=] with |registration|’s [=active worker=] is true.
   </section>
 
+  <section algorithm>
+    <h3 id="purge-service-worker-registration-algorithm"><dfn export>Purge Service Worker Registrations</dfn></h3>
+
+      : Input
+      :: |origin|, an [=/origin=]
+      :: |unclaim|, an optional boolean, false by default
+      : Output
+      :: None
+
+      1. [=map/For each=] <var ignore=''>scope</var> → |registration| of [=scope to registration map=]:
+          1. Let |scopeURL| be |registration|'s [=service worker registration/scope url=].
+          1. If |scopeURL|'s [=/origin=] is |origin|, then:

Nit: I tend to invert early-exit conditions like this.

1. If |scopeURL|'s [=/origin=] is not |origin|, then [=continue=].

Then you don't need to nest the remaining steps.

> +      :: None
+
+      1. [=map/For each=] <var ignore=''>scope</var> → |registration| of [=scope to registration map=]:
+          1. Let |scopeURL| be |registration|'s [=service worker registration/scope url=].
+          1. If |scopeURL|'s [=/origin=] is |origin|, then:
+              1. Let |job| be the result of running [=Create Job=] with *unregister*, |scopeURL|, null, null, and null.
+              1. Set |job|'s [=immediate unregister flag=].
+              1. Let |jobQueue| be [=scope to job queue map=][|job|'s [=job/scope url=], [=URL serializer|serialized=]].
+              1. Assert: |jobQueue| is not null.
+              1. [=While=] |jobQueue| is not empty:
+                  1. Let |job| be the first item in |jobQueue|.
+                  1. Invoke [=Reject Job Promise=] with |job| and "{{AbortError}}" {{DOMException}}.
+                  1. [=queue/Dequeue=] from |jobQueue|.
+              1. Invoke [=Schedule Job=] with |job|.
+              1. Wait until |job|'s [=job promise=] settles.
+              1. If |unclaim| is true, then:

If unclaim is false, wouldn't that leave pages being controlled by a service worker in a "redundant" state? That doesn't seem right. If we're ripping the service worker out, surely we have to unclaim too?

> +          1. Let |scopeURL| be |registration|'s [=service worker registration/scope url=].
+          1. If |scopeURL|'s [=/origin=] is |origin|, then:
+              1. Let |job| be the result of running [=Create Job=] with *unregister*, |scopeURL|, null, null, and null.
+              1. Set |job|'s [=immediate unregister flag=].
+              1. Let |jobQueue| be [=scope to job queue map=][|job|'s [=job/scope url=], [=URL serializer|serialized=]].
+              1. Assert: |jobQueue| is not null.
+              1. [=While=] |jobQueue| is not empty:
+                  1. Let |job| be the first item in |jobQueue|.
+                  1. Invoke [=Reject Job Promise=] with |job| and "{{AbortError}}" {{DOMException}}.
+                  1. [=queue/Dequeue=] from |jobQueue|.
+              1. Invoke [=Schedule Job=] with |job|.
+              1. Wait until |job|'s [=job promise=] settles.
+              1. If |unclaim| is true, then:
+                  1. For each [=/service worker client=] |client| [=using=] |registration|:
+                      1. Assert: |client|'s [=active service worker=] is not null.
+                      1. Invoke [=Handle Service Worker Client Unload=] with |client|.

I agree we shouldn't be calling "Handle Service Worker Client Unload" here (the client isn't unloading). The registration state is already purged in the unregister job.



-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/ServiceWorker/pull/1506#pullrequestreview-369458093