Closed Bug 1452528 Opened 6 years ago Closed 6 years ago

fetch() from shared worker clients don't seem to get intercepted by the controller if they are too "early"

Categories

(Core :: DOM: Service Workers, defect, P2)

61 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: falken, Assigned: bkelly)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36

Steps to reproduce:

Tested on Nightly 61.0a1 (2018-04-08) (64-bit) for Linux.

Firefox seems to correctly have the service worker intercept fetch() requests from the shared worker. But if the fetch() happens too early, the fetch goes to network.

In https://chromium-review.googlesource.com/c/chromium/src/+/999241 I'm adding WPT for this behavior after redirects. Firefox passes the tests when the setup is:

In shared worker:
self.onconnect = function(e) {
  ...
  port.addEventListener('message', function(e) {    
    fetch(target).then(r => r.text()).then(result => port.postMessage('fetch(): ' + result));
  });
};

On page:
port.postMessage('ping')

But if the postMessage ping/pong is removed, and the shared worker just does:

self.onconnect = function(e) {
  ...
  fetch(target).then(r => r.text()).then(result => port.postMessage('fetch(): ' + result));
};

The service worker doesn't intercept the fetch.

You can kind of see this live in https://sw-shared-worker.glitch.me/ (but I keep changing the test at that link, sorry). If I do both :
self.onconnect = function(e) {
  fetch('/hello').then(r => r.text()).then(result => port.postMessage('fetched (first): ' + result));  
  port.addEventListener('message', function(e) {    
    fetch('/hello').then(r => r.text()).then(result => port.postMessage('fetched (ping): ' + result));
  });
};

The result for "Start shared worker (sw1 network-redirect to sw2 scope)" then clicking "Ping shared worker is" is:

msg from worker: fetched (first): hello from the server
msg from worker: fetched (ping): the shared worker is controlled by sw1

(I think you could probably change https://sw-shared-worker-wanderview.glitch.me/ for "Start shared worker (sw redirect)" to see this.)
Thanks for filing this.

I would have to investigate, but my guess is this might be due to our worker code still trying to use a docshell for its nsIChannel callbacks.  The workers should really be creating their own ServiceWorkerInterceptController, etc.

I guess this does not effect dedicated workers.  Over here we have a test that does a fetch immediately in a dedicated worker and it seems to work:

https://searchfox.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/worker-client-id.https.html
Thanks, also we have a shared worker test at:
https://searchfox.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/shared-worker-controlled.https.html

With an immediate fetch (actually XHR, but some testing showed fetch vs XHR doesn't make a difference) which makes me think the failure has something to do with the redirect.
Oh, indeed.  I bet its this code:

https://searchfox.org/mozilla-central/rev/7ccb618f45a1398e31a086a009f87c8fd3a790b6/dom/clients/manager/ClientChannelHelper.cpp#128-140

I even filed a spec issue about this:

https://github.com/w3c/ServiceWorker/issues/1239

I think this is probably the same thing you ran into and we decided it just keeps the first controller.  Right?

In that case I need to make this code only clear the controller if its a document load non-subresource load instead of all non-subresource loads.
Flags: needinfo?(falken)
Oh wow, yes that spec issue is exactly what I later filed in https://github.com/w3c/ServiceWorker/issues/1289.

I'm a bit confused though. My testing is showing Firefox has the controller operational for fetch() after a small delay (inside onmessage), but not operational for fetch() if it's too soon after startup (inside onconnect). Does that change your theory?

The WPT is just written as the spec says, so for the network-redirect to out-of-scope case, it expects that sw1 is the controller for the worker.
Flags: needinfo?(falken)
(In reply to Matt Falkenhagen from comment #4)
> Oh wow, yes that spec issue is exactly what I later filed in
> https://github.com/w3c/ServiceWorker/issues/1289.

Sorry I forgot I filed it!

> I'm a bit confused though. My testing is showing Firefox has the controller
> operational for fetch() after a small delay (inside onmessage), but not
> operational for fetch() if it's too soon after startup (inside onconnect).
> Does that change your theory?

No, it makes sense.  In firefox we actually communicate back the controller to the client in two ways:

1. A manager object maintains a list of clients and sends an IPC message asynchronously to update the controlled state.  This is followed whenever we control a client.  For example, clients.claim() or a new client that is first intercepted.
2. We also tag the network channel with the controller when its for a non-subresource load.  That way we can synchronously have the controller ready as soon as the client is execution ready.

The code I linked to in comment 3 wipes the network channel controller for method (2) here.  Method (1), though, still happily sends the controller through the async IPC path.  So it ends up getting controlled, but just a little later.

> The WPT is just written as the spec says, so for the network-redirect to
> out-of-scope case, it expects that sw1 is the controller for the worker.

Sounds good!
Flags: needinfo?(bkelly)
I needed a break from my other work so I fixed this.  It only took a short amount of time since Matt's WPT test is now in our tree.
Assignee: nobody → bkelly
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(bkelly)
Andrea, this fixes an issue with worker scripts that both match a service worker scope and redirect.  They need to function differently than document loads because the spec treats them differently.  Document loads have a manual redirect mode which causes them to re-enter all service workers along the redirect path.  In contrast, workers stopping consulting service workers after the first FetchEvent handler that does not call respondWith().

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e392085bb06457c4b908e49bb78b6fba5564885
Attachment #8968612 - Flags: review?(amarchesini)
Attachment #8968612 - Attachment description: bug1452528_workerredirect.patch → Don't clear the controller on non-subresource channel loads when redirect mode is "follow". r=baku
Attachment #8968612 - Flags: review?(amarchesini) → review+
Priority: -- → P2
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4fd09d03e9d
Don't clear the controller on non-subresource channel loads when redirect mode is "follow". r=baku
https://hg.mozilla.org/mozilla-central/rev/b4fd09d03e9d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.