1206947 - Loads initiated from workers should be intercepted by worker's controlling service worker

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

It is possible for a document that is not controlled to create a worker which is controlled, since workers are non-subresource loads that match the worker's URL against the list of SW registrations. Bug 1184798 fixes that.

network requests from a worker (importScripts, fetch, XHR) currently fall back to the worker's document (since worker interface requestor returns the parent document for a nsINetworkInterceptController). In the above case this will mean that a worker's subresources do not hit the SW but go directly to the network. This bug is to fix things so that workers act as true controllers.

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Setting this to block v1, but it isn't critical unless lack of this leads to treeherder failures on Bug 1184798 (just a lack of features and not a security bug or something).

Comment 2

[(no longer active)]

(In reply to Nikhil Marathe [:nsm] (please needinfo?) from comment #1)
> Setting this to block v1, but it isn't critical unless lack of this leads to
> treeherder failures on Bug 1184798 (just a lack of features and not a
> security bug or something).

It looks like that patch stuck?

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1206947 - All workers should have their own loadgroup. r?khuey,bkelly

Bug 1206947 - All workers should have their own loadgroup. r?khuey,bkelly

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1206947 - WIP Interception support.

Bug 1206947 - WIP Interception support.

This patch adds support for interceptions from workers, but parts of it are hacky.

- Fixes ScriptLoader to use TYPE_SCRIPT for importScripts() and the correct
  worker type for worker script.
- Adds an extra param to PrepareFetchEvent to pass the worker URI instead of
  a document. Obviously this is terrible. PrepareFetchEvent should be
  overloaded or similar.
- Copies FetchEventDispatcher to WorkerPrivate, but this should be refactored with nsDocShell.
- WorkerNetworkInterceptController implements correct interception logic for both subresource and non-subresource loads

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

These are only the minimum changes to get interceptions working. In addition we need to do the following to comply with the spec:
- Workers should act as true 'clients' and be added to the set of controlled documents (now controlled clients) in ServiceWorkerManager. Worker initialization and shutdown should add and remove itself from this set. This ensures we follow the SW activation rules for when controlled clients come and go.
- All potentially controlled workers must also add themselves to SWM just like all documents do. This allows clients.claim() and clients.matchAll() to work.

Ancillary
- implement postMessage from SW to worker and back.
- expose ServiceWorkerRegistration and the ServiceWorkerContainer APIs on workers.

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Also, please test for:
importScripts, fetch(), XHR go through worker's controller and get intercepted
nested workers act like non-subresource requests and must check controller against their own URL
Make sure to test both Dedicated and SharedWorkers

Comment 8

[(no longer active)]

Kyle, is this still on your plate?  Do you have WIP patches?

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I'm not actively working on it, and I don't have any code written.  I do have plans to farm this out to someone at some point this quarter or next.  Is this a priority for you?

Comment 10

[(no longer active)]

No, just wanted to see what the current status is.

If you have plans on how this should work, please post them here when you have a chance!

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

FYI, we can also trigger the conditions in comment 0 if a page is reloaded with shift-ctrl-r.  Since the service worker is bypassed and the document is not controlled treat the worker as not controlled as well.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

We need bug 1032521 for this in order to properly populate FetchEvent.clientId, etc.

I spoke with Kyle on IRC and he's not currently working this.

Comment 13

[gareth.aye]

Just to add a test case here, https://latexbase.com uses a service worker that needs to intercept worker requests. This works in Blink but does not in Firefox (at least 48 and below). The service worker is disabled by default in Firefox (since it doesn't work) but you can add /forceServiceWorker=1 to turn it on and observe the breakage. What's interesting is that, when I tried to pare down my issue to a minimal test case here https://github.com/gaye/bug-1206947, both Chrome and Firefox did the right thing with worker requests.

Comment 14

[gareth.aye]

Er /?forceServiceWorker=1 above. Still can't edit bugzilla comments. Rahhhhhh

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

This was fixed by bug 1231211.