Closed
Bug 1170543
Opened 10 years ago
Closed 9 years ago
Do not promote waiting service worker to active until all active events have been completed
Categories
(Core :: DOM: Service Workers, defect, P2)
Core
DOM: Service Workers
Tracking
()
RESOLVED
FIXED
mozilla50
People
(Reporter: jaoo, Assigned: bkelly)
References
(Blocks 1 open bug)
Details
Attachments
(5 files, 1 obsolete file)
7.89 KB,
patch
|
asuth
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
3.64 KB,
patch
|
asuth
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
3.48 KB,
patch
|
asuth
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
2.00 KB,
patch
|
asuth
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
9.44 KB,
patch
|
asuth
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•10 years ago
|
Updated•10 years ago
|
Assignee | ||
Comment 1•9 years ago
|
||
Catalin, is this still an issue after your SW lifetime refactor?
Flags: needinfo?(catalin.badea392)
Comment 2•9 years ago
|
||
Yes, we don't wait for in-progress requests when replacing the active worker.
Flags: needinfo?(catalin.badea392)
Comment 3•9 years ago
|
||
register-wait-forever-in-install-worker.https.html examines this behavior. It needs to be enabled when this is fixed.
Blocks: 1189023
Comment 4•9 years ago
|
||
(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•9 years ago
|
Blocks: ServiceWorkers-compat
Assignee | ||
Comment 5•9 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•9 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)
Updated•9 years ago
|
Priority: -- → P2
Assignee | ||
Comment 7•9 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•9 years ago
|
||
This is the test that blink wrote for this feature.
Assignee | ||
Comment 9•9 years ago
|
||
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•9 years ago
|
||
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•9 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•9 years ago
|
||
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•9 years ago
|
||
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•9 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•9 years ago
|
||
The WPT test imported from blink. Just renumbering here.
Attachment #8774918 -
Attachment is obsolete: true
Attachment #8775281 -
Flags: review?(bugmail)
Updated•9 years ago
|
Attachment #8775275 -
Flags: review?(bugmail) → review+
Updated•9 years ago
|
Attachment #8775278 -
Flags: review?(bugmail) → review+
Updated•9 years ago
|
Attachment #8775279 -
Flags: review?(bugmail) → review+
Comment 15•9 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
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•9 years ago
|
||
I pushed this earlier and it looks reasonably green.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc05620fa508&selectedJob=24643831
Comment 17•9 years ago
|
||
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•9 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•9 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
Comment 20•9 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Assignee | ||
Updated•9 years ago
|
status-firefox47:
--- → wontfix
status-firefox48:
--- → wontfix
status-firefox49:
--- → affected
status-firefox-esr45:
--- → disabled
Assignee | ||
Comment 21•9 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•9 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•9 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•9 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•9 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 26•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8775278 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8775279 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8775280 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8775281 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 27•9 years ago
|
||
bugherder uplift |
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
You need to log in
before you can comment on or make changes to this bug.
Description
•