Open Bug 1472303 Opened Last year Updated 22 days ago

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

Categories

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

enhancement

Tracking

()

People

(Reporter: bkelly, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: SW-MUST[wptsync upstream])

Attachments

(1 file)

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.
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.
Priority: -- → P2
Assignee: nobody → mrbkap
asuth: on your radar
Flags: needinfo?(bugmail)
Whiteboard: SW-MUST
Comment on attachment 8999657 [details]
Bug 1472303 - Block self-update from top level scripts. r=asuth

Andrew Sutherland [:asuth] has approved the revision.
Attachment #8999657 - Flags: review+
Pushed by bugmail@asutherland.org:
https://hg.mozilla.org/integration/autoland/rev/8a40d04dfcbb
Block self-update from top level scripts. r=asuth
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12633 for changes under testing/web-platform/tests
Whiteboard: SW-MUST → SW-MUST[wptsync upstream]
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://hg.mozilla.org/mozilla-central/rev/8a40d04dfcbb
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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
Maybe we should move this out of WPT, and add a WPT for update() throwing InvalidStateError?
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.
Flags: needinfo?(bugmail)
Assignee: mrbkap → nobody
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12676 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://hg.mozilla.org/mozilla-central/rev/ad4c1dca81f7
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Assignee: nobody → ytausky
Status: REOPENED → ASSIGNED
Where are we on this bug?
Flags: needinfo?(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.
Flags: needinfo?(ytausky)
I think we can try and land this now, then?
Flags: needinfo?(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.
Flags: needinfo?(ytausky)
Target Milestone: mozilla63 → ---
Assignee: ytausky → perry
Assignee: perry → nobody
Status: ASSIGNED → NEW
Priority: P2 → P3
You need to log in before you can comment on or make changes to this bug.