(In reply to [Vacation: Nov 3-19] from comment #3)
> register-wait-forever-in-install-worker.https.html examines this behavior.
> It needs to be enabled when this is fixed.
My patches from bug 1189659 fix this test. The test actually checks that [[Update]] terminates a previous installing worker and successfully activates the new worker. I think this doesn't need to block 1189023.
Regarding the fix for this bug, we can check ServiceWorkerPrivate.mTokenCount for the number of pending operations. We could do something like:
1. Prevent the ServiceWorkerPrivate from accepting new events
2. When mTokenCount reaches 0, notify ServiceWorkerManager that the worker is done
3. continue the activate algorithm

I think we are changing the spec a bit. We're moving the check for in-flight events earlier so it effectively keeps the service worker in the waiting state. A worker will only be promoted active once all events are done. Its a subtle change, but avoids the need to create a new pseudo-state between waiting and active.
See the discussion here:
https://github.com/slightlyoff/ServiceWorker/issues/916
Note, there are existing use cases that google have seen where sites break without this. We probably want to implement it soonish.

Summary: Wait for exiting worker (active one) to finish and terminate it when purging the active worker during the activate algorithm (follow-up bug 1131352) → Do not promote waiting service worker to active until all active events have been completed

Google has implemented this. Matt writes:
"It should be in Chrome 53, expected to reach stable channel in early September."
That is equivalent to FF49. Which means we should implement this very soon.
Andrew, do you have any time to look at this? I think it should be somewhat straightforward and google wrote some tests already.

Created attachment 8775275[details][diff][review]
P1 Create separate KeepAliveTokens for service worker events and script evaluation. r=asuth
So we basically need to track if the current service worker is "idle" or not. The definition of "idle" here is roughly:
- Not processing any events.
Which we can roughly translate to:
- Not being held alive by any event waitUntil() calls.
As a first step, this patch makes every event use a separate KeepAliveToken. This will help us distinguish in later patches between being kept alive for the idle grace timer vs being kept alive due to an event.

Created attachment 8775278[details][diff][review]
P2 Explicitly track the idle KeepAliveToken separately. r=asuth
Explicitly tie the concept of "idleness" to the keep alive token count. We are idle if either:
1) There are no KeepAliveTokens outstanding (worker is not running...) Or...
2) There is a single KeepAliveToken and its the grace timer's token. In this case the worker is being held alive, but its not doing anything.
We also asynchronously tell the SWM whenever a worker goes idle.

Created attachment 8775279[details][diff][review]
P3 Expose ServiceWorker idle thread state to ServiceWorkerManager. r=asuth
This is the patch that actually does:
Explicitly tie the concept of "idleness" to the keep alive token count. We are idle if either:
1) There are no KeepAliveTokens outstanding (worker is not running...) Or...
2) There is a single KeepAliveToken and its the grace timer's token. In this case the worker is being held alive, but its not doing anything.
We also asynchronously tell the SWM whenever a worker goes idle.

Created attachment 8775280[details][diff][review]
P4 Don't active service worker until the previous active service worker is idle. r=asuth
This patch then implements the main logic required for this bug. We delay activation until the currently .active worker is no longer processing events (i.e. is idle).

Comment on attachment 8775281[details][diff][review]
P5 Import actiation.https.html wpt test from blink. r=asuth
Review of attachment 8775281[details][diff][review]:
-----------------------------------------------------------------
I know you didn't write this test (although it is a good one! kudos to the author!), but I just wanted to say this is a really high quality patch set! So kudos to you too! :)
::: testing/web-platform/tests/service-workers/service-worker/activation.https.html
@@ +47,5 @@
> + return navigator.serviceWorker.register(worker_url, { scope: scope });
> + })
> + .then(r => {
> + registration = r;
> + add_result_callback(() => registration.unregister);
This line looks suspicious. registration.unregister will never get called. I think it wants call parens so it actually kicks off an unregister when the test completes.
If the blink test is already WPT-bound, it's okay to leave this broken to avoid merge hassles, but if we're the ones adding it for reals now, ideally fix to avoid it getting copied-and-pasted and causing future sadness.

(In reply to Andrew Sutherland [:asuth] from comment #17)
> > + add_result_callback(() => registration.unregister);
>
> This line looks suspicious. registration.unregister will never get called.
> I think it wants call parens so it actually kicks off an unregister when the
> test completes.
>
> If the blink test is already WPT-bound, it's okay to leave this broken to
> avoid merge hassles, but if we're the ones adding it for reals now, ideally
> fix to avoid it getting copied-and-pasted and causing future sadness.
Nice catch! I'll let the blink folks know and just fix it here. I told them I'd upstream the test for them.

Comment on attachment 8775275[details][diff][review]
P1 Create separate KeepAliveTokens for service worker events and script evaluation. r=asuth
Approval Request Comment
Approval Request Comment
[Feature/regressing bug #]: Service workers.
[User impact if declined]: This is a high priority web compatibility issue with chrome. We need to uplift to 49 in order to match when chrome will ship their version of bug 1170543. There are high profile sites that will depend on this feature.
[Describe test coverage new/current, TreeHerder]: New wpt test included in this bug.
[Risks and why]: Minimal. Only affects service workers.
[String/UUID change made/needed]: None