Improve Activate with Try Activate #1065

jungkees · 2017-02-03T07:13:08Z

This adds concepts and algorithms to track events (all service worker
events: lifecycle events, functional events, extendable message events)
that have pending extension promises so they can be used to check if the
registration's waiting worker can be promoted.

This also changes Install algorithm and related steps to match to the
implementations behavior. That is, instead of waiting for the promotion
condition in Install algorithm, newly introduced Try Activate algorithm
just tries to activate depending on the condition and return. Try
Activate is called when:

A service worker is installed.
The last client controlled by the existing active worker is unloaded.
skipWaiting() is called.
The extend lifetime promises for the existing active worker settle.

Related issue: #916.

This adds concepts and algorithms to track events (all service worker events: lifecycle events, functional events, extendable message events) that have pending extension promises so they can be used to check if the registration's waiting worker can be promoted. This also changes Install algorithm and related steps to match to the implementations behavior. That is, instead of waiting for the promotion condition in Install algorithm, newly introduced Try Activate algorithm just tries to activate depending on the condition and return. Try Activate is called when: - A service worker is installed. - The last client controlled by the existing active worker is unloaded. - skipWaiting() is called. - The extend lifetime promises for the existing active worker settle. Related issue: #916.

jungkees · 2017-02-03T07:16:38Z

Chromium and Gecko implemented and shipped the resolution in #916 already. The spec follows with this PR. Please review.

/cc @mattto @wanderview @annevk @slightlyoff

jakearchibald · 2017-02-07T14:58:22Z

docs/index.bs

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

    A [=/service worker=] has an associated <dfn export id="dfn-set-of-event-types-to-handle">set of event types to handle</dfn> (a [=ordered set|set=]) whose [=item=] is an <a>event listener</a>'s event type. It is initially an empty set.

+    A [=/service worker=] has an associated <dfn export id="dfn-set-of-extended-events">set of extended events</dfn> (a [=ordered set|set=]) whose [=item=] is an [=event=]. It is initially an empty set.


Should [=event=] be {{ExtendableEvent}}?

Yes, that narrow typing would be better. Done.

jakearchibald · 2017-02-07T15:00:57Z

docs/index.bs

@@ -364,6 +366,7 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
            1. Else, let it be initialized to a new {{Client}} object that represents the worker associated with |incumbentGlobal|.
            1. Let the {{ExtendableMessageEvent/ports}} attribute of |e| be initialized to |newPorts|.
            1. <a>Dispatch</a> |e| at |destination|.
+            1. Invoke [=Update Service Worker Extended Events Set=] with |serviceWorker| and |e|.


So is this something every service worker event needs to do? Can we do this automatically when extendable events are created? Worried about every spec remembering to do this.

We gate all the service worker events through Handle Fetch, Handle Foreign Fetch, Handle Functional Events, and ServiceWorker.postMessage(). So, maintenance would be okay. Also, I intended it to be called after dispatching an event returns.

I think I'm missing something. How would this work for push events?

Handle Functional Event runs https://github.com/w3c/ServiceWorker/pull/1065/files#diff-27b79860afe28f01aed4f1f6228367faR3082 after the callback steps given by other specs that are supposed to dispatch the functional events.

jakearchibald · 2017-02-07T15:12:16Z

docs/index.bs

+      : Output
+      :: None
+
+      1. Assert: |registration|'s [=waiting worker=] is not null.


This is called when promises passed to event.waitUntil settle, which can happen when there isn't a waiting worker, so isn't it common for the assertion to fail?

Right. I missed that point. Made it return null when waiting worker is null there.

jakearchibald · 2017-02-07T15:25:49Z

docs/index.bs

-          1. Set |redundantWorker| to |registration|'s <a>active worker</a>.
-          1. Wait for |redundantWorker| to finish handling any in-progress requests.
-          1. <a lt="Terminate Service Worker">Terminate</a> |redundantWorker|.
+      1. If |registration|'s [=active worker=] is not null, |redundantWorker| be |registration|'s [=active worker=].


nit: Missing let/set before |redundantWorker|?

jakearchibald · 2017-02-07T15:29:48Z

docs/index.bs

-      1. If |registration|'s <a>active worker</a> is not null, then:
-          1. Set |redundantWorker| to |registration|'s <a>active worker</a>.
-          1. Wait for |redundantWorker| to finish handling any in-progress requests.
-          1. <a lt="Terminate Service Worker">Terminate</a> |redundantWorker|.


When are these workers terminated now?

I thought it would be not needed when Try Activate successfully called Activate. But you're right. It'd be safer to call Terminate Service Worker before get it redundant. In the follow-up commit, I addressed this point and also moved steps for invoking Update Worker State for the redundant works up, which both Chromium and Gecko do.

jakearchibald · 2017-02-07T15:46:01Z

docs/index.bs

@@ -3100,6 +3122,21 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
  </section>

  <section algorithm>
+    <h3 id="update-service-worker-extended-events-set-algorithm"><dfn>Update Service Worker Extended Events Set</dfn></h3>


I'm a little worried about the lifecycle of this, but I might be worrying over nothing.

If a service worker abruptly terminates (due to long-running tasks or whatever), what happens to these events? Should we clear the set of extended events on service worker terminate?

I guess we'll need to handle timeouts to waitUntil extension promises at some point.

Good point! I added a step to Terminate Service Worker to remove all the items in the service worker's set of extended events.

For extended events, I thought we decided to leave them run to completion: #916 (comment). So, I removed the steps that abort the in-progress requests.

To clarify what happens when a service worker abruptly terminates, Terminate Service Worker resets the set of extended events to an empty set (done in this PR), retains the tasks (for functional events) in the queue so the next worker run will execute them.

Ah, I missed that. Sounds good!

jungkees · 2017-02-08T07:09:50Z

@jakearchibald, thanks for review! I uploaded a follow-up commit. PTAL.

According to the latest spec, the skipWaiting promise should be resolved after 'activate' event is dispatched. This change is from w3c/ServiceWorker#1065. Tracing through the spec, skipWaiting() enters "Try Activate". "Try Activate" invokes "Activate". "Activate" blocks until the final step: "13. Run the Update Worker State algorithm passing registration’s active worker and activated as the arguments." "Update Worker State" queues a task to set ServiceWorker#state to 'activated'. But in step 10, we have dispatched the 'activate' event. Therefore the order should be: 1. 'activate' event handler runs 2. skipWaiting() promise resolves 3. ServiceWorker#state is set to 'activated' So we correct the test case here and delete all the wrong expected files. BUG=725616 Change-Id: Id0765988c7cdf48f39bb73ccb3fc0cce6ea60949 Reviewed-on: https://chromium-review.googlesource.com/646244 Commit-Queue: Matt Falkenhagen <falken@chromium.org> Reviewed-by: Matt Falkenhagen <falken@chromium.org> Cr-Commit-Position: refs/heads/master@{#499513}

jungkees requested review from mkruisselbrink and jakearchibald February 3, 2017 07:13

jakearchibald requested changes Feb 7, 2017

View reviewed changes

Address Jake's comments

8784046

jakearchibald approved these changes Feb 16, 2017

View reviewed changes

jungkees and others added 2 commits February 16, 2017 18:45

Update bikesheded documents

05f0af6

Merge branch 'master' into improve-activate-fix

2015e32

jungkees merged commit f1a1120 into master Feb 16, 2017

jungkees deleted the improve-activate-fix branch February 16, 2017 09:58

mfalken mentioned this pull request Sep 1, 2017

skipWaiting() promise should resolve after promotion to .active #1187

Open

chromium-wpt-export-bot mentioned this pull request Sep 4, 2017

Correct the skip-waiting-installed.html behavior web-platform-tests/wpt#7251

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Activate with Try Activate #1065

Improve Activate with Try Activate #1065

jungkees commented Feb 3, 2017

jungkees commented Feb 3, 2017

jakearchibald Feb 7, 2017

jungkees Feb 8, 2017

jakearchibald Feb 7, 2017

jungkees Feb 8, 2017

jakearchibald Feb 14, 2017

jungkees Feb 15, 2017

jakearchibald Feb 7, 2017

jungkees Feb 8, 2017

jakearchibald Feb 7, 2017

jungkees Feb 8, 2017

jakearchibald Feb 7, 2017

jungkees Feb 8, 2017

jakearchibald Feb 7, 2017

jungkees Feb 8, 2017

jakearchibald Feb 14, 2017

jungkees commented Feb 8, 2017

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

		A [=/service worker=] has an associated <dfn export id="dfn-set-of-event-types-to-handle">set of event types to handle</dfn> (a [=ordered set\|set=]) whose [=item=] is an <a>event listener</a>'s event type. It is initially an empty set.

		A [=/service worker=] has an associated <dfn export id="dfn-set-of-extended-events">set of extended events</dfn> (a [=ordered set\|set=]) whose [=item=] is an [=event=]. It is initially an empty set.

Improve Activate with Try Activate #1065

Improve Activate with Try Activate #1065

Conversation

jungkees commented Feb 3, 2017

jungkees commented Feb 3, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jungkees commented Feb 8, 2017