Define Purge Service Worker Registrations #1506

jungkees · 2020-02-26T00:23:35Z

This change adds an algorithm, Purge Service Worker Registrations, that
immediately unregister - by calling into Clear Registration - service
worker registrations for a given origin. It takes an origin and a
boolean argument for unclaiming controlled clients. This is designed for
Clear-Site-Data's "storage" directive. job's immediate unregister flag
added in this change can be used for unregister({immediate: true}) or
alternative designs as a TODO.

Issue: #614.
CSD issue: w3c/webappsec-clear-site-data#54.
CSD PR: w3c/webappsec-clear-site-data#63.

Preview | Diff

This change adds an algorithm, Purge Service Worker Registrations, that immediately unregister - by calling into Clear Registration - service worker registrations for a given origin. It takes an origin and a boolean argument for unclaiming controlled clients. This is designed for Clear-Site-Data's "storage" directive. job's immediate unregister flag added in this change can be used for unregister({immediate: true}) or alternative designs as a TODO. Issue: #614.

jungkees · 2020-02-26T00:30:26Z

I gave it a try for unblocking the kill-switch work for Clear-Site-Data: "storage". Please take a look.

/cc @asakusuma @n8schloss @asutherland @cdumez @youennf @slightlyoff @SteveBeckerMSFT

Clear-Site-Data: "storage" has used Service Workers' unregister() as Service Workers' had no external algorithm that allows immediate purging of the service worker registrations. This change calls into Purging Service Worker Reigstration algorithm defined in Service Workers with the origin and intention to unclaim the controlled clients when "storage" directive is specified. Issue: w3c#54. Service Workers issue: w3c/ServiceWorker#614. Service Workers PR: w3c/ServiceWorker#1506.

asakusuma

Thanks for getting the ball rolling here, @jungkees. And thanks for getting the initial implementation done @SteveBeckerMSFT

asakusuma · 2020-02-26T01:52:16Z

docs/index.bs

+              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}}.


I'm not very familiar with speccing errors, but is there a way to ensure that the error message makes it clear that the promise was rejected because the service worker was purged?

I think "AbortError" DOMException is most relevant for the error context here. When the service operation sends out the Clear-Site-Data: "storage", it can expect the pending register/update/unregister jobs to be aborted and delete from the queue. Would it make sense?

I think we should use "AbortError" for https://dom.spec.whatwg.org/#aborting-ongoing-activities generally. I'd use the generic error here type. (The message that goes with it is currently not standardized and up to implementations.)

Changed it to TypeError.

asakusuma · 2020-02-26T01:59:14Z

docs/index.bs

+              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:


Should we only purge the job queue if unclaim is true? It might be unexpected to kill all in-flight jobs without also specifying unclaim.

I made the caller (Clear-Site-Data) purge the registrations with the unclaim enabled. I wanted to consider the possibility of any future change where a caller may want to purge the storage without unclaiming.

However, if our decision is making Clear-Site-Data: "strorage" unclaim by default, I can move the unclaiming steps into Unregister algorithm so it runs when job's immediate unregister flag is set.

asakusuma · 2020-02-26T02:01:35Z

docs/index.bs

+              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|.


What's the purpose of calling Handle Service Worker Client Unload here? I believe Handle Service Worker Client Unload will call Try Unregister and Try Activate, not sure either of those are necessary since we already did unregister and we shouldn't be trying to activate a new service worker.

I think you're right. My concern was about taking care of the registration with different scope than the one that is claimed for in the case of claim(): #682. But as we basically purge the registration here entirely, we don't need to update the registration state. Can @asakusuma or anyone confirm that is right?

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.

Removed the step calling into Handle Service Worker Client Unload.

jakearchibald

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?

jakearchibald · 2020-03-05T10:34:44Z

docs/index.bs

+
+      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.

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

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

Addressed. That's nicer indeed.

jakearchibald · 2020-03-05T10:42:00Z

docs/index.bs

+                  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?

I thought deleting registration and maintaining controller as two separate primitives, but agree to your point that it wouldn't be useful to keep the controller which is in "redundant" state. I moved unclaim steps into Unregister that unconditionally runs when the immediate unregister flag is set.

jakearchibald · 2020-03-05T10:49:52Z

docs/index.bs

+              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.

- Moving unclaim steps to Unregister running unconditionally when immediate unregister flag is set - Removing Handle Service Worker Client Unload in unclaim steps - Using TypeError instad of AbortError DOMExcetion

jungkees · 2020-03-17T00:54:25Z

it feels like we need to do something about the currently running job.

Agreed. I'll follow up on this as a TODO.

annevk · 2020-06-12T18:22:05Z

One thing that has happened since February is that we're getting much closer to being able to integrate the underlying semantics of Clear-Site-Data into the storage standard. This is being discussed in whatwg/storage#88 and issues referenced from there.

https://storage.spec.whatwg.org/ also defines a set of primitives that service workers need to use to store their data. We're currently working on doing the same for localStorage/sessionStorage/IDB.

The work you are doing here really ties into all that. It might be worth tackling as a follow-up, but I thought I'd raise it now to create some more awareness and alert you to the possibility that the particular way these things integrate might change a bit.

jungkees · 2020-06-15T18:52:06Z

@annevk, thanks for the heads up. Sounds great. Looking forward to plugging into the primitives Storage comes up with.

wanderview · 2020-08-13T18:59:10Z

What is the status of this PR? Is it close to merging? I think Microsoft already implemented and shipped the clear-site-data purging in chromium.

jungkees · 2020-08-14T00:30:22Z

Things we should address to merge this PR:

Covering currently running jobs more accurately: Define Purge Service Worker Registrations #1506 (comment)
Check out if we should use any primitive updated in the Storage standard: Define Purge Service Worker Registrations #1506 (comment)

I'm afraid I can't find cycles now. If anyone can help with this, I'd appreciate that.

asakusuma · 2021-01-14T22:06:54Z

@jungkees @jakearchibald would it be possible to merge this PR or add a placeholder so that we can start cross linking from other specs and proposals, like storage/buckets?

jakearchibald · 2021-01-18T11:37:38Z

I think we should remove some of the not-quite-correct normative steps, but happy to land a placeholder algorithm.

jungkees requested review from jakearchibald, wanderview, annevk and mfalken February 26, 2020 00:23

jungkees mentioned this pull request Feb 26, 2020

Use Purge Service Worker Registrations for storage w3c/webappsec-clear-site-data#63

Open

asakusuma reviewed Feb 26, 2020

View reviewed changes

jakearchibald requested changes Mar 5, 2020

View reviewed changes

Address comments:

f76c101

- Moving unclaim steps to Unregister running unconditionally when immediate unregister flag is set - Removing Handle Service Worker Client Unload in unclaim steps - Using TypeError instad of AbortError DOMExcetion

wanderview mentioned this pull request Jun 12, 2020

Do service/shared workers and BroadcastChannel deserve a special strategy? privacycg/storage-partitioning#9

Closed

annevk mentioned this pull request Jul 8, 2020

Replacement design whatwg/storage#88

Open

wanderview mentioned this pull request Aug 13, 2020

consider allowing a non-scope identifier for registrations #1512

Open

asakusuma mentioned this pull request Sep 10, 2020

Virtual calls around TPAC time #1536

Closed

Base automatically changed from master to main February 4, 2021 19:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define Purge Service Worker Registrations #1506

Define Purge Service Worker Registrations #1506

jungkees commented Feb 26, 2020 •

edited by pr-preview bot

Loading

jungkees commented Feb 26, 2020 •

edited

Loading

asakusuma left a comment

asakusuma Feb 26, 2020

jungkees Feb 27, 2020

annevk Mar 3, 2020

jungkees Mar 17, 2020

asakusuma Feb 26, 2020

jungkees Feb 27, 2020

asakusuma Feb 26, 2020

jungkees Feb 27, 2020

jakearchibald Mar 5, 2020

jungkees Mar 17, 2020

jakearchibald left a comment

jakearchibald Mar 5, 2020

jungkees Mar 17, 2020

jakearchibald Mar 5, 2020

jungkees Mar 17, 2020

jakearchibald Mar 5, 2020

jungkees commented Mar 17, 2020

annevk commented Jun 12, 2020

jungkees commented Jun 15, 2020

wanderview commented Aug 13, 2020

jungkees commented Aug 14, 2020

asakusuma commented Jan 14, 2021

jakearchibald commented Jan 18, 2021

Define Purge Service Worker Registrations #1506

Are you sure you want to change the base?

Define Purge Service Worker Registrations #1506

Conversation

jungkees commented Feb 26, 2020 • edited by pr-preview bot Loading

jungkees commented Feb 26, 2020 • edited Loading

asakusuma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakearchibald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jungkees commented Mar 17, 2020

annevk commented Jun 12, 2020

jungkees commented Jun 15, 2020

wanderview commented Aug 13, 2020

jungkees commented Aug 14, 2020

asakusuma commented Jan 14, 2021

jakearchibald commented Jan 18, 2021

jungkees commented Feb 26, 2020 •

edited by pr-preview bot

Loading

jungkees commented Feb 26, 2020 •

edited

Loading