1228277 - Service Worker skipWaiting doesn't work when called during the script evaluation step

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

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

Trying to set the skipWaiting flag while loading the worker for the first time (script evaluation step) will fail because our main thread code expects to find the associated ServiceWorkerInfo in the registration object. However, mInstallingWorker is not set until after the script evaluation ended.

We pass the skip-waiting.https.html web platform test because it does a sequence of skipWaiting().then(skipWaiting()), which takes long enough for the worker to become installing, but the first call always fails.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

For what its worth, most of the examples I see call skipWaiting() from the install event handler.  I think its to avoid extra work every time the service worker is started up.

Comment 2

[(no longer active)]

Attachment: Support calling skipWaiting() really early during top-level service worker script evaluation

We do this by remembering that we need to do this if we
don't find the installing service worker, and do it
lazily later.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Since skipWaiting() is called from the worker, can't we set the flag directly on the ServiceWorkerInfo by way of WorkerPrivate and ServiceWorkerPrivate?

Comment 4

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

Comment on attachment 8712003 [details] [diff] [review]
Support calling skipWaiting() really early during top-level service worker script evaluation

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

I'm also in favor of setting the flag directly on the ServiceWorkerInfo.
I guess we need ServiceWorkerPrivate to be reachable from WorkerPrivate.

::: dom/workers/ServiceWorkerManager.cpp
@@ +2902,5 @@
> +ServiceWorkerRegistrationInfo::QueueSetSkipWaitingFlag(uint64_t aServiceWorkerID)
> +{
> +  AssertIsOnMainThread();
> +
> +  MOZ_ASSERT(!mSetSkipWaitingFlagQueue.Contains(aServiceWorkerID));

skipWaiting() can be called twice and hit this assert.

@@ +2903,5 @@
> +{
> +  AssertIsOnMainThread();
> +
> +  MOZ_ASSERT(!mSetSkipWaitingFlagQueue.Contains(aServiceWorkerID));
> +  mSetSkipWaitingFlagQueue.AppendElement(aServiceWorkerID);

If during evaluation the script calls skipWaiting() and then throws an error, it will add to the queue but never remove from it since the worker never reaches the installing phase.

Comment 5

[(no longer active)]

Attachment: Support calling skipWaiting() really early during top-level service worker script evaluation

We do this by moving the flag to ServiceWorkerRegistrationInfo, so that
we can set it irrespective of whether mInstallingWorker has been set
before.

Comment 6

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

Comment on attachment 8712723 [details] [diff] [review]
Support calling skipWaiting() really early during top-level service worker script evaluation

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

With this patch, setting the skipWaiting flag once will cause all workers from that registration
to skip the waiting phase, which is wrong.

::: dom/workers/ServiceWorkerManager.cpp
@@ +1855,5 @@
>  
>  void
>  ServiceWorkerRegistrationInfo::TryToActivate()
>  {
> +  if (!IsControllingDocuments() || SkipWaitingFlag()) {

If an older worker has called skipWaiting() this will be true for all subsequent workers.

Comment 7

[(no longer active)]

Ah, of course!  I'm not really doing a good job here.  ;-)  New patch hopefully tomorrow.

Comment 8

[(no longer active)]

FWIW I won't have time to work on this.  Someone else please pick it up.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

The test provided by blink for bug 1170543 needs this.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Track service worker scripts during evaluation in ServiceWorkerRegistrationInfo. r=asuth

I think the easiest solution here is to track the evaluating worker in the registration object just like .installing, .waiting, and .active.  (We don't expose it to script of course, though.)  This will then let the skipWaiting() code locate the evaluating script by its ID in the next patch.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make skipWaiting() check the evaluating service worker script. r=asuth

This patch makes skipWaiting() look for any matching worker ID on the registration.  I also removed the error return value since it was unused.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Remove skipWaiting() work around from the wpt test. r=asuth

This removes the wpt test work around we had in place.  This does not really trigger a failure without P1, though, since we typically pick up one of the later skipWaiting() calls.

The test being added in bug 1170543 will directly verify P1 and P2, however.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e621b42e5ab

Comment 13

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

Comment on attachment 8775256 [details] [diff] [review]
P1 Track service worker scripts during evaluation in ServiceWorkerRegistrationInfo. r=asuth

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

The comments on the state changing methods are very much appreciated.

Comment 14

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

Comment on attachment 8775257 [details] [diff] [review]
P2 Make skipWaiting() check the evaluating service worker script. r=asuth

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +2848,4 @@
>    }
>  
> +  RefPtr<ServiceWorkerInfo> worker =
> +    registration->GetServiceWorkerInfoById(aServiceWorkerID);

This is a very nice cleanup!

::: dom/workers/ServiceWorkerManager.h
@@ +254,5 @@
>  
>    nsresult
>    ClaimClients(nsIPrincipal* aPrincipal, const nsCString& aScope, uint64_t aId);
>  
> +  void

(I confirm the one caller does not care and did not check the return value.)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6032373138d2
P1 Track service worker scripts during evaluation in ServiceWorkerRegistrationInfo. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4e7fea164eb
P2 Make skipWaiting() check the evaluating service worker script. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a326dd181aa
P3 Remove skipWaiting() work around from the wpt test. r=asuth

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6032373138d2
https://hg.mozilla.org/mozilla-central/rev/d4e7fea164eb
https://hg.mozilla.org/mozilla-central/rev/2a326dd181aa

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8775256 [details] [diff] [review]
P1 Track service worker scripts during evaluation in ServiceWorkerRegistrationInfo. r=asuth

Approval Request Comment
Approval Request Comment
[Feature/regressing bug #]: Service workers.
[User impact if declined]: This is a web compatibility issue with chrome.  It also blocks a high priority web compat issue in bug 1170543.  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 bug 1170543.
[Risks and why]: Minimal.  Only affects service workers.
[String/UUID change made/needed]: None

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8775257 [details] [diff] [review]
P2 Make skipWaiting() check the evaluating service worker script. r=asuth

See comment 17.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8775258 [details] [diff] [review]
P3 Remove skipWaiting() work around from the wpt test. r=asuth

See comment 17.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8775256 [details] [diff] [review]
P1 Track service worker scripts during evaluation in ServiceWorkerRegistrationInfo. r=asuth

Improve the compatibility, taking it!

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/156642ee5fb4
https://hg.mozilla.org/releases/mozilla-aurora/rev/a4aeae74f3ca
https://hg.mozilla.org/releases/mozilla-aurora/rev/a850b3b58cb1