service worker activate should be executed after install onstatechange events are fired

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bkelly, Assigned: dimi)

Tracking

(Blocks 1 bug)

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 wontfix, firefox46 fixed, firefox47 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

See:

  https://code.google.com/p/chromium/issues/detail?id=530085

And:

  https://github.com/slightlyoff/ServiceWorker/commit/eb76491581397b4fb94a02a4cc010de8d0d7e666

I think this means in our code we should just execute Activate() via a runnable back to the current thread.  This will ensure the onestatechange runnable is flushed from the main thread event queue before the activate runs.
Assignee

Updated

3 years ago
Assignee: nobody → dlee
Status: NEW → ASSIGNED
Assignee

Comment 1

3 years ago
Posted patch WIP. testcase (obsolete) — Splinter Review
Assignee

Comment 3

3 years ago
Some testcases fail:

[1]. Make TryToActivate in ContinueAfterInstallEvent ASYNC
*test_aboutserviceworkers fail
1.There is already an active worker
2.DocShell check IsAvailable(), get true
3.A softupdate is triggered before and now exeute ContinueAfterInstallEvent
 - PurgeActiveWorker
 - Call ASYNC activate()
4.DispatchFetchEvent is called because step2 passed before, MOZ_ASSERT(mActiveWorker) fail due to purgeActiveWorker in step3

[2]. Wrap a function include both PurgeActiveWorker and TryToActive, and make it ASYNC 
*skip-waiting.https.html fail
1.There is already an active worker
2.Register again and now there is an installing worker
3.Set installing worker skip waiting flag
4.installing worker now execute ContinueAfterInstallEvent, call ASYNC (PurgeActiveWorker and TryToActive)
5.Set skipwaiting again, because there is a waiting worker(step 4) so TryToActivate will be executed.
  TryToActivate in SetSkipWaiting is SYNC, so activate() is executed and triggers ASYNC "FinishActivate"
6.Step4 ASYNC call is execute and will remove current activeworker and then returned because there is no waiting worker
7.FinishActivate(triggered in step5) is now executed and return because there is no active worker(step6 remove it)
8.So "Acitvated" statechange will never be execute and test timeout
Assignee

Comment 4

3 years ago
Hi Ben,
I remove the assert of DispatchFetchEvent because of Comment 3 first scenario.
Attachment #8706832 - Attachment is obsolete: true
Attachment #8710325 - Flags: review?(bkelly)
Comment on attachment 8710325 [details] [diff] [review]
Async execute Activate after oninstall event

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

This is a good start, but I'd like to change a few things.  Can you see how the tests work with these changes?

Also, I'm going to be mostly on PTO till the end of next week.  Sorry for any delays in reviewing.

::: dom/workers/ServiceWorkerManager.cpp
@@ +1871,5 @@
>    if (!IsControllingDocuments() || mWaitingWorker->SkipWaitingFlag()) {
> +    if (aAsync) {
> +      nsCOMPtr<nsIRunnable> r =
> +      NS_NewRunnableMethod(this,
> +                           &ServiceWorkerRegistrationInfo::Activate);

I think we want to include the conditional in the async execution.  The state of the registration could change between making the check and executing the Activate() the way you have it here.

Also, I think we should just make all the callsites to TryToActivate() be async unconditionally.  I'm not sure why we would want some sync and other async.

So, can you make a TryToActivateAsync() that issues a runnable to the current TryToActivate() or something?

@@ +3629,5 @@
>      }
>  
> +    // This should only happen if IsAvailable() returned true, but it is possible
> +    // that active worker has been removed after IsAvailable() check.
> +    if (!registration->mActiveWorker) {

I think this is ok, but I don't like the race skipWaiting() creates by purging the worker before Activate() is run.  I'm going to write a spec issue for that.
Attachment #8710325 - Flags: review?(bkelly) → review-
I wrote this spec issue for the skipWaiting thing:

  https://github.com/slightlyoff/ServiceWorker/issues/821
Assignee

Comment 7

3 years ago
Running try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d52ebb2623f1
to see what testcases we need to fix after changing all |TryToActivate| calls to asynchronous.
Assignee

Updated

3 years ago
Depends on: 1241929
Assignee

Comment 8

3 years ago
I see most test cases pass except Web-platform-tests "skip-waiting-installed.https.html"

The reason is that skipWaiting() promise is now resolved prior than sw script receive "activated" event
and skip-waiting-installed.https.html[1] expects 'skipWaiting promise should be resolved after activated'

Following is the detail steps:
1. sw script calls skipWaiting()
2. SWM |SetSkipWaiting| is called and then it executes ASYNC TryToActivate 
3. |Activate| executes and calls SendLifyCycleEvent("Activated")
4. skipWaiting promise is resolved and at this time sw script not yet receives |onactivated| event

Hi Ben,
I cannot find the assumption - "skipWaiting promise should be resolved after activated" is defined somewhere in spec.
Do we need to follow this ? or it is ok to change the testcase ?

[1]https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/mozilla/tests/service-workers/service-worker/resources/skip-waiting-installed-worker.js#25
[2]https://www.w3.org/TR/service-workers/#service-worker-global-scope-skipwaiting
Flags: needinfo?(bkelly)
I agree.  The test seems to be wrong.

Also, fwiw, I would recommend using the github spec instead of the TR.
Flags: needinfo?(bkelly)
Assignee

Comment 11

3 years ago
Attachment #8710325 - Attachment is obsolete: true
Attachment #8715694 - Flags: review?(bkelly)
Comment on attachment 8715694 [details] [diff] [review]
Async execute Activate after oninstall event. v2

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

This is very close, but I realized making this always async opens us up to a race with registration removal.  We should probably make the runnable contain the principal+scope and look up the registration again before performing the TryToActivate() logic.  If the registration is gone, then do nothing.  Sorry I didn't think of this early!

::: dom/workers/ServiceWorkerManager.cpp
@@ +1861,1 @@
>  ServiceWorkerRegistrationInfo::TryToActivate()

Can you add a comment to TryToActivate() that you should not call it directly and probably want TryToActivateAsync() instead?

@@ +1861,4 @@
>  ServiceWorkerRegistrationInfo::TryToActivate()
>  {
> +  if (!IsControllingDocuments() ||
> +      (mWaitingWorker && mWaitingWorker->SkipWaitingFlag())) {

On second thought, we probably need to make the async runnable take a principle+scope and look up the registration again.  I think there is a very slim possibility that the registration could be removed while the async activate runnable is queued.  We shouldn't activate in that case.
Attachment #8715694 - Flags: review?(bkelly) → review-
Assignee

Comment 13

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #12)
> @@ +1861,4 @@
> >  ServiceWorkerRegistrationInfo::TryToActivate()
> >  {
> > +  if (!IsControllingDocuments() ||
> > +      (mWaitingWorker && mWaitingWorker->SkipWaitingFlag())) {
> 
> On second thought, we probably need to make the async runnable take a
> principle+scope and look up the registration again.  I think there is a very
> slim possibility that the registration could be removed while the async
> activate runnable is queued.  We shouldn't activate in that case.

Thanks for review !
I thought when we do registration removal, we will always clear registration[1], so activate will not execute because there is no waiting worker[2].

[1]https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#395
[2]https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#1900
Flags: needinfo?(bkelly)
You're right!  I looked to see if we did that, but I only looked at what RemoveRegistration() does.  Ok, I guess we don't need the extra step.

Maybe include a comment saying that if the registration is removed, then its waiting worker will have been cleared?  And maybe we can add asserts in RemoveRegistration() somewhere that the workers are all null?

Thanks.
Flags: needinfo?(bkelly)
Assignee

Comment 15

3 years ago
- Add comment in TryToActivate & TryToActivateAsync
- Add MOZ_ASSERT in RemoveRegistration
Attachment #8715694 - Attachment is obsolete: true
Attachment #8719688 - Flags: review?(bkelly)
Comment on attachment 8719688 [details] [diff] [review]
Async execute Activate after oninstall event. v3

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

Thanks!
Attachment #8719688 - Flags: review?(bkelly) → review+
Also note that I'm touching the RemoveRegistration*() code in bug 1242482.  If that lands first you will need to rebase.  Since I'm moving the registration->Clear() to RemoveRegistration(), I think we can probably just lose the asserts I previously requested.
Assignee

Comment 18

3 years ago
(In reply to Ben Kelly [:bkelly] from comment #17)
> Also note that I'm touching the RemoveRegistration*() code in bug 1242482. 
> If that lands first you will need to rebase.  Since I'm moving the
> registration->Clear() to RemoveRegistration(), I think we can probably just
> lose the asserts I previously requested.

Got it, thanks!
Hi Dimi, I know this is a silly thing to ask for, but given that bug 1242482 is important for v47, and that :bkelly is on holidays until just before the branch point, could please wait for his patches to land first and rebase yours on top? Except if try rejects them or they get backed out of course.

Details: Bug 1242482 has the tracking-firefox47 flag because it's blocking a few SW/Devtools features that we're trying to ship for v47, plus we'd like to leave some time for testing/hardening.
Assignee

Comment 20

3 years ago
(In reply to Jan Keromnes [:janx] from comment #19)
> Hi Dimi, I know this is a silly thing to ask for, but given that bug 1242482
> is important for v47, and that :bkelly is on holidays until just before the
> branch point, could please wait for his patches to land first and rebase
> yours on top? Except if try rejects them or they get backed out of course.
> 
> Details: Bug 1242482 has the tracking-firefox47 flag because it's blocking a
> few SW/Devtools features that we're trying to ship for v47, plus we'd like
> to leave some time for testing/hardening.

No problem, that's exactly why i didn't mark this checkin-needed :)
(In reply to Dimi Lee[:dimi][:dlee] from comment #20)
> No problem, that's exactly why i didn't mark this checkin-needed :)

That's awesome, thanks a lot! Bug 1242482 was successfully merged to m-c yesterday and seems to stick \o/
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 24

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/87a62c4c3474
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Blocks: 1252055
Comment on attachment 8723344 [details] [diff] [review]
Async execute Activate after oninstall event.

Approval Request Comment
[Feature/regressing bug #]: Service workers.
[User impact if declined]: Web compatibility with chrome.  Some service worker sites may not work as expected without this patch.  This bug also blocks us from uplifting bug 1252055 as well.
[Describe test coverage new/current, TreeHerder]: Mochitests and web-platform-tests.
[Risks and why]: Minimal.  Only affects service workers and has been in nightly for a while now.
[String/UUID change made/needed]: None.
Attachment #8723344 - Flags: approval-mozilla-beta?
Comment on attachment 8723344 [details] [diff] [review]
Async execute Activate after oninstall event.

Web compat improvement for service workers
Please uplift to beta. This should end up in beta 2.
Attachment #8723344 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.