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

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 1

[Dimi Lee [:dimi]]

Attachment: WIP. testcase

Comment 2

[Dimi Lee [:dimi]]

running try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=926b1d19d1c2

Comment 3

[Dimi Lee [:dimi]]

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

Comment 4

[Dimi Lee [:dimi]]

Attachment: Async execute Activate after oninstall event

Hi Ben,
I remove the assert of DispatchFetchEvent because of Comment 3 first scenario.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

I wrote this spec issue for the skipWaiting thing:

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

Comment 7

[Dimi Lee [:dimi]]

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.

Comment 8

[Dimi Lee [:dimi]]

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

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

I agree.  The test seems to be wrong.

Also, fwiw, I would recommend using the github spec instead of the TR.

Comment 10

[Dimi Lee [:dimi]]

try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=8837ce2ade5c&selectedJob=16257027

Comment 11

[Dimi Lee [:dimi]]

Attachment: Async execute Activate after oninstall event. v2

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 13

[Dimi Lee [:dimi]]

(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

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 15

[Dimi Lee [:dimi]]

Attachment: Async execute Activate after oninstall event. v3

- Add comment in TryToActivate & TryToActivateAsync
- Add MOZ_ASSERT in RemoveRegistration

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8719688 [details] [diff] [review]
Async execute Activate after oninstall event. v3

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

Thanks!

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 18

[Dimi Lee [:dimi]]

(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!

Comment 19

[Jan Keromnes [:janx]]

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.

Comment 20

[Dimi Lee [:dimi]]

(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 :)

Comment 21

[Jan Keromnes [:janx]]

(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/

Comment 22

[Dimi Lee [:dimi]]

Attachment: Async execute Activate after oninstall event.

rebase
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=45c582c48a7a&selectedJob=17141098

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87a62c4c3474

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/87a62c4c3474

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Beta 46 patch

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 28

[Carsten Book [:Tomcat]]

 https://hg.mozilla.org/releases/mozilla-beta/rev/352f811d9d68