1472303 - ServiceWorkerRegistration::Update() should block self-update during evaluation regardless of Inner implementation

Status: NEW

(Core :: DOM: Service Workers, enhancement, P3)

Description

[Ben Kelly [:bkelly, not reviewing]]

We have code to block triggering an update() from script evaluation in a service worker:

https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/dom/serviceworkers/ServiceWorkerRegistrationImpl.cpp#781-787

Currently this is in the legacy ServiceWorkerRegistrationImpl, so it doesn't take effect in the RemoteServiceWorkerRegistrationImpl case.  We should move this check to ServiceWorkerRegistration instead.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Probably the best way to do this would be to make ServiceWorkerGlobalScope QI'able.  Then we could just QI the global and check if ServiceWorkerGlobalScope::Registration() == this.  And also then GetCurrentThreadWorkerPrivate() could be called to check if the script is in top level evaluation.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1472303 - Block self-update from top level scripts. r=asuth

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 4

[Marion Daly [:mdaly]]

asuth: on your radar

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8999657 [details]
Bug 1472303 - Block self-update from top level scripts. r=asuth

Andrew Sutherland [:asuth] has approved the revision.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a40d04dfcbb
Block self-update from top level scripts. r=asuth

Comment 7

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12633 for changes under testing/web-platform/tests

Comment 8

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 9

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/8a40d04dfcbb

Comment 10

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 11

[Matt Falkenhagen]

Chrome is timing out on the WPT test added here.

The update() call is failing with an InvalidStateError. I think this aligns with the spec at https://w3c.github.io/ServiceWorker/#service-worker-registration-update: if the state is installing, reject the promise with InvalidStateError.

Also I don't think this part is valid:

  reg.addEventListener("updatefound",
    () => assert_unreached("shouldn't find an update"));

The spec says UAs can terminate long running workers, but it would still be spec-conformant to find an update. (Chrome allows some updates before blocking the call.)

I'm not sure how to change this test. We could let the service worker reach active, then give it a message to try self-update, but this bug seems about update at top-level.

I think you'd have to install the service worker once, use some implementation specific way to stop the worker, then start it again.

Not sure WPT would be possible here. Chrome has a similar test for self-updating workers at:
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/tests/serviceworker/update-no-controllee.https.html

Comment 12

[Matt Falkenhagen]

Maybe we should move this out of WPT, and add a WPT for update() throwing InvalidStateError?

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

Sorry about that, and thank you for responding here on Bugzilla!

There's a bunch of interesting stuff going on here:
- Firefox's handling of top-level update() by resolving with the registration was wrong before and after this patch.  Our implementation assumed the proposal at the top of https://github.com/w3c/ServiceWorker/issues/800 rather than what actually landed.  Since the test tests for this, it's wrong.
- There's an interesting edge-case where I believe the worker is actually in the "parsed" state rather than the installing state.
  - Specifically, I think the worker's state at the top-level should actually be "parsed" since the evaluation should occur as part of Step 16 of https://w3c.github.io/ServiceWorker/#update-algorithm and its "Run Service Worker" invocation.  The state won't be set to installing until the "Update" algorithm invokes the "Install" algorithm.
- During this parsed state for an initial install, beyond state="parsed", we also expect "Get Newest Worker" to return null.  I believe this is what should be causing the InvalidStateError rejection in this case, which is good, because the issue 800 check won't actually fire here because the state is not "installing" yet.
- Indeed, the updatefound check is a little dubious in the face of successful installation.  The test example :bkelly raised in issue 800 included a failure to install.

I'm going to back this out from mozilla-central which should hopefully propagate back up via the sync mechanism.

Thank you also for citing a related chromium test to check out!

(In reply to Matt Falkenhagen from comment #12)
> Maybe we should move this out of WPT, and add a WPT for update() throwing
> InvalidStateError?

Yes, it would be good to make sure we have branch coverage for the cases where update() will reject without dispatching a job.  Firefox can do this when we get back around to this bug.

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad4c1dca81f7c484d7ced574b50e2a44fb5738d6
Bug 1472303 - Backed out changeset 8a40d04dfcbb. r=asuth

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12676 for changes under testing/web-platform/tests

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 17

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/ad4c1dca81f7

Comment 18

[Matt Falkenhagen]

Thanks for the detailed response! Yes, I think you're right that it's parsed rather than installing. I jumped to conclusions there about why we were returning InvalidStateError.

Comment 19

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 20

[Marion Daly [:mdaly]]

Where are we on this bug?

Comment 21

[Yaron Tausky [:ytausky]]

It spawned bug 1488792 to fix the part where we weren't compliant with the spec; that bug has a patch that's waiting for review. Andrew and I discussed this and I think we came to the conclusion that it's best to declare it done after that bug is fixed, since testing proved to be problematic. I don't remember the details though, maybe it's worth it to revisit that conclusion.

Comment 22

[Andrew Sutherland [:asuth] (he/him)]

I think we can try and land this now, then?

Comment 23

[Yaron Tausky [:ytausky]]

It still requires a Mochi test, I think. We can probably use the console message about shutting down a service worker to ensure that it is indeed getting started again while being in the activated state.

Comment 24

[Jens Stutte [:jstutte]]

Moved this to block bug 1231208 in order to close bug 1462460.