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

RESOLVED FIXED in Firefox 49

Status

()

Core
DOM: Service Workers
P2
normal
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: jaoo, Assigned: bkelly)

Tracking

(Blocks: 1 bug)

Trunk
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

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

Details

Attachments

(5 attachments, 1 obsolete attachment)

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

Updated

3 years ago
Component: DOM → DOM: Service Workers
Depends on: 1131352
See Also: → bug 1131352

Updated

3 years ago
Blocks: 1173500
No longer blocks: 1059784
(Assignee)

Comment 1

2 years ago
Catalin, is this still an issue after your SW lifetime refactor?
Flags: needinfo?(catalin.badea392)
Yes, we don't wait for in-progress requests when replacing the active worker.
Flags: needinfo?(catalin.badea392)

Comment 3

2 years ago
register-wait-forever-in-install-worker.https.html examines this behavior.  It needs to be enabled when this is fixed.
Blocks: 1189023
(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
No longer blocks: 1189023
(Assignee)

Updated

2 years ago
Blocks: 1226983
(Assignee)

Comment 5

2 years ago
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
(Assignee)

Comment 6

2 years ago
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.
Flags: needinfo?(bugmail)
Priority: -- → P2
(Assignee)

Comment 7

2 years ago
I'm going to see if I can implement this before the SW meeting this week.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Flags: needinfo?(bugmail)
(Assignee)

Comment 8

2 years ago
Created attachment 8774918 [details] [diff] [review]
P2 Import actiation.https.html wpt test from blink. r=asuth

This is the test that blink wrote for this feature.
(Assignee)

Updated

2 years ago
Depends on: 1228277
(Assignee)

Updated

2 years ago
Depends on: 1289658
(Assignee)

Comment 9

2 years ago
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.
Attachment #8775275 - Flags: review?(bugmail)
(Assignee)

Comment 10

2 years ago
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.
Attachment #8775278 - Flags: review?(bugmail)
(Assignee)

Comment 11

2 years ago
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.
(Assignee)

Comment 12

2 years ago
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.
Attachment #8775279 - Flags: review?(bugmail)
(Assignee)

Comment 13

2 years ago
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).
Attachment #8775280 - Flags: review?(bugmail)
(Assignee)

Updated

2 years ago
Attachment #8775280 - Attachment description: P3 Expose ServiceWorker idle thread state to ServiceWorkerManager. r=asuth → P4 Don't active service worker until the previous active service worker is idle. r=asuth
(Assignee)

Comment 14

2 years ago
Created attachment 8775281 [details] [diff] [review]
P5 Import actiation.https.html wpt test from blink. r=asuth

The WPT test imported from blink.  Just renumbering here.
Attachment #8774918 - Attachment is obsolete: true
Attachment #8775281 - Flags: review?(bugmail)
Attachment #8775275 - Flags: review?(bugmail) → review+
Attachment #8775278 - Flags: review?(bugmail) → review+
Attachment #8775279 - Flags: review?(bugmail) → review+
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.
Attachment #8775280 - Flags: review?(bugmail) → review+
(Assignee)

Comment 16

2 years ago
I pushed this earlier and it looks reasonably green.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc05620fa508&selectedJob=24643831
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.
Attachment #8775281 - Flags: review?(bugmail) → review+
(Assignee)

Comment 18

2 years ago
(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

2 years ago
Pushed by bkelly@mozilla.com:
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
(Assignee)

Updated

2 years ago
status-firefox41: affected → wontfix
status-firefox47: --- → wontfix
status-firefox48: --- → wontfix
status-firefox49: --- → affected
status-firefox-esr45: --- → disabled
(Assignee)

Comment 21

2 years ago
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
Attachment #8775275 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 22

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

Comment 21.
Attachment #8775278 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 23

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

See comment 21.
Attachment #8775279 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 24

2 years ago
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.
Attachment #8775280 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 25

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

Comment 21.
Attachment #8775281 - Flags: approval-mozilla-aurora?
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.
Attachment #8775275 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8775278 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8775279 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8775280 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8775281 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Updated

2 years ago
Blocks: 1290116
(Assignee)

Updated

a year ago
Depends on: 1336529
You need to log in before you can comment on or make changes to this bug.