-
Notifications
You must be signed in to change notification settings - Fork 312
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SharedWorker script interception after redirects #1289
Comments
The firefox behavior where the script is served from network is a bug in FF59 and earlier. AFAICT it has been fixed in FF60+ which is in beta at the moment. I believe chrome not updating |
I think if the worker client's creation URL does not reflect the redirect, then that is a bug in the spec that should be fixed. Note that the final fetched URL is used by workers in step 13.3 here: |
Also, AIUI the creation URL is set on the worker client when it becomes execution ready after the initial script fetch is completed. So I don't see how it could be used for scope matching. |
The worker client is set execution ready at step 24 here: https://html.spec.whatwg.org/#worker-processing-model I think the client creation URL should be left blank until execution ready to match window client behavior. |
Thanks wanderview. I agree the desired behavior is to intercept each redirect and for the self.location to reflect the final URL (regardless of service workers), and probably the spec needs fixing. Testing https://sw-shared-worker.glitch.me/ on Firefox Nightly 61.0a1 (2018-03-19) seems to show the redirect bug still occurs. The shared worker script is served from network, but the service worker still controls fetch() from the shared worker. Test by clicking Register, then Start, then Ping. The shared worker code is basically:
If it's from the network, with "SERVED by the sw" added to the first message if it's from the service worker. The second message indicates whether the fetch() was intercepted by the service worker (it says "hello from service worker" or "hello from server"). Chrome output:
Firefox output:
|
@mattto Sorry for my confusion. I saw a difference in other behavior, so I thought it accounted for this. But... I'm not sure the that each redirect should be intercepted according to the spec in this case. Worker scripts are not given a manual redirect mode AFAIK. So after the first redirect it should get "service-worker mode" set to "none" in step 4.2 here: https://fetch.spec.whatwg.org/#http-fetch Does that make sense or am I still confused? I have no idea what is desirable from a web developers perspective, though. |
Interesting, yes I think you're right. https://html.spec.whatwg.org/multipage/webappapis.html#fetch-a-classic-script doesn't mention anything about redirect mode. Stepping back, there are two issues:
If I'm understanding correctly, it looks like the current spec is saying:
Chrome's behavior is:
Firefox's behavior is:
Regarding desirable behavior, it seems intuitive if non-subresource requests like new SharedWorker() have the same interception and matching behavior as navigation requests. Particularly, it seems a bit weird if a service worker doesn't get a fetch event for the request that results in its controlled client. To change the spec to make worker scripts behave like navigations, would seem to require:
But I'm not sure what the implications would be of doing this, e.g., giving worker scripts a manual redirect mode. cc @annevk @domenic : do you know if this is a deliberate difference between navigations and other non-subresource requests (i.e., redirects are intercepted by service worker for navigations but not for worker scripts)? |
I don't think this was considered. This also applies to dedicated workers. I don't see any immediate issues with switching to "manual", other than some added complexity. |
I don't think your test is adequate to determine (2). AFAICT both the initial and final URL are covered by the service worker's scope. |
Ahh, yes you're right @wanderview . I forgot to add {scope} to the register() call. I'll try that and report back. |
I actually just made a demo for it. Firefox uses the last service worker that triggers a fetch event (or would have triggered if there is no fetch handler): |
I have a bug in my demo that chrome is hitting, though. |
After fixing the bug I can see:
|
BTW, it looks to me like the spec does use the request URL (not the client creation URL) to set the controller. See step 12.6 in Handle Fetch: https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm It explicitly sets the active controller for non-subresource requests. This is what firefox is currently trying to do as well. |
The main downside is that it means worker script loading now has to handle redirects out-of-fetch like navigation right? It would be nice not to expand that. |
Presumably we could create an equivalent solution inside of Fetch since these redirects are much more constrained. Not sure what ends up being better. |
Yea, doing something seems reasonable. It is really weird that a no-fetch service worker for scope1 in this case ends up controlling a script served over the network from scope2. |
I'm happy to take a look at what the best fix for the specification would be, unless someone wants to volunteer, but I'd like to wait until tests are written and it's clear implementers will follow through. We already have a number of weird interop issues with workers and Fetch also has a number of outstanding PRs waiting on implementers to take action, so I'd rather not add to that if there's still uncertainty. |
While the current spec is a bit weird with redirects, I don't think it's critical to make a change quickly here. I'm happy to conform to the current spec until we can sort this out at a f2f meeting. Maybe tpac 2018. |
Thanks wanderview for fixing my flawed test. I notice now that the Fetch spec treats redirects from service worker vs network differently also. Is the below summary now accurate? The spec says:
Assuming network redirects only, that means the spec says:
Chrome does this:
Firefox does what the spec says. So assuming network redirects only, it does (as above):
I briefly tested service worker generated redirects and Firefox seems to treat those the same as network redirects, but I may be missing something. |
Thanks, it seems we should remove some prose from 2.4 Selection and Use https://w3c.github.io/ServiceWorker/#selection or make it non-normative text, as setting a controller seems covered explicitly by Handle Fetch. |
Can you share what you tested here? I tried to reproduce, but can't. Note, please test with FF60+ as this code has changed recently. FF59 and earlier did not properly control the shared worker at all. Only the initial script load was intercepted, but not subresource requests. My test is here: https://sw-shared-worker-wanderview.glitch.me/ Click register and then "start shared worker (sw redirect)". This will do a In the console I see that the shared worker ends up getting handled by the scope 2 service worker.
I think this is correct. Even though the response returned from the service worker is a redirect status code, its still a non-null response so we don't set the service worker mode to none. |
I guess this is mostly true, but I'm not sure "from network" is the main determinant. The spec says to bypass the service worker on any redirect if the service worker does not call
Agree.
I think you have to assume a fall back service worker that does not call respondWith() for this part to be true. If the service worker converts the request to manual or does something else that produces a Response via respondWith() then I don't think these statements hold. |
Thanks! I agree with all the above. I think my test environment was flawed and I see the behavior you mention now. |
This tests behavior discussed here: w3c/ServiceWorker#1289 Namely it tests when a request for a worker goes through a redirect chain: 1) On redirect from A -> B, whether the service worker at B sees the request. 2) After the final redirect, which service worker controls the resulting client. The tests are written as specified today. Therefore, Firefox passes this test (verified in Nightly) and Chrome does not. Currently it only tests shared worker but dedicated worker can be added in a follow-up patch. Bug: 731604, 724371 Change-Id: Id3b1ea8b952760be0ef9917f2c6a3afe60ca1fb5
This tests behavior discussed here: w3c/ServiceWorker#1289 Namely it tests when a request for a worker goes through a redirect chain: 1) On redirect from A -> B, whether the service worker at B sees the request. 2) After the final redirect, which service worker controls the resulting client. The tests are written as specified today. Therefore, Firefox passes this test (verified in Nightly) and Chrome does not. (Actually a small change is required to the test to make Firefox pass it, see: https://bugzilla.mozilla.org/show_bug.cgi?id=1452528) Currently it only tests shared worker but dedicated worker can be added in a follow-up patch. Bug: 829720 Change-Id: Id3b1ea8b952760be0ef9917f2c6a3afe60ca1fb5
This tests behavior discussed here: w3c/ServiceWorker#1289 Namely it tests when a request for a worker goes through a redirect chain: 1) On redirect from A -> B, whether the service worker at B sees the request. 2) After the final redirect, which service worker controls the resulting client. The tests are written as specified today. Therefore, Firefox passes this test (verified in Nightly) and Chrome does not. (Actually a small change is required to the test to make Firefox pass it, see: https://bugzilla.mozilla.org/show_bug.cgi?id=1452528) Currently it only tests shared worker but dedicated worker can be added in a follow-up patch. Bug: 829720 Change-Id: Id3b1ea8b952760be0ef9917f2c6a3afe60ca1fb5
This tests behavior discussed here: w3c/ServiceWorker#1289 Namely it tests when a request for a worker goes through a redirect chain: 1) On redirect from A -> B, whether the service worker at B sees the request. 2) After the final redirect, which service worker controls the resulting client. The tests are written as specified today. Therefore, Firefox passes this test (verified in Nightly) and Chrome does not. (Actually a small change is required to the test to make Firefox pass it, see: https://bugzilla.mozilla.org/show_bug.cgi?id=1452528) Currently it only tests shared worker but dedicated worker can be added in a follow-up patch. Bug: 829720 Change-Id: Id3b1ea8b952760be0ef9917f2c6a3afe60ca1fb5
This tests behavior discussed here: w3c/ServiceWorker#1289 Namely it tests when a request for a worker goes through a redirect chain: 1) On redirect from A -> B, whether the service worker at B sees the request. 2) After the final redirect, which service worker controls the resulting client. The tests are written as specified today. Therefore, Firefox passes this test (verified in Nightly) and Chrome does not. (Actually a small change is required to the test to make Firefox pass it, see: https://bugzilla.mozilla.org/show_bug.cgi?id=1452528) Currently it only tests shared worker but dedicated worker can be added in a follow-up patch. Bug: 829720 Change-Id: Id3b1ea8b952760be0ef9917f2c6a3afe60ca1fb5
This tests behavior discussed here: w3c/ServiceWorker#1289 Namely it tests when a request for a worker goes through a redirect chain: 1) On redirect from A -> B, whether the service worker at B sees the request. 2) After the final redirect, which service worker controls the resulting client. The tests are written as specified today. Therefore, Firefox passes this test (verified in Nightly) and Chrome does not. (Actually a small change is required to the test to make Firefox pass it, see: https://bugzilla.mozilla.org/show_bug.cgi?id=1452528) Currently it only tests shared worker but dedicated worker can be added in a follow-up patch. Bug: 829720 Change-Id: Id3b1ea8b952760be0ef9917f2c6a3afe60ca1fb5 Reviewed-on: https://chromium-review.googlesource.com/999241 Commit-Queue: Matt Falkenhagen <[email protected]> Reviewed-by: Hiroki Nakagawa <[email protected]> Cr-Commit-Position: refs/heads/master@{#549125}
This tests behavior discussed here: w3c/ServiceWorker#1289 Namely it tests when a request for a worker goes through a redirect chain: 1) On redirect from A -> B, whether the service worker at B sees the request. 2) After the final redirect, which service worker controls the resulting client. The tests are written as specified today. Therefore, Firefox passes this test (verified in Nightly) and Chrome does not. (Actually a small change is required to the test to make Firefox pass it, see: https://bugzilla.mozilla.org/show_bug.cgi?id=1452528) Currently it only tests shared worker but dedicated worker can be added in a follow-up patch. Bug: 829720 Change-Id: Id3b1ea8b952760be0ef9917f2c6a3afe60ca1fb5 Reviewed-on: https://chromium-review.googlesource.com/999241 Commit-Queue: Matt Falkenhagen <[email protected]> Reviewed-by: Hiroki Nakagawa <[email protected]> Cr-Commit-Position: refs/heads/master@{#549125}
This tests behavior discussed here: w3c/ServiceWorker#1289 Namely it tests when a request for a worker goes through a redirect chain: 1) On redirect from A -> B, whether the service worker at B sees the request. 2) After the final redirect, which service worker controls the resulting client. The tests are written as specified today. Therefore, Firefox passes this test (verified in Nightly) and Chrome does not. (Actually a small change is required to the test to make Firefox pass it, see: https://bugzilla.mozilla.org/show_bug.cgi?id=1452528) Currently it only tests shared worker but dedicated worker can be added in a follow-up patch. Bug: 829720 Change-Id: Id3b1ea8b952760be0ef9917f2c6a3afe60ca1fb5 Reviewed-on: https://chromium-review.googlesource.com/999241 Commit-Queue: Matt Falkenhagen <[email protected]> Reviewed-by: Hiroki Nakagawa <[email protected]> Cr-Commit-Position: refs/heads/master@{#549125}
…n of workers after redirects., a=testonly Automatic update from web-platform-testsservice worker: Add tests for inteception of workers after redirects. This tests behavior discussed here: w3c/ServiceWorker#1289 Namely it tests when a request for a worker goes through a redirect chain: 1) On redirect from A -> B, whether the service worker at B sees the request. 2) After the final redirect, which service worker controls the resulting client. The tests are written as specified today. Therefore, Firefox passes this test (verified in Nightly) and Chrome does not. (Actually a small change is required to the test to make Firefox pass it, see: https://bugzilla.mozilla.org/show_bug.cgi?id=1452528) Currently it only tests shared worker but dedicated worker can be added in a follow-up patch. Bug: 829720 Change-Id: Id3b1ea8b952760be0ef9917f2c6a3afe60ca1fb5 Reviewed-on: https://chromium-review.googlesource.com/999241 Commit-Queue: Matt Falkenhagen <[email protected]> Reviewed-by: Hiroki Nakagawa <[email protected]> Cr-Commit-Position: refs/heads/master@{#549125} wpt-commits: 6fe36d79072d5261ea504435b0dfedaf39f5805a wpt-pr: 10340 wpt-commits: 6fe36d79072d5261ea504435b0dfedaf39f5805a wpt-pr: 10340
…n of workers after redirects., a=testonly Automatic update from web-platform-testsservice worker: Add tests for inteception of workers after redirects. This tests behavior discussed here: w3c/ServiceWorker#1289 Namely it tests when a request for a worker goes through a redirect chain: 1) On redirect from A -> B, whether the service worker at B sees the request. 2) After the final redirect, which service worker controls the resulting client. The tests are written as specified today. Therefore, Firefox passes this test (verified in Nightly) and Chrome does not. (Actually a small change is required to the test to make Firefox pass it, see: https://bugzilla.mozilla.org/show_bug.cgi?id=1452528) Currently it only tests shared worker but dedicated worker can be added in a follow-up patch. Bug: 829720 Change-Id: Id3b1ea8b952760be0ef9917f2c6a3afe60ca1fb5 Reviewed-on: https://chromium-review.googlesource.com/999241 Commit-Queue: Matt Falkenhagen <falkenchromium.org> Reviewed-by: Hiroki Nakagawa <nhirokichromium.org> Cr-Commit-Position: refs/heads/master{#549125} wpt-commits: 6fe36d79072d5261ea504435b0dfedaf39f5805a wpt-pr: 10340 wpt-commits: 6fe36d79072d5261ea504435b0dfedaf39f5805a wpt-pr: 10340 UltraBlame original commit: 4d895b739d93c3f042cb91a6ab5c071939a13252
…n of workers after redirects., a=testonly Automatic update from web-platform-testsservice worker: Add tests for inteception of workers after redirects. This tests behavior discussed here: w3c/ServiceWorker#1289 Namely it tests when a request for a worker goes through a redirect chain: 1) On redirect from A -> B, whether the service worker at B sees the request. 2) After the final redirect, which service worker controls the resulting client. The tests are written as specified today. Therefore, Firefox passes this test (verified in Nightly) and Chrome does not. (Actually a small change is required to the test to make Firefox pass it, see: https://bugzilla.mozilla.org/show_bug.cgi?id=1452528) Currently it only tests shared worker but dedicated worker can be added in a follow-up patch. Bug: 829720 Change-Id: Id3b1ea8b952760be0ef9917f2c6a3afe60ca1fb5 Reviewed-on: https://chromium-review.googlesource.com/999241 Commit-Queue: Matt Falkenhagen <falkenchromium.org> Reviewed-by: Hiroki Nakagawa <nhirokichromium.org> Cr-Commit-Position: refs/heads/master{#549125} wpt-commits: 6fe36d79072d5261ea504435b0dfedaf39f5805a wpt-pr: 10340 wpt-commits: 6fe36d79072d5261ea504435b0dfedaf39f5805a wpt-pr: 10340 UltraBlame original commit: 4d895b739d93c3f042cb91a6ab5c071939a13252
…n of workers after redirects., a=testonly Automatic update from web-platform-testsservice worker: Add tests for inteception of workers after redirects. This tests behavior discussed here: w3c/ServiceWorker#1289 Namely it tests when a request for a worker goes through a redirect chain: 1) On redirect from A -> B, whether the service worker at B sees the request. 2) After the final redirect, which service worker controls the resulting client. The tests are written as specified today. Therefore, Firefox passes this test (verified in Nightly) and Chrome does not. (Actually a small change is required to the test to make Firefox pass it, see: https://bugzilla.mozilla.org/show_bug.cgi?id=1452528) Currently it only tests shared worker but dedicated worker can be added in a follow-up patch. Bug: 829720 Change-Id: Id3b1ea8b952760be0ef9917f2c6a3afe60ca1fb5 Reviewed-on: https://chromium-review.googlesource.com/999241 Commit-Queue: Matt Falkenhagen <falkenchromium.org> Reviewed-by: Hiroki Nakagawa <nhirokichromium.org> Cr-Commit-Position: refs/heads/master{#549125} wpt-commits: 6fe36d79072d5261ea504435b0dfedaf39f5805a wpt-pr: 10340 wpt-commits: 6fe36d79072d5261ea504435b0dfedaf39f5805a wpt-pr: 10340 UltraBlame original commit: 4d895b739d93c3f042cb91a6ab5c071939a13252
Suppose '/redirect' redirects to '/in-scope/shared-worker.js', and the page does new SharedWorker('/redirect'). And there is a service worker with scope 'in-scope'.
In Chrome today, you end up with a shared worker script served by the service worker.
In Firefox today, you end up with a shared worker script served from network from 'in-scope/shared-worker.js' .
Also, subresource requests from the shared worker are intercepted by the service worker for both browsers. So I think the implementation intent is for the initial request and each redirect to look up the appropriate service worker to intercept that URL, and finally to make the SharedWorker client be controlled if the final URL was in-scope.
I have a basic test at https://sw-shared-worker.glitch.me/ showing this.
Somewhat related,
self.location
in Chrome is 'redirect' and 'in-scope/shared-worker.js' in Firefox.The spec seems to say to intercept only for the initial request URL, because service worker registration matching is based on "Creation URL" in 2.4 Selection and Use: "That is, the service worker client attempts to consult a service worker registration whose scope url matches its creation URL." Creation URL is a concept from HTML. It looks like Creation URL is set at set up a worker environment settings object, which is called from "run a worker" in Step 9, which is before any network fetch which happens in Step 13.
I've not really tested other redirect hooks like AppCache.
I was looking at this for our new "servicified" service worker architecture. I'm not very motivated to invest a lot of time in this since Shared Workers don't have much usage or browser support, but will try to implement something reasonable.
The text was updated successfully, but these errors were encountered: