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.

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.

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.

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

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.

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

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.

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

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

The test provided by blink for bug 1170543 needs this.

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.

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

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

Pushed by bkelly@mozilla.com: 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

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 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 on attachment 8775257 [details] [diff] [review] P2 Make skipWaiting() check the evaluating service worker script. r=asuth See comment 17.

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

Comment on attachment 8775256 [details] [diff] [review] P1 Track service worker scripts during evaluation in ServiceWorkerRegistrationInfo. r=asuth Improve the compatibility, taking it!

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