Kill resurrection #1415

jakearchibald · 2019-06-03T12:25:35Z

Fixes #1204.
Tests web-platform-tests/wpt#17139.
More tests web-platform-tests/wpt#17203.

jakearchibald · 2019-06-03T12:28:20Z

docs/index.bs

@@ -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=].


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

Thanks I've been doing the same :)

jakearchibald · 2019-06-03T12:30:06Z

docs/index.bs


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


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.

Looks nicer.

jakearchibald · 2019-06-03T12:31:02Z

docs/index.bs

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


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

docs/index.bs

jakearchibald · 2019-06-03T12:33:42Z

docs/index.bs

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


The null check is already performed above.

jakearchibald · 2019-06-03T12:34:34Z

docs/index.bs

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


Just tidied this up a bit

jakearchibald · 2019-06-03T13:21:10Z

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

Register SW.
await navigator.serviceWorker.ready.
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.

mfalken

Looks good! Just have nits.

mfalken · 2019-06-03T13:31:16Z

docs/index.bs

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


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

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

mfalken · 2019-06-03T13:38:07Z

docs/index.bs

@@ -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=].


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

Yeah, probably for the best. Will change.

mfalken · 2019-06-03T13:38:30Z

docs/index.bs

@@ -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=].


Thanks I've been doing the same :)

mfalken · 2019-06-03T13:38:47Z

docs/index.bs


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


Looks nicer.

mfalken · 2019-06-03T13:38:53Z

docs/index.bs

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


mfalken · 2019-06-03T13:39:26Z

docs/index.bs

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


Can Get Registration return an unregistered registration?

Hah, no. Good catch.

docs/index.bs

mfalken · 2019-06-03T13:50:30Z

docs/index.bs

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


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

mfalken · 2019-06-03T13:51:10Z

docs/index.bs

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


jungkees

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

jungkees · 2019-06-05T21:18:17Z

docs/index.bs

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

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

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 · 2019-06-05T21:20:24Z

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

jakearchibald · 2019-06-06T05:54:14Z

I'll file bugs once this lands.

jakearchibald · 2019-06-06T07:48:19Z

Chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=971571 (cc @mattto @SteveBeckerMSFT)
Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1557244 (cc @asutherland)
WebKit: https://bugs.webkit.org/show_bug.cgi?id=198602 (cc @youennf)

jakearchibald added 2 commits June 3, 2019 09:56

Remove repetition in respondWith

c3f8eca

Kill resurrection (fixes #1204)

b622281

jakearchibald requested a review from mfalken June 3, 2019 12:25

Better id

13fa43a

jakearchibald commented Jun 3, 2019

View reviewed changes

docs/index.bs Show resolved Hide resolved

jakearchibald commented Jun 3, 2019

View reviewed changes

jakearchibald mentioned this pull request Jun 3, 2019

resurrected service workers don't fire an activate event which may break expectations #1204

Closed

mfalken approved these changes Jun 3, 2019

View reviewed changes

jakearchibald added 2 commits June 3, 2019 15:46

Addressing feedback

29002b7

Oops

88f1b6a

jakearchibald mentioned this pull request Jun 3, 2019

Service workers: Resurrection is no longer a thing web-platform-tests/wpt#17139

Merged

jungkees self-requested a review June 5, 2019 20:02

jungkees approved these changes Jun 5, 2019

View reviewed changes

jungkees reviewed Jun 5, 2019

View reviewed changes

jungkees mentioned this pull request Jun 5, 2019

Normative spec doesn't necessarily terminate service workers w3c/webappsec-clear-site-data#54

Open

jakearchibald merged commit 5c60822 into master Jun 6, 2019

jakearchibald deleted the kill-ressurection branch June 6, 2019 07:37

jungkees mentioned this pull request Jun 13, 2019

Should we restore the registration record removing step in Clear Registration? #1433

Open

asutherland mentioned this pull request Mar 5, 2024

Should persistent notifications be associated with service worker registration or the scope? whatwg/notifications#205

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kill resurrection #1415

Kill resurrection #1415

jakearchibald commented Jun 3, 2019 •

edited

Loading

jakearchibald Jun 3, 2019

mfalken Jun 3, 2019

jakearchibald Jun 3, 2019

mfalken Jun 3, 2019

jakearchibald Jun 3, 2019

mfalken Jun 3, 2019

jakearchibald Jun 3, 2019

jakearchibald Jun 3, 2019

mfalken Jun 3, 2019

jakearchibald commented Jun 3, 2019

mfalken left a comment

mfalken Jun 3, 2019

jakearchibald Jun 3, 2019

mfalken Jun 3, 2019

jakearchibald Jun 3, 2019

mfalken Jun 3, 2019

mfalken Jun 3, 2019

mfalken Jun 3, 2019

mfalken Jun 3, 2019

jakearchibald Jun 3, 2019

mfalken Jun 3, 2019

mfalken Jun 3, 2019

jungkees left a comment

jungkees Jun 5, 2019

mfalken Jun 6, 2019

jungkees Jun 6, 2019

jungkees commented Jun 5, 2019

jakearchibald commented Jun 6, 2019

jakearchibald commented Jun 6, 2019

		@@ -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=].

Kill resurrection #1415

Kill resurrection #1415

Conversation

jakearchibald commented Jun 3, 2019 • edited Loading

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 commented Jun 3, 2019

mfalken 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

Choose a reason for hiding this comment

jungkees 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

jungkees commented Jun 5, 2019

jakearchibald commented Jun 6, 2019

jakearchibald commented Jun 6, 2019

jakearchibald commented Jun 3, 2019 •

edited

Loading