1486445 - final response URL is not propagating through service worker interception correctly

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 2

[Anne (:annevk)]

(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.)

Comment 3

[Tom Tung [:tt, :ttung]]

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

Comment 4

[Tom Tung [:tt, :ttung]]

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

Comment 5

[Tom Tung [:tt, :ttung]]

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

Comment 6

[Tom Tung [:tt, :ttung]]

(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.

Comment 7

[Tom Tung [:tt, :ttung]]

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

Comment 8

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1486445 - P1 - Propagate the sw internally redirected URL to fetching request; r?asuth

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.

Comment 9

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1486445 - P1 - Propagate the sw internally redirected URL to fetching request; r?asuth

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.

Comment 10

[Tom Tung [:tt, :ttung]]

Attachment: 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.

Comment 11

[Tom Tung [:tt, :ttung]]

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 12

[Andrew Sutherland [:asuth] (he/him)]

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 13

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 14

[Tom Tung [:tt, :ttung]]

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd73949fded4
P1 - Propagate the sw internally redirected URL to fetching request; r=asuth

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

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

Comment 18

[Tom Tung [:tt, :ttung]]

Thanks for the review!

Comment 19

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

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

Comment 20

[Noemi Erli[:noemi_erli]]

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

Comment 21

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 22

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

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)

Comment 23

[Tom Tung [:tt, :ttung]]

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.