1170543 - Do not promote waiting service worker to active until all active events have been completed

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect, P2)

Description

[José Antonio Olivera Ortega [:jaoo]]

See bug 1131352 comment 45.

(In reply to Andrea Marchesini (:baku) from comment #45)
> Comment on attachment 8609843 [details] [diff] [review]
> Part 2: Add ServiceWorkerGlobalScope skipWaiting()
> 
> Review of attachment 8609843 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/workers/ServiceWorkerManager.cpp
> @@ +1332,5 @@
> > +  nsRefPtr<ServiceWorkerInfo> exitingWorker = mActiveWorker.forget();
> > +  if (!exitingWorker)
> > +    return;
> > +
> > +  // FIXME(jaoo): Wait for exitingWorker to finish and terminate.
> 
> write here the follow-up bug ID.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Catalin, is this still an issue after your SW lifetime refactor?

Comment 2

[Cătălin Badea (:catalinb)]

Yes, we don't wait for in-progress requests when replacing the active worker.

Comment 3

[(no longer active)]

register-wait-forever-in-install-worker.https.html examines this behavior.  It needs to be enabled when this is fixed.

Comment 4

[Cătălin Badea (:catalinb)]

(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

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

I'm going to see if I can implement this before the SW meeting this week.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Import actiation.https.html wpt test from blink. r=asuth

This is the test that blink wrote for this feature.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8775278 [details] [diff] [review]
P2 Explicitly track the idle KeepAliveToken separately. r=asuth

Sorry, this patch was actually just renaming the keep alive token for the grace timer.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

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

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Import actiation.https.html wpt test from blink. r=asuth

The WPT test imported from blink.  Just renumbering here.

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8775280 [details] [diff] [review]
P4 Don't active service worker until the previous active service worker is idle. r=asuth

Review of attachment 8775280 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/ServiceWorkerRegistrationInfo.cpp
@@ +219,5 @@
>   */
>  void
>  ServiceWorkerRegistrationInfo::TryToActivate()
>  {
> +  bool controlling = IsControllingDocuments();

While you're here: Should this also be asserting it's on the main thread?  If you do this, maybe fix the ACtivateAsync uppercase C above in the comment.  Not required.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

I pushed this earlier and it looks reasonably green.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc05620fa508&selectedJob=24643831

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

(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 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7558f1c86e2
P1 Create separate KeepAliveTokens for service worker events and script evaluation. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e030b615ff4
P2 Explicitly track the idle KeepAliveToken separately. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a8daba93dc3
P3 Expose ServiceWorker idle thread state to ServiceWorkerManager. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ea4b2c81b22
P4 Don't active service worker until the previous active service worker is idle. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3489ac13c44
P5 Import actiation.https.html wpt test from blink. r=asuth

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d7558f1c86e2
https://hg.mozilla.org/mozilla-central/rev/8e030b615ff4
https://hg.mozilla.org/mozilla-central/rev/2a8daba93dc3
https://hg.mozilla.org/mozilla-central/rev/6ea4b2c81b22
https://hg.mozilla.org/mozilla-central/rev/a3489ac13c44

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8775278 [details] [diff] [review]
P2 Explicitly track the idle KeepAliveToken separately. r=asuth

Comment 21.

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8775279 [details] [diff] [review]
P3 Expose ServiceWorker idle thread state to ServiceWorkerManager. r=asuth

See comment 21.

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8775280 [details] [diff] [review]
P4 Don't active service worker until the previous active service worker is idle. r=asuth

Comment 21.

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8775281 [details] [diff] [review]
P5 Import actiation.https.html wpt test from blink. r=asuth

Comment 21.

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8775275 [details] [diff] [review]
P1 Create separate KeepAliveTokens for service worker events and script evaluation. r=asuth

One more web compat fix, taking it.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9606affd4ad0
https://hg.mozilla.org/releases/mozilla-aurora/rev/1614ddee2ff8
https://hg.mozilla.org/releases/mozilla-aurora/rev/69cda3e2305a
https://hg.mozilla.org/releases/mozilla-aurora/rev/42b9f972ca98
https://hg.mozilla.org/releases/mozilla-aurora/rev/b8fa569b3a32