Closed Bug 1486445 Opened 2 years ago Closed 2 years ago
final response URL is not propagating through service worker interception correctly
46 bytes, text/x-phabricator-request
|Details | Review|
Bug 1486445 - P2 - Add a wpt test to verify url of a service worker redirected request is propagated to response; r?asuth
46 bytes, text/x-phabricator-request
|Details | Review|
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
Assignee: nobody → shes050117
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()  is called (it should be called only while service worker start to synthesize response ) 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.  https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/dom/fetch/InternalResponse.h#335  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()  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.  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  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.  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.
Attachment #9005615 - Flags: review+
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.
Attachment #9005616 - Flags: review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/bd73949fded4 P1 - Propagate the sw internally redirected URL to fetching request; r=asuth
Pushed by email@example.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.
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.
You need to log in before you can comment on or make changes to this bug.