Bug 1222008 attempted to implement propagating the final response URL through a service worker interception to the outer response. Anne found, however, that this doesn't seem to be working right. A fetch() that is intercepted seems to still use the request.url for the final response.url. This is captured in this glitch: https://sw-fetch-final-response-url.glitch.me/ The web console should display: ===> Window got response.url: https://sw-fetch-final-response-url.glitch.me/controlled/replaced But on firefox it currently displays: ===> Window got response.url: https://sw-fetch-final-response-url.glitch.me/controlled/test Interestingly, if the service worker intercepts with a redirected fetch() then the final response URL is propagated. This is seen in this console output: ===> Window got redirected response.url: https://sw-fetch-final-response-url.glitch.me/controlled/replaced This probably means the dom/fetch code is not looking for URL changes on internal redirects. The service worker only does a full redirect if the synthesized response has .redirected set to true. And here is probably the conditional that needs to be removed: https://searchfox.org/mozilla-central/rev/e126996d9b0a3d7653de205898517f4f5b632e7f/dom/fetch/FetchDriver.cpp#1285

Well, it may be more complicated than that. Not sure if the mRequest->AddURL() is strictly right for the synthetic response URL case. That will result in the final Response.redirected always being true when intercepted by the service worker. In the case of the internal redirect you probably need to replace the request's URL list with the nsIChannel's final URL.

(https://github.com/web-platform-tests/wpt/pull/12680 tests this, though also tests https://github.com/whatwg/fetch/pull/696 so might not be useful for fixing this in isolation.)

The behavior looks interesting to me... It seems mRequest->AddURL() doesn't work fine since the result in FF for "redirected response.url" is supposed to be the value for "response.url". Since I implemented bug 1222008, I think I should fix this

I copied the code in https://sw-fetch-final-response-url.glitch.me/ to my localhost and test it. I tried to log URL while InternalResponse::IsRedirected() [1] is called (it should be called only while service worker start to synthesize response [2]) and I got two URL lists and each one only has one entry: http://localhost:8080/controlled/replaced and http://localhost:8080/controlled/redirect-to-replaced It seems the problem is that somehow service worker didn't overwrite the URL to the right place because it got the expected responses and tried to synthesize them into synthesized responses. [1] https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/dom/fetch/InternalResponse.h#335 [2] https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/dom/serviceworkers/ServiceWorkerEvents.cpp#373

I still don't understand why FF shows "Window got redirected response.url: https://sw-fetch-final-response-url.glitch.me/controlled/replaced" which should be the result for "Window got response.url". I suspected there was an issue that made Service Worker hijacks a wrong channel or treats a wrong channel as an intercepted channel. But, I set a breakpoint in HttpChannelChild::BeginNonIPCRedirect() just before calling HttpChannelChild::SetupRedirect() [1] and the responseURI is "http://localhost:8080/controlled/replaced" for the first hijacked request. That probably means that at least we pass the correct input for the redirect and it seems to imply service worker redirect a correct channel to a hijacked channel. Therefore, the fix should be as bkelly suggested in comment #0 and comment #1. We probably want to remove that check carefully. Note that: it seems to me we may consider using another flag to differentiate service worker's internal redirects or the others. [1] https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/netwerk/protocol/http/HttpChannelChild.cpp#1933

(In reply to Tom Tung [:tt] from comment #5) > Therefore, the fix should be as bkelly suggested in comment #0 and comment I meant, therefore, the issue should just like because the response is marked as a REDIRECT_INTERNAL, the correct response's URL doesn't be propagated to the final response's URL.

I'm considering to create a URL setter for the internal response in order to overwrite the URL [1] while the request is internally redirected by a service worker. Note that: - If the original request and internally redirected request are different origins the request should fail and the check is in SW, IIRC. - If the internally redirected request has redirects, it should work fine in current logic. [1] https://searchfox.org/mozilla-central/rev/e126996d9b0a3d7653de205898517f4f5b632e7f/dom/fetch/FetchDriver.cpp#1285

Bug 1222008 didn't propagate a sw redirected URL to outer response properly. To fix that this patch mainly make a redirecting request be overwritten while it's redirected by a service worker. This patch also add a private setter function for InternalRequest and a public checking function to avoid that function from being used widely. Bug 1486445 - P2 - Add a wpt test to verify url of a service worker redirected request is propagated to response; r?asuth This patch add a wpt test to ensure the service worker redirected request URL is propagated to the outer response.

Bug 1222008 didn't propagate a sw redirected URL to outer response properly. To fix that this patch mainly make a redirecting request be overwritten while it's redirected by a service worker. This patch also add a private setter function for InternalRequest and a public checking function to avoid that function from being used widely.

This patch add a wpt test to ensure the service worker redirected request URL is propagated to the outer response.

To clear the content for my confusion (comment #3 comment #4 comment #5): I misunderstood the behavior of comment #0. The result of "Window got redirected response.url" is because of the URL has been redirected by the server rather than FF did something wrong. So my patches simply overwrite the request URL while a service worker does an internal redirect. However, I'm a bit afraid that giving a public setter will make it be abused. So, I added a checker in public and created a private setter instead.

Comment on attachment 9005615 [details] Bug 1486445 - P1 - Propagate the sw internally redirected URL to fetching request; r?asuth Andrew Sutherland [:asuth] has approved the revision.

Comment on attachment 9005616 [details] Bug 1486445 - P2 - Add a wpt test to verify url of a service worker redirected request is propagated to response; r?asuth Andrew Sutherland [:asuth] has approved the revision.

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

Pushed by shes050117@gmail.com: https://hg.mozilla.org/integration/autoland/rev/bd73949fded4 P1 - Propagate the sw internally redirected URL to fetching request; r=asuth

Pushed by shes050117@gmail.com: https://hg.mozilla.org/integration/autoland/rev/78b3718b7ede P2 - Add a wpt test to verify url of a service worker redirected request is propagated to response; r=asuth

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

Thanks for the review!

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

https://hg.mozilla.org/mozilla-central/rev/bd73949fded4 https://hg.mozilla.org/mozilla-central/rev/78b3718b7ede

Upstream PR merged

Can't merge web-platform-tests PR due to failing upstream checks: Github PR https://github.com/web-platform-tests/wpt/pull/12985 * Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/DH3tBigiT8mMYJnqNmc1Wg)

We probably want to uplift this to beta since it's a regression. If we indeed want to do that, I will uplift patches to beta a week later if everything is fine. Marked it leave-open for now.