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.

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

Kill resurrection #1415

Merged
merged 5 commits into from Jun 6, 2019

Conversation

@jakearchibald
Copy link
Contributor

commented Jun 3, 2019

@jakearchibald jakearchibald requested a review from mattto Jun 3, 2019
@@ -518,12 +518,12 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
<section algorithm="navigator-service-worker-unregister">
<h4 id="navigator-service-worker-unregister">{{ServiceWorkerRegistration/unregister()}}</h4>

Note: The {{ServiceWorkerRegistration/unregister()}} method unregisters the [=/service worker registration=]. It is important to note that the currently <a>controlled</a> [=/service worker client=]'s <a>active service worker</a>'s <a>containing service worker registration</a> is effective until all the [=/service worker clients=] (including itself) using this [=/service worker registration=] unload. That is, the {{ServiceWorkerRegistration/unregister()}} method only affects subsequent <a lt="navigate">navigations</a>.
Note: The {{ServiceWorkerRegistration/unregister()}} method unregisters the [=/service worker registration=]. It is important to note that the currently [=controlled=] [=/service worker client=]'s [=active service worker=]'s [=containing service worker registration=] is effective until all the [=/service worker clients=] (including itself) using this [=/service worker registration=] unload. That is, the {{ServiceWorkerRegistration/unregister()}} method only affects subsequent [=navigate|navigations=].

This comment has been minimized.

Copy link
@jakearchibald

jakearchibald Jun 3, 2019

Author Contributor

Just converting to [=bikeshed shorthand=]. Couldn't help myself. Sorry.

This comment has been minimized.

Copy link
@mattto

mattto Jun 3, 2019

Member

Thanks I've been doing the same :)


<dfn method for="ExtendableEvent"><code>waitUntil(|f|)</code></dfn> method *must* run these steps:

1. If the {{Event/isTrusted}} attribute is false, [=throw=] an "{{InvalidStateError}}" {{DOMException}}.
1. Let |event| be the [=context object=].
1. [=ExtendableEvent/Add lifetime promise=] |f| to |event|.

This comment has been minimized.

Copy link
@jakearchibald

jakearchibald Jun 3, 2019

Author Contributor

I moved this into its own algorithm as we have it copy-pasted in the spec twice, and it's something other specs may want to do.

This comment has been minimized.

Copy link
@mattto

mattto Jun 3, 2019

Member

Looks nicer.

1. If |registration| is not null, invoke [=Try Activate=] with |registration|.
1. Upon [=upon fulfillment|fulfillment=] or [=upon rejection|rejection=] of |promise|, [=queue a microtask=] to run these substeps:
1. Decrement |event|'s [=ExtendableEvent/pending promises count=] by one.
1. If |event|'s [=ExtendableEvent/pending promises count=] is 0, then:

This comment has been minimized.

Copy link
@jakearchibald

jakearchibald Jun 3, 2019

Author Contributor

Seems pointless to do the next steps if the count is > 0.

This comment has been minimized.

Copy link
@mattto

mattto Jun 3, 2019

Member

Agreed.

1. If |registration|'s [=uninstalling flag=] is set, invoke [=Try Clear Registration=] with |registration|.
1. If |registration| is not null, invoke [=Try Activate=] with |registration|.
1. If |registration| is [=service worker registration/unregistered=], invoke [=Try Clear Registration=] with |registration|.
1. Invoke [=Try Activate=] with |registration|.

This comment has been minimized.

Copy link
@jakearchibald

jakearchibald Jun 3, 2019

Author Contributor

The null check is already performed above.

1. If |scopeString| matches |key|, set |registration| to |value|.
1. Return |registration|.
1. If |scopeString| matches |key|, then return |value|.
1. Return null.

This comment has been minimized.

Copy link
@jakearchibald

jakearchibald Jun 3, 2019

Author Contributor

Just tidied this up a bit

This comment has been minimized.

Copy link
@mattto

mattto Jun 3, 2019

Member

Nice.

@jakearchibald

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

It's worth noting that this surfaces more of the weirdness in navigator.serviceWorker.ready.

  1. Register SW.
  2. await navigator.serviceWorker.ready.
  3. Unregister & reregister.

Now you're in a state where navigator.serviceWorker.ready has settled, but the service worker in your scope isn't actually ready.

@mattto
mattto approved these changes Jun 3, 2019
Copy link
Member

left a comment

Looks good! Just have nits.

@@ -1435,18 +1445,10 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe

<dfn method for="FetchEvent"><code>respondWith(|r|)</code></dfn> method *must* run these steps:

1. Let |event| be the [=context object=].
1. If the <a>dispatch flag</a> is unset, [=throw=] an "{{InvalidStateError}}" {{DOMException}}.
1. If the [=FetchEvent/respond-with entered flag=] is set, [=throw=] an "{{InvalidStateError}}" {{DOMException}}.

This comment has been minimized.

Copy link
@mattto

mattto Jun 3, 2019

Member

Should these be "event's dispatch flag..." and "event's respond-with entered flag" instead of "the flag"?

This comment has been minimized.

Copy link
@jakearchibald

jakearchibald Jun 3, 2019

Author Contributor

Yeah, I got a bit lazy since I wasn't touching these lines, but I should fix this up while I'm here.

@@ -220,6 +218,8 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe

A [=/service worker registration=] has an associated <dfn export>navigation preload header value</dfn>, which is a [=byte sequence=]. It is initially set to \`<code>true</code>\`.

A [=/service worker registration=] is said to be <dfn export id="dfn-service-worker-registration-unregistered">unregistered</dfn> if [=scope to registration map=][[=service worker registration/scope url=]] is not this [=/service worker registration=].

This comment has been minimized.

Copy link
@mattto

mattto Jun 3, 2019

Member

nitty: is it required to say "this registration's scope url" to qualify "scope url"?

This comment has been minimized.

Copy link
@jakearchibald

jakearchibald Jun 3, 2019

Author Contributor

Yeah, probably for the best. Will change.

@@ -518,12 +518,12 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
<section algorithm="navigator-service-worker-unregister">
<h4 id="navigator-service-worker-unregister">{{ServiceWorkerRegistration/unregister()}}</h4>

Note: The {{ServiceWorkerRegistration/unregister()}} method unregisters the [=/service worker registration=]. It is important to note that the currently <a>controlled</a> [=/service worker client=]'s <a>active service worker</a>'s <a>containing service worker registration</a> is effective until all the [=/service worker clients=] (including itself) using this [=/service worker registration=] unload. That is, the {{ServiceWorkerRegistration/unregister()}} method only affects subsequent <a lt="navigate">navigations</a>.
Note: The {{ServiceWorkerRegistration/unregister()}} method unregisters the [=/service worker registration=]. It is important to note that the currently [=controlled=] [=/service worker client=]'s [=active service worker=]'s [=containing service worker registration=] is effective until all the [=/service worker clients=] (including itself) using this [=/service worker registration=] unload. That is, the {{ServiceWorkerRegistration/unregister()}} method only affects subsequent [=navigate|navigations=].

This comment has been minimized.

Copy link
@mattto

mattto Jun 3, 2019

Member

Thanks I've been doing the same :)


<dfn method for="ExtendableEvent"><code>waitUntil(|f|)</code></dfn> method *must* run these steps:

1. If the {{Event/isTrusted}} attribute is false, [=throw=] an "{{InvalidStateError}}" {{DOMException}}.
1. Let |event| be the [=context object=].
1. [=ExtendableEvent/Add lifetime promise=] |f| to |event|.

This comment has been minimized.

Copy link
@mattto

mattto Jun 3, 2019

Member

Looks nicer.

1. If |registration| is not null, invoke [=Try Activate=] with |registration|.
1. Upon [=upon fulfillment|fulfillment=] or [=upon rejection|rejection=] of |promise|, [=queue a microtask=] to run these substeps:
1. Decrement |event|'s [=ExtendableEvent/pending promises count=] by one.
1. If |event|'s [=ExtendableEvent/pending promises count=] is 0, then:

This comment has been minimized.

Copy link
@mattto

mattto Jun 3, 2019

Member

Agreed.

@@ -2454,7 +2455,7 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
:: none

1. Let |registration| be the result of running the <a>Get Registration</a> algorithm passing |job|'s [=job/scope url=] as the argument.
1. If |registration| is null or |registration|'s <a>uninstalling flag</a> is set, then:
1. If |registration| is null or |registration| is [=service worker registration/unregistered=], then:

This comment has been minimized.

Copy link
@mattto

mattto Jun 3, 2019

Member

Can Get Registration return an unregistered registration?

This comment has been minimized.

Copy link
@jakearchibald

jakearchibald Jun 3, 2019

Author Contributor

Hah, no. Good catch.

docs/index.bs Show resolved Hide resolved
@@ -3177,7 +3178,6 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. Set |matchingScope| to the result of <a lt="URL parser">parsing</a> |matchingScopeString|.
1. Assert: |matchingScope|'s [=url/origin=] and |clientURL|'s [=url/origin=] are [=same origin=].
1. Let |registration| be the result of running <a>Get Registration</a> algorithm passing |matchingScope| as the argument.
1. If |registration| is not null and |registration|'s <a>uninstalling flag</a> is set, return null.
1. Return |registration|.

This comment has been minimized.

Copy link
@mattto

mattto Jun 3, 2019

Member

Should we collapse these two steps into just "Return the result of running..."

1. If |scopeString| matches |key|, set |registration| to |value|.
1. Return |registration|.
1. If |scopeString| matches |key|, then return |value|.
1. Return null.

This comment has been minimized.

Copy link
@mattto

mattto Jun 3, 2019

Member

Nice.

Copy link
Collaborator

left a comment

Thanks @jakearchibald @mattto. I reviewed the changes too and it LGTM!

@@ -3007,7 +3005,7 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. If |registration| is null, then:
1. Invoke <a>Resolve Job Promise</a> with |job| and false.
1. Invoke <a>Finish Job</a> with |job| and abort these steps.
1. Set |registration|'s <a>uninstalling flag</a>.
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|.

This comment has been minimized.

Copy link
@jungkees

jungkees Jun 5, 2019

Collaborator

This comment should not block merging the PR.

@mattto, I just saw https://chromium.googlesource.com/chromium/src.git/+/fafebc8a59d87a8c6f0620a91ee7cb3a0d8331eb. Do you think we should use Clear Registration over Try Clear Registration here as well?

Also, we didn't do "drop the active version as a controller of any clients" in the spec in any case. Because of the changes this PR makes, any reload (and subsequent navigations for sure) won't be able to use the unregistered registration again. So, the clients having the active worker of the unregistered registration as a controller will be isolated in their own scenario within their lifetime. Do you think we still need it given the changes in this PR?

This comment has been minimized.

Copy link
@mattto

mattto Jun 6, 2019

Member

We could discuss in an issue, but I think dropping the active version is orthogonal to this PR. The point of dropping the active version is a kind of "unclaim" or "immediate unregister" that's been proposed in other issues. In the case of the Chrome CL, we did this because it's an emergency force delete situation when something's gone wrong with the service worker (e.g., disk corruption).

This comment has been minimized.

Copy link
@jungkees

jungkees Jun 6, 2019

Collaborator

That sounds reasonable. I agree "unclaim" is a separate issue we can discuss, if necessary. I am fine with Unregister using Try Clear Registration as in this PR too.

@jungkees

This comment has been minimized.

Copy link
Collaborator

commented Jun 5, 2019

/cc @asutherland, @SteveBeckerMSFT to help track the changes.

@jakearchibald

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

I'll file bugs once this lands.

@jakearchibald jakearchibald merged commit 5c60822 into master Jun 6, 2019
2 checks passed
2 checks passed
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
@jakearchibald jakearchibald deleted the kill-ressurection branch Jun 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.