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

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: catalinb, Assigned: bkelly)

Tracking

(Blocks 1 bug)

Trunk
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 wontfix, firefox47 wontfix, firefox48 wontfix, firefox49 fixed, firefox-esr45 disabled, firefox50 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Reporter

Description

4 years ago
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.

Comment 2

3 years ago
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.
Attachment #8712003 - Flags: review?(catalin.badea392)
Since skipWaiting() is called from the worker, can't we set the flag directly on the ServiceWorkerInfo by way of WorkerPrivate and ServiceWorkerPrivate?
Reporter

Comment 4

3 years ago
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.
Attachment #8712003 - Flags: review?(catalin.badea392) → review-

Comment 5

3 years ago
We do this by moving the flag to ServiceWorkerRegistrationInfo, so that
we can set it irrespective of whether mInstallingWorker has been set
before.
Attachment #8712723 - Flags: review?(catalin.badea392)

Updated

3 years ago
Attachment #8712003 - Attachment is obsolete: true
Reporter

Comment 6

3 years ago
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.
Attachment #8712723 - Flags: review?(catalin.badea392) → review-

Comment 7

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

Comment 8

3 years ago
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.
Assignee: nobody → bkelly
Blocks: 1170543
Status: NEW → ASSIGNED
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.
Attachment #8712723 - Attachment is obsolete: true
Attachment #8775256 - Flags: review?(bugmail)
Attachment #8775256 - Attachment description: bug1228277_p1_evaluatingworker.patch → P1 Track service worker scripts during evaluation in ServiceWorkerRegistrationInfo. 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.
Attachment #8775257 - Flags: review?(bugmail)
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
Attachment #8775258 - Flags: review?(bugmail)
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.
Attachment #8775256 - Flags: review?(bugmail) → review+
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.)
Attachment #8775257 - Flags: review?(bugmail) → review+
Attachment #8775258 - Flags: review?(bugmail) → review+

Comment 15

3 years ago
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

Comment 16

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6032373138d2
https://hg.mozilla.org/mozilla-central/rev/d4e7fea164eb
https://hg.mozilla.org/mozilla-central/rev/2a326dd181aa
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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
Attachment #8775256 - Flags: approval-mozilla-aurora?
Comment on attachment 8775257 [details] [diff] [review]
P2 Make skipWaiting() check the evaluating service worker script. r=asuth

See comment 17.
Attachment #8775257 - Flags: approval-mozilla-aurora?
Comment on attachment 8775258 [details] [diff] [review]
P3 Remove skipWaiting() work around from the wpt test. r=asuth

See comment 17.
Attachment #8775258 - Flags: approval-mozilla-aurora?
Comment on attachment 8775256 [details] [diff] [review]
P1 Track service worker scripts during evaluation in ServiceWorkerRegistrationInfo. r=asuth

Improve the compatibility, taking it!
Attachment #8775256 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8775257 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8775258 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.