1227015 - Service Worker life cycle code should not use mutable shared state on registration across jobs

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

See this spec issue comment:

  https://github.com/slightlyoff/ServiceWorker/issues/789#issuecomment-158824120

Basically, the spec defines operations like this:

  1) Set the registration script URL to foo
  2) Queue an async task to do something with the registration (assuming script URL is foo)

So you can get:

  1) Do operation 1
    a) set registration script URL to foo
    b) queue operation 1 to run async
  2) Do operation 2
    a) set registration script URL to bar
    b) queue operation 2 to run async
  3) Operation 1 async runs, but oops, its using bar instead of foo

I think this is a case where the description of what chrome is doing went off the rails in translation to the spec.  Even if the spec says to do this, we should stop doing it in our code.  We should instead have job just use its specific mScriptURL.

Code that wants to read registration->mScriptURL should instead use newestWorker.scriptURL.  For example, the short circuit in the .register() call for doing nothing when registering the same script.

Then we should remove registration->mScriptURL because its a crazy race condition.

I've suggested spec changes to that affect in the linked spec issue.

What do other people think?

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

To further clarify, I think we should:

1) Make all ServiceWorkerRegisterJob code use its own mScriptURL
2) If the job is created with an empty mScriptURL, then populate it during Start() using newest worker's script URL.
3) Remove ServiceWorkerRegistrationInfo::mScriptURL.

I tried to describe this in spec language as well, here:

  https://github.com/slightlyoff/ServiceWorker/issues/789#issuecomment-158824120

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

This is a company issue, even if we think the spec is wrong here.  I doubt the spec accurately describes what chrome does.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Matt described Chrome's behavior on github:

https://github.com/slightlyoff/ServiceWorker/issues/789#issuecomment-159162097

I think thats where the spec is going, so I'm going to implement the same here.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Eddy, I need to remove the nsIServiceWorkerRegistrationInfo.scriptSpec attribute.  Will this conflict with your plans for the debugger?

Instead of using the script spec for the registration, we should really be using the script specs for the installing/waiting/active workers.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Create ServiceWorkerScriptJobBase as parent class to register and install jobs. r=ehsan

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Move mScriptSpec from registration job into script job base. r=ehsan

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Require a script spec on install jobs. r=ehsan

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Make register job always require an explicit script spec. r=ehsan

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Remove ServiceWorkerRegistrationInfo mScriptSpec. r=ehsan

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Abort updates if the script spec has changed. r=ehsan

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=66e5ab903357

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Remove ServiceWorkerRegistrationInfo mScriptSpec. r=ehsan

Updated to not fire a notification that the registration changed before the installing worker is set.  Now that we're not overriding the registration script spec nothing has changed at this point.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a39d7e2d618

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Try based on bug 1230164:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6924605dc7f

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Create ServiceWorkerScriptJobBase as parent class to register and install jobs. r=ehsan

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Move mScriptSpec from registration job into script job base. r=ehsan

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Require a script spec on install jobs. r=ehsan

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Make register job always require an explicit script spec. r=ehsan

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Remove ServiceWorkerRegistrationInfo mScriptSpec. r=ehsan

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Abort updates if the script spec has changed. r=ehsan

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

For now these patches keep nsIServiceWorkerRegistrationInfo.scriptSpec.  I simply return newest()->mScriptSpec in this case.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Try is green.  So this should be good to go once reviewed.

Comment 22

[Fernando Jiménez Moreno [:ferjm]]

It seems that this fixes the oranges introduced by bug 1225121.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb4554bd3bde
https://hg.mozilla.org/integration/mozilla-inbound/rev/c46b5abddec5
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bef51e9c8ae
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fb0c56916d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fc6fca28ddf
https://hg.mozilla.org/integration/mozilla-inbound/rev/03c28b711e1f

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Attachment: bug1227015_p7_errorresult.patch

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4faa4cf68052

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Back out of P7 for warning-as-error bustage:

  remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6d0f6df2eba7

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a267542e8cb7

Comment 28

[Phil Ringnalda (:philor)]

Backed out the whole lot in https://hg.mozilla.org/integration/mozilla-inbound/rev/dc33b155d02c

Good luck with landing this, because it's got bustification and makes the tests ungoodish. On the push where I was deciding to back you out, of the 10 linux64 debug w-p-t-e10s-8 runs in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=407e7946bee4&group_state=expanded&selectedJob=18396789&filter-searchStr=3f171212d95278fc9e091609125837ed695c0544 two were green, three were failures in fetch-frame-resource.https.html which I would just star as the normal too-frequent intermittent except that I knew from seeing it all day that you were making that fail much more frequently and more severely (two or three failures per run rather than the usual one), four were other serviceworker failures, and one maybe wasn't even you.

Your most troublesome suites are Linux mochitest-e10s-4, and Linux and Mac web-platform-tests-e10s, -4 on opt and -8 on debug, and I think you're going to have to not only retrigger until you see green, but also retrigger until you can say that you aren't either adding new intermittents or making existing intermittents more frequent.

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

I believe the issue is that wpt test ServiceWorkerGlobalScope/update.https.html has this code at the top level of its service worker script:

  // update() during the script evaluation should be ignored.
  self.registration.update();

We don't actually ignore update() calls at the top level of the script, so this starts an infinite async loop.  This loop then throws off the timing of all the other tests that follow it.  I think this only really triggers in e10s because of how the PropagateSoftUpdate() mechanism works there.

As far as I can tell this is not quite spec'd.  I'm testing a patch to implement ignoring these updates and I will write a spec issue.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37ca6ccce86a

The event source failures in m-e10s-4 still need investigation.

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Disable ServiceWorkerGlobalScope/update.https.html due to races. r=ehsan

The ServiceWorkerGlobalScope/update.https.html has multiple issues.  First, it triggers an infinite async loop with that sync update() call.  Even if I fix that, however, its still races.  Sometimes our navigation triggered update fires before the service worker script's update() call can run.  This confuses the test.

We need to disable this test until we delay the navigation update to post load.

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

I think this may just need to land in the same push with bug 1226443 in order to fix the update-on-navigate races.

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/84e011be4e35
https://hg.mozilla.org/integration/mozilla-inbound/rev/2090c1e30933
https://hg.mozilla.org/integration/mozilla-inbound/rev/03abf4d48e38
https://hg.mozilla.org/integration/mozilla-inbound/rev/244093d57c03
https://hg.mozilla.org/integration/mozilla-inbound/rev/78896c0bcb95
https://hg.mozilla.org/integration/mozilla-inbound/rev/e261f601b14d
https://hg.mozilla.org/integration/mozilla-inbound/rev/00ac71165014
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fbc71a2912a

Comment 33

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c873567c2898 - it's not going to be that easy.

Definite, no question about it increase in the frequency of failures in fetch-frame-resource.https.html, plus the unknown https://treeherder.mozilla.org/logviewer.html#?job_id=18479830&repo=mozilla-inbound and the unknown https://treeherder.mozilla.org/logviewer.html#?job_id=18477917&repo=mozilla-inbound, and that's just in a very few runs, not in the 20-30 that I would have triggered on try as just a starter if I were you.

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51d23f0cb661
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ebdba6df716
https://hg.mozilla.org/integration/mozilla-inbound/rev/066ffdf89e63
https://hg.mozilla.org/integration/mozilla-inbound/rev/d24d444fcec1
https://hg.mozilla.org/integration/mozilla-inbound/rev/2834622813f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/3632028dfc0c
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d7a3934b955
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0601cbf175f

Comment 35

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/51d23f0cb661
https://hg.mozilla.org/mozilla-central/rev/1ebdba6df716
https://hg.mozilla.org/mozilla-central/rev/066ffdf89e63
https://hg.mozilla.org/mozilla-central/rev/d24d444fcec1
https://hg.mozilla.org/mozilla-central/rev/2834622813f8
https://hg.mozilla.org/mozilla-central/rev/3632028dfc0c
https://hg.mozilla.org/mozilla-central/rev/8d7a3934b955
https://hg.mozilla.org/mozilla-central/rev/e0601cbf175f