Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Define Purge Service Worker Registrations #1506

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Define Purge Service Worker Registrations #1506

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ae30000
Define Purge Service Worker Registrations
jungkees Feb 25, 2020
f76c101
Address comments:
Mar 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 35 additions & 2 deletions docs/index.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2351,6 +2351,8 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
A <a>job</a> has a <dfn id="dfn-job-list-of-equivalent-jobs">list of equivalent jobs</dfn> (a list of <a>jobs</a>). It is initially the empty list.

A [=job=] has a <dfn id="dfn-job-force-bypass-cache-flag">force bypass cache flag</dfn>. It is initially unset.

A [=job=] has a <dfn id="dfn-immediate-unregister-flag">immediate unregister flag</dfn>. It is initially unset.
</div>


Expand Down Expand Up @@ -3161,7 +3163,7 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
: Output
:: none

1. If the [=environment settings object/origin=] of |job|'s [=job/scope url=] is not |job|'s [=job/client=]'s [=environment settings object/origin=], then:
1. If |job|'s [=job/client=] is not null, and the [=environment settings object/origin=] of |job|'s [=job/scope url=] is not |job|'s [=job/client=]'s [=environment settings object/origin=], then:
1. Invoke [=Reject Job Promise=] with |job| and "{{SecurityError}}" {{DOMException}}.
1. Invoke <a>Finish Job</a> with |job| and abort these steps.
1. Let |registration| be the result of running <a>Get Registration</a> algorithm passing |job|'s [=job/scope url=] as the argument.
Expand All @@ -3170,7 +3172,8 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. Invoke <a>Finish Job</a> with |job| and abort these steps.
1. [=map/Remove=] [=scope to registration map=][|job|'s [=job/scope url=]].
1. Invoke <a>Resolve Job Promise</a> with |job| and true.
1. Invoke [=Try Clear Registration=] with |registration|.
1. If |job|'s [=immediate unregister flag=] is set, invoke [=Clear Registration=] with |registration|.
1. Else, invoke [=Try Clear Registration=] with |registration|.

Note: If [=Try Clear Registration=] does not trigger [=Clear Registration=] here, [=Clear Registration=] is tried again when the last client [=using=] the registration is [=Handle Service Worker Client Unload|unloaded=] or the [=extend lifetime promises=] for the registration's service workers settle.

Expand Down Expand Up @@ -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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed. That's nicer indeed.

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:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

1. Let |job| be the first item in |jobQueue|.
1. Invoke [=Reject Job Promise=] with |job| and "{{AbortError}}" {{DOMException}}.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it to TypeError.

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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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|.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the step calling into Handle Service Worker Client Unload.

1. Set |client|'s [=active service worker=] to null.
1. Invoke [=Notify Controller Change=] with |client|.
</section>

<section algorithm>
<h3 id="update-registration-state-algorithm"><dfn>Update Registration State</dfn></h3>

Expand Down