A fairly large change was made to the SW spec at the last f2f meeting: https://github.com/whatwg/fetch/pull/146 Basically: 1) If the Response passed to .resolveWith() has a .url set, then it should be passed on to the original intercepted channel. In effect, it should look like a redirect. 2) If Response.url is null, then use the Request.url. This effectively covers the synthetic Response case. 3) Navigations do not inspect the final URL in the html and fetch spec. They instead do "manual" redirects and follow Location headers themselves. So the URL shown in window.location should not be affected by these changes. In gecko, I think this means: a) If the Response.url is non-null and different from Request.url, then do a fake redirect like we do for opaque responses today. b) Otherwise, do nothing c) null Response.url for non-basic Responses should be treated as an error We will also have to change a ton of tests. This is a breaking change in that its observable in a number of places: * Intercepted fetch() will return a different Response.url * Intercepted stylesheets will load sub-resources relative to different URL * Intercepted workers will load relative importScripts() from different URL I'm marking this postv1, but we should probably do it sooner rather than later.

Note, there is some further discussion about document baseURI here: https://github.com/slightlyoff/ServiceWorker/issues/787

Some release timing feedback from chrome on this change: "Since M48 already branched, I'd say the earliest is M49 (branchpoint in Jan, reaches stable in Mar), though it could easily be a later milestone." I think to match we would need to get it into 45. Unfortunately, that does not seem feasible given we only have 2 more weeks before merge. We should try to get this in to 46.

Hopefully we can answer the open spec questions today. Namely, should document.baseURI be updated for the final URI? I think yes, but we need to see.

(In reply to Ben Kelly [:bkelly] from comment #3) > Hopefully we can answer the open spec questions today. Namely, should > document.baseURI be updated for the final URI? I think yes, but we need to > see. Agreed.

I believe we have finally decided to always propagate back Response.url as the baseURI of the outer resource: https://github.com/slightlyoff/ServiceWorker/issues/787

Setting a needinfo as a reminder.

Is that different from behaving like a redirect? I.e. are we inventing a new type of response that HTTP can't produce, and that is specific to service workers?

It's the same as a redirect for everything except a navigate. For a navigate we keep the location bar matching the request URL.

So for navigations there would be things that service-workers can do, but that HTTP-servers can't do?

One followup question is what happens if the resulting page calls pushState or replaceState? Does that change both the baseURI as well as the document URI? Or does it just update the document URI? Usually the two URIs are the same and both are updated when pushState/replaceState is called. Another question is what happens if the resulting page contains a <base href>? Does that override the baseURI set by the service-worker? Or the other way around?

(In reply to Jonas Sicking (:sicking) from comment #9) > So for navigations there would be things that service-workers can do, but > that HTTP-servers can't do? Yes, which it can already do without this change. Today service worker interception replaces navigation content without changing the location bar. This bug would just update the baseURI so relative resources continue to load correctly. (In reply to Jonas Sicking (:sicking) from comment #10) > One followup question is what happens if the resulting page calls pushState > or replaceState? Does that change both the baseURI as well as the document > URI? Or does it just update the document URI? pushState/replaceState would function normally after the page is loaded. It would change both. > Another question is what happens if the resulting page contains a <base > href>? Does that override the baseURI set by the service-worker? Or the > other way around? <base> work normally. It would override the baseURI.

I don't think it's accurate to say that a SW can already do things that HTTP-servers can't. Today when a SW responds with a resource from a different URL it acts just like if a HTTP server had loaded content from the other URL and responded with it. What we're talking about now is creating pages where document.URL != document.baseURI but where document.getElementsByTagName('base').length == 0. That is something that * Could never happen before * Still can't be done by a HTTP server I agree it might not be important. But it's something that we should only do intentionally after having examined the pros and cons. And we should acknowledge that it means that SWs don't just describe HTTP semantics, but also invent new ones.

Ok, but are you objecting to this or suggesting a change? Everyone in the room here is on board with this change and Jake has done a web developer survey to try to get their preferences included.

I would rather that we changed the URL in the URL-bar to be honest. A few reasons are: * It'll make implementation significantly easier since we'd only have to implement changes inside of necko. Adding new semantics means adding additional API to necko, wiring this additional information through docshell, document-loader-factory and the DOM. * It'll add less confusion for developers. When a page does <img src="pic.jpg"> and the URL bar says https://website.com/coolstuff/page.html, why does it try to load an image from https://website.com/whatever/pic.jpg? * It'll save developers from some of the weird cases around having a baseURI. Such as that <a href="#foo"> won't scroll within the current page, but rather navigate to a new page. Even worse is things like <svg:circle fill="url(#bar)"> which I believe will behave different in different browsers. Note that using <base href> is not particularly common, so most developers are not used to these edge cases.

That would break a specific use case for service workers. Specifically the ability to show an offline shell without redirecting. AFAIK this has been a goal from the beginning. We have also been shipping the "don't change location bar" for a long time and it's been a featured thing in almost every demo/article. We need to change the baseURI in the stylesheet case in order for relative images and fonts to work. We believe it's less confusing to keep baseURI behavior and document consistent. Jake has actually talked to a lot of devs about this. They have a wide variety of understanding and expectations. No one approach came out as most clear for devs. With that in mind we feel keeping relative resources functional and baseURI consistent is the best we can do.

To back up my claim that not-changing-the-address-bar is an advertised feature people use for offline shells, see its use on The Guardian: https://www.theguardian.com/info/developer-blog/2015/nov/04/building-an-offline-page-for-theguardiancom And other sites. For more details on the results of Jake's research with web devs, see: https://github.com/slightlyoff/ServiceWorker/issues/787#issuecomment-184165823

I definitely agree that we don't want to change the URL when serving a offline-shell. I'd even agree that doing that is more important than not introducing semantics that can't be expressed in HTTP. What I'm not convinced of is that the proposed solution is the only solution to that problem. One big concern is that having this behavior by default for navigations means that the complexities in comment 14 are exposed to developers even for the case when they are not serving an offline shell, but rather using an alternative resource for other reasons (i'm not sure what reasons those would be, but given that we handle using different responses for different URLs for resources, I assume that such reasons exist). One potential solution is: * Require that pages that are used as offline-shells include a <base> tag. Or that they use absolute URLs for stylesheets and images. This is what well-written websites do for 404 pages today. * Enable SWs that are responding with a differet-url Response to a navigation Request to choose if it should be treated as a redirect or not (we can choose whatever default we want)

(In reply to Jonas Sicking (:sicking) from comment #17) > One big concern is that having this behavior by default for navigations > means that the complexities in comment 14 are exposed to developers even for > the case when they are not serving an offline shell, but rather using an > alternative resource for other reasons (i'm not sure what reasons those > would be, but given that we handle using different responses for different > URLs for resources, I assume that such reasons exist). Have you spoken to any developers about this? Or is it just your belief? > One potential solution is: > * Require that pages that are used as offline-shells include a <base> tag. > Or that they use absolute URLs > for stylesheets and images. This is what well-written websites do for 404 > pages today. This is our current state today. After talking with devs (see above link) we have decided we should fix the baseURIs so relative resources continue to work. > * Enable SWs that are responding with a differet-url Response to a > navigation Request to choose if > it should be treated as a redirect or not (we can choose whatever default > we want) This is already possible. `evt.respondWith(response)` does not change location bar, while `evt.respondWith(Response.redirect(url))` does change location bar. I'm sorry if you disagree, but this decision has been made deliberately with data from the community and devs. If you have additional data showing its a problem, we would love to see that. Simply stating a belief that its more confusing than the other options does not match the data we have.

Another potential solution is to add the ability for http responses to explicitly set a base-uri, thereby at least keeping http semantics and SW semantics in sync. This seems like it could be useful for said 404 pages. But should I interpret your answer as that these other solutions have been considered and rejected? And that everyone was aware of the problems in comment 14?

> This is already possible. `evt.respondWith(response)` does not change location bar, while > `evt.respondWith(Response.redirect(url))` does change location bar. That still means that for resources we have the ability to do a combined redirect-and-return-resource using `evt.respondWith(responseWithDifferentURL)`, but we don't have that ability for navigations. So sounds like developers asked for different capabilities for navigations and resources?

(In reply to Jonas Sicking (:sicking) from comment #20) > That still means that for resources we have the ability to do a combined > redirect-and-return-resource using > `evt.respondWith(responseWithDifferentURL)`, but we don't have that ability > for navigations. So sounds like developers asked for different capabilities > for navigations and resources? I don't understand what this means. And also I think resources already behave in this manner. If an img element gets redirected its .src attribute is not updated to the final URL. That is conceptually the same as leaving the location bar unchanged. I would look up more spec cases, but I'm on tethering at the moment. The last time I spoke with Boris about this, though, I remember him thinking this was reasonable given how we don't update sub-resource element attributes for final redirect URL.

For a fetch to URL X, there's a big difference between A) Redirecting a fetch to a new URL, Y, and returning a resource there, and B) Returning a resource but fudging its baseURL to be Y. A involves a redirect and returns a resource with URL Y. B does not involve a redirect and returns a resource with URL X. I agree that you can't tell the difference through the <img>.src attribute. In fact, there's relatively few places where you can tell a difference during resource loads, but it's not zero places. I would imagine that fetch() would treat the two differently. I would expect behavior A to result in a Response object with a .url = Y, and B to result in a Response object with .url = X. I would also expect that if fetch() used { redirect: "error" }, that behavior A would result in an error while behavior B would not. Another example is SVG files loaded into an <img> or used as a CSS image, if such an SVG file contain a <circle fill="url(#foo)">. Using behavior A I would expect the circle to be painted using the pattern found at element with id=foo. Using behavior B I would expect another network request to be made to URL Y before a pattern could be extracted. In gecko I would also expect this network request to be blocked since we don't allow SVG-in-image to load external resources. I would really encourage you to not think about this in terms of "how will img.src and the location bar behave", since the web platform contains a lot of APIs and you'll have to define behavior for all of them. Instead I would encourage you to think about this in terms of what how necko behaves towards the gecko callsite. Which is equivalent to thinking about this in terms of the fetch specification. I know the latter isn't always easy since not all specifications has been updated and expressed in terms of the fetch specification. However in practice in implementations all web platform features do use the same network stack and are affected by how that network stack behaves.

> Another potential solution is to add the ability for http responses to explicitly set a base-uri This does not seem totally unreasonable, via headers.

Jonas: I'm struggling to get a lock on where your objection is. I know it's a pain, but could you answer the questions at https://jakearchibald.com/2016/service-workers-and-base-uris/ (either there or here). I was unsure of this change initially, but making baseURL the response URL feels like the most consistent rule.

Jonas, have you had a chance to provide the feedback Jake asked for? Other browsers are starting to implement this so if you have spec feedback, now's the time.

I made a glitch to test this, and it appears chrome has implemented it: https://sw-passthrough-redirect.glitch.me/ We should implement it as well.

Edge 16 with service workers enabled in about:flags also shows the same behavior as chrome on the glitch in comment 26.

I don't have time to work this immediately, but I think we should do it soonish. Tom, do you have any bandwidth to take this? Maybe for early 59 timeframe? I think what we need to do is: 1. Change the respondWith() code that does the "fake" internal redirect for opaque responses and make it do the redirect whenever the Response.url is non-empty and does not match the original Request.url. 2. If the Response.redirected value is false, do an internal redirect like today. 3. If the Response.redirected value is true, do a real redirect instead. 4. Probably fix a bunch of tests.

Sure, I'll work on this.

Hi Ben, I'm not pretty sure if I understand what should do in this bug clearly or if I misunderstand anything, so would you mind giving me some feedback for this patch? [The current code path] IIUC, when |FetchEvent::respondWith()| resolves a promise, we pass responseURLSpec[1] to the InterceptChannel via class StartResponse if the response type is opauqe[2]. Then, StartSynthesizedResponse decides to do the internal redirect[3] only when the response URL/responseURI is different from the request/original URL. (Now, only opaque response may have different URI since [2]) [Solution] Set the responseURLSpec[1] when the response's URL is different from request's. Also, pass the response.redirected to BeginNonIPCRedirect(). Decide whether do the internal or the real redirect by response.redirected. For how to do the real redirect, I guess changing the redirect flag for SetupRedirect() in BeginNonIPCRedirect[4] to REDIRECT_TEMPORARY might be enough since it will make the FetchDriver record the redirecting URL into the URL list on the response. [ToDo] If this patch is correct so far, I need to deal with the non-e10s case (I guess I need to modify the code in InterceptHttpChannel) and fix the broken tests. [1] http://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.cpp#346 [2] http://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.cpp#664-670 [3] http://searchfox.org/mozilla-central/source/netwerk/protocol/http/InterceptedChannel.cpp#304-305 [4] http://searchfox.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#1738

Comment on attachment 8924107 [details] [diff] [review] bug1222008.patch Review of attachment 8924107 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Thanks! Do you have any idea what tests break with this yet? I'm just curious how widespread it is and if there are WPT tests that chrome is currently passing somehow. ::: dom/workers/ServiceWorkerEvents.cpp @@ +670,5 @@ > + } else if (!ir->GetUnfilteredURL().IsEmpty() && > + !mRequestURL.EqualsASCII(ir->GetUnfilteredURL().get())) { > + // Do the (internal/real) redirect to the response URL if it's different > + // from the request URL. > + responseURL = ir->GetUnfilteredURL(); I think you can remove all the conditions for opaque and equality here. You should be able to pass `ir->GetUnfilteredURL()` directly instead of computing `responseURL` here. The later necko code will do the string comparison for equality. ::: netwerk/protocol/http/HttpChannelChild.h @@ +432,5 @@ > > // Perform a redirection without communicating with the parent process at all. > void BeginNonIPCRedirect(nsIURI* responseURI, > + const nsHttpResponseHead* responseHead, > + const bool internalRedirect); nit: Maybe flip this argument to `aRealRedirect` instead of `internalRedirect` to match the logic in nsINetworkInterceptController interface.

(In reply to Ben Kelly [:bkelly] from comment #31) Thanks for the quick feedback! I'll address your suggestions, make sure it works fine on non-e10s(InterceptHttpChannel), check out how many tests fail and how do chrome deal with them. > Comment on attachment 8924107 [details] [diff] [review] > bug1222008.patch > > Review of attachment 8924107 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good. Thanks! > > Do you have any idea what tests break with this yet? I'm just curious how > widespread it is and if there are WPT tests that chrome is currently passing > somehow. I have no idea about it now. I'll check it out today. > ::: dom/workers/ServiceWorkerEvents.cpp > @@ +670,5 @@ > > + } else if (!ir->GetUnfilteredURL().IsEmpty() && > > + !mRequestURL.EqualsASCII(ir->GetUnfilteredURL().get())) { > > + // Do the (internal/real) redirect to the response URL if it's different > > + // from the request URL. > > + responseURL = ir->GetUnfilteredURL(); > > I think you can remove all the conditions for opaque and equality here. You > should be able to pass `ir->GetUnfilteredURL()` directly instead of > computing `responseURL` here. The later necko code will do the string > comparison for equality. Sure > ::: netwerk/protocol/http/HttpChannelChild.h > @@ +432,5 @@ > > > > // Perform a redirection without communicating with the parent process at all. > > void BeginNonIPCRedirect(nsIURI* responseURI, > > + const nsHttpResponseHead* responseHead, > > + const bool internalRedirect); > > nit: Maybe flip this argument to `aRealRedirect` instead of > `internalRedirect` to match the logic in nsINetworkInterceptController > interface. Sure

Address the comment and ensure it does the same thing in non-e10s mode. I'll try to catch out the broken tests on my machine. Meanwhile, I'll push it to try in case I overlook any broken test.

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

I put the failed tests below: XpcShell [Fixed] netwerk/test/unit/test_synthesized_response.js [Fixed] netwerk/test/unit_ipc/test_synthesized_response_wrap.js - Solution: Fill up the last argument (adding in the patch) to startSynthesizedResponse() in the test Mochitest [Fixed] dom/tests/mochitest/fetch/test_fetch_basic_http_sw_reroute.html - Solution: Correct the redirected URL [1] if the request is hijacked by the service worker [1] http://searchfox.org/mozilla-central/source/dom/tests/mochitest/fetch/test_fetch_basic_http.js#53-54 [Fixing] dom/workers/test/serviceworkers/test_fetch_event_with_thirdpartypref.html - Error: synthesize CORS response should result in outer CORS response - Solution: Since we do the internal redirect while a service worker responds with a different URL (without redirected), we should get opaque type of response. - Error: INFO TEST-UNEXPECTED-FAIL | dom/workers/test/serviceworkers/test_fetch_event_with_thirdpartypref.html | Load CORS cross origin XML Document should be allowed - Still finding the reason... dom/workers/test/serviceworkers/test_fetch_event.html dom/workers/test/serviceworkers/test_devtools_serviceworker_interception.html Wpt service-workers/service-worker/fetch-event-redirect.https.html service-workers/service-worker/fetch-frame-resource.https.htm service-workers/service-worker/fetch-response-taint.https.html service-workers/service-worker/redirected-response.https.html

(In reply to Tom Tung [:tt] from comment #35) > I put the failed tests below: Thanks! > [Fixing] > dom/workers/test/serviceworkers/test_fetch_event_with_thirdpartypref.html > - Error: synthesize CORS response should result in outer CORS response > - Solution: Since we do the internal redirect while a service worker > responds with a different URL (without redirected), we should get opaque > type of response. Wait, the type of the response should still be CORS. I think probably the outer channel is no-cors and is hitting this path in its security code: https://searchfox.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#712 The service worker spec has this annoying thing where we have to override this behavior in some synthetic cases. We do it already here: https://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.cpp#338 The problem, though, is that call is made before the additional redirect. We either need to move it to happen after the redirect (not sure where to put that). Or maybe make the LoadInfo set a flag when we have called SynthesizeServiceWorkerTainting() so that we don't change it further in MaybeIncreaseTainting(). I think freezing the tainting value after synthesis is probably the easiest approach. Christoph, do you have any opinions here? The issue is we're making synthesized channels perform an internal redirect to their synthetic Response.url. The problem is that when a channel with SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS is synthesized with a CORS response from the SW the SW spec wants the resulting outer channel to be LoadTainting::CORS. I'm basically suggesting we follow the same solution used in bug 1369862.

(In reply to Ben Kelly [:bkelly] from comment #36) Got it! Thanks for telling me that! I'll try the approach you suggested if Christoph doesn't have other suggestion.

(In reply to Tom Tung [:tt] from comment #35) > - Error: INFO TEST-UNEXPECTED-FAIL | > dom/workers/test/serviceworkers/test_fetch_event_with_thirdpartypref.html | > Load CORS cross origin XML Document should be allowed > - Still finding the reason... Hi Ben, In this test, "index.html" tries to open an XML document under the same origin (http://mochi.test:8888/tests/dom/workers/test/serviceworkers/fetch/load_cross_origin_xml_document_cors.xml). Then, the request is hijacked by a service worker and respond with another response [2]. The response is generated by fetching a request with a cross-origin URL (http://example.com/tests/dom/security/test/cors/file_CrossSiteXHR_server.sjs?status=200&allowOrigin=*) with CORS mode. After applying my patch, since the requesting URL is different from responding URL and the |response.redirected = false|, it should do an internal redirect. However, I got few warning messages and fail in this test case. I put the warning messages below: GECKO(65301) | [Child 65303, Main Thread] WARNING: NS_ENSURE_TRUE(!(NS_HasBeenCrossOrigin(aChannel, true))) failed: file /Mozilla/Work/m-c/dom/security/nsContentSecurityManager.cpp, line 196 GECKO(65301) | [Child 65303, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805303F4: file /Mozilla/Work/m-c/dom/security/nsContentSecurityManager.cpp, line 706 GECKO(65301) | [Child 65303, Main Thread] WARNING: 'NS_FAILED(aRv)', file /Mozilla/Work/m-c/dom/workers/ServiceWorkerEvents.cpp, line 245 It seems the security mode in the load flag is set to be "nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED", but the URL of the checking channel is redirected to response's "http://example.com/tests/dom/security/test/cors/file_CrossSiteXHR_server.sjs?status=200&allowOrigin=*". Thus, the test fails while checking the DOSOPChecks(). I'm not sure about the correct behavior and the real problem here. Does the security mode wrong not being redirected correctly? Or the reponse's URL should be "http://mochi.test:8888/tests/dom/workers/test/serviceworkers/fetch/load_cross_origin_xml_document_cors.xml" after the internal redirect. Or there is another issue? Would you mind giving me some suggestions or hints? Thanks! [1] http://searchfox.org/mozilla-central/source/dom/workers/test/serviceworkers/fetch/index.html#140 [2] http://searchfox.org/mozilla-central/source/dom/workers/test/serviceworkers/fetch_event_worker.js#343-351

Anne, what do you think about comment 38? If a same-origin outer request (xml document in this case) is intercepted with a cross-origin CORS request by the SW, should we still use the final Response.url? In this case the feature (xml doc) does not expect to see a cross-origin URL at all. Do we really want to start exposing a cross-origin documentURI here? I feel like it might be safer to expect this sort of use-case to reject. You can't synthesize a cross-origin Response onto a Request in same-origin mode, etc. If we do want to support this we probably need to change nsContentSecurityManager and nsCORSListenerProxy to better understand synthesized channels.

(In reply to Ben Kelly [:bkelly] from comment #36) > Christoph, do you have any opinions here? The issue is we're making > synthesized channels perform an internal redirect to their synthetic > Response.url. The problem is that when a channel with > SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS is synthesized with a CORS response > from the SW the SW spec wants the resulting outer channel to be > LoadTainting::CORS. I'm basically suggesting we follow the same solution > used in bug 1369862. Yeah that makes sense. Adding a new flag to the loadinfo, similar as in Bug 1369862 sounds like the right path forward to me.

(In reply to Tom Tung [:tt] from comment #35) > dom/workers/test/serviceworkers/test_devtools_serviceworker_interception.html The test does the steps as follows: 1. The controlled document send out a request with "fake.html" 2. The service worker hijacks the request and respond a response with "hello.html" 3. Waiting for the OnStopRequest() which sending the "http-on-stop-request" topic to the observer and then checking the timing information. 4. Timeout Currently, we do an internal redirect for this case because the response "hello.html" hasn't redirected. However, this implies the OnStopRequest will stop the redirected request ("hello.html") [1]. Meanwhile, we record the FinishSynthesizedResponseEnd on the original channel [2] after redirecting. It means we track this marker on the original channel without having a chance to copy it to the redirected channel. Thus, in this test, we cannot get the original channel (with all the timing markers) by listening to the "http-on-stop-request" topic, although we can get the redirect channel ("hello.html") without all the timing markers (missing the FinishSynthesizedResponseEnd) by listening to the "http-on-stop-request". Both situations cannot meet the testing requirement (get all the timing makrers). I guess we could either manually sending OnStopRequest for the original request or record the FinishSynthesizedResponseEnd on the redirect channel. Not sure which ways are better or if there is a better solution to fix this problem. [1] https://searchfox.org/mozilla-central/source/netwerk/protocol/http/InterceptedHttpChannel.cpp#1015 [2] https://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.cpp#216

(In reply to Tom Tung [:tt] from comment #35) > Wpt > > service-workers/service-worker/fetch-event-redirect.https.html It looks like this change makes Firefox pass the rest test cases which are expected fail. Remove the ".ini" file fix the issue. > service-workers/service-worker/fetch-frame-resource.https.htm This one looks problematic; my Nightly crashed while running it. > service-workers/service-worker/fetch-response-taint.https.html We fail on 6 test cases, but I got 16 failures on Chrome Canary. I list our failure test cases below, I'll dig a bit more tomorrow: fetching url:"https://web-platform.test:8443/?url=https%3A%2F%2Fwww1.web-platform.test%3A8443%2Fservice-workers%2Fservice-worker%2Fresources%2Ffetch-access-control.py%3FACAOrigin%3D*&mode=cors&credentials=omit&" mode:"same-origin" credentials:"omit" should succeed. with error message: promise_test: Unhandled rejection with value: object "TypeError: NetworkError when attempting to fetch resource." Chrome Canary pass this test. fetching url:"https://web-platform.test:8443/?url=https%3A%2F%2Fwww1.web-platform.test%3A8443%2Fservice-workers%2Fservice-worker%2Fresources%2Ffetch-access-control.py%3FACAOrigin%3D*&mode=cors&credentials=omit&" mode:"same-origin" credentials:"same-origin" should succeed. with error message: promise_test: Unhandled rejection with value: object "TypeError: NetworkError when attempting to fetch resource." Chrome Canary pass this test. fetching url:"https://web-platform.test:8443/?url=https%3A%2F%2Fwww1.web-platform.test%3A8443%2Fservice-workers%2Fservice-worker%2Fresources%2Ffetch-access-control.py%3FACAOrigin%3D*&mode=cors&credentials=omit&" mode:"same-origin" credentials:"include" should succeed. with error message: promise_test: Unhandled rejection with value: object "TypeError: NetworkError when attempting to fetch resource." Chrome Canary pass this test. fetching url:"https://web-platform.test:8443/?url=https%3A%2F%2Fwww1.web-platform.test%3A8443%2Fservice-workers%2Fservice-worker%2Fresources%2Ffetch-access-control.py%3FACAOrigin%3Dhttps%3A%2F%2Fweb-platform.test%3A8443%26ACACredentials%3Dtrue&mode=cors&credentials=include&" mode:"same-origin" credentials:"omit" should succeed. with error message: promise_test: Unhandled rejection with value: object "TypeError: NetworkError when attempting to fetch resource." Chrome Canary: fail as well with error message: promise_test: Unhandled rejection with value: object "TypeError: Failed to fetch" fetching url:"https://web-platform.test:8443/?url=https%3A%2F%2Fwww1.web-platform.test%3A8443%2Fservice-workers%2Fservice-worker%2Fresources%2Ffetch-access-control.py%3FACAOrigin%3Dhttps%3A%2F%2Fweb-platform.test%3A8443%26ACACredentials%3Dtrue&mode=cors&credentials=include&" mode:"same-origin" credentials:"same-origin" should succeed. with error message: promise_test: Unhandled rejection with value: object "TypeError: NetworkError when attempting to fetch resource." Chrome Canary: fail as well with error message: promise_test: Unhandled rejection with value: object "TypeError: Failed to fetch" fetching url:"https://web-platform.test:8443/?url=https%3A%2F%2Fwww1.web-platform.test%3A8443%2Fservice-workers%2Fservice-worker%2Fresources%2Ffetch-access-control.py%3FACAOrigin%3Dhttps%3A%2F%2Fweb-platform.test%3A8443%26ACACredentials%3Dtrue&mode=cors&credentials=include&" mode:"same-origin" credentials:"include" should succeed. with error message: promise_test: Unhandled rejection with value: object "TypeError: NetworkError when attempting to fetch resource." Chrome Canary: fail as well with error message: promise_test: Unhandled rejection with value: object "TypeError: Failed to fetch" > service-workers/service-worker/redirected-response.https.html I pass this test after addressing comment #36 (adding a flag)

(In reply to Tom Tung [:tt] from comment #42) > > service-workers/service-worker/fetch-response-taint.https.html > > We fail on 6 test cases, but I got 16 failures on Chrome Canary. I list our > failure test cases below, I'll dig a bit more tomorrow: They are all the same problem as comment #38. It fails on same origin check in nsContentSecurityManager if we are passing an outer request in the same origin with same-origin mode and it is intercepted with a cross-origin CORS request by the SW. Note: It's interesting that Chrome have the different behavior in these test cases. They fails on the first three test cases but pass the other three test cases. I'll investigate this after investigating "service-workers/service-worker/fetch-frame-resource.https.html" and fixing "dom/workers/test/serviceworkers/test_devtools_serviceworker_interception.html".

> If a same-origin outer request (xml document in this case) is intercepted with a cross-origin CORS request by the SW, should we still use the final Response.url? In this case the feature (xml doc) does not expect to see a cross-origin URL at all. Do we really want to start exposing a cross-origin documentURI here? I don't really see the problem. Security checks don't rely on the URL of a document. I think we knew going on that this was a mismatch with browser architecture, but we thought it was a better model to put the authority with the response, rather than the request. We can revisit that of course, especially with it still not being implemented, but I don't think anything has changed meanwhile.

re-push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=76bc9cad98d8ddf01217f5c402481581cbd39cc9&selectedJob=142647303 I find out I overlook three broken tests: 1. dom/workers/test/serviceworkers/test_devtools_track_serviceworker_time.html This test left a service worker registered without cleaning it up Seems that it's affected by the failure of "test_devtools_serviceworker_interception.htm". I can pass the test in my local machine. 2. /service-workers/service-worker/redirected-response.https.html Unexpected-Pass 3. /service-workers/service-worker/worker-interception.https.html Verify worker script intercepted by cors response succeeds - promise_test: Unhandled rejection with value: undefined

Tom, I think we need to add some "channel is synthesized, disable URL checks" load flag. The security manager would then need to look for this to bypass its checking. We'll need to have some scary comments indicating this should not be used outside of service worker interception, etc.

(In reply to Ben Kelly [:bkelly] from comment #46) > Tom, I think we need to add some "channel is synthesized, disable URL > checks" load flag. The security manager would then need to look for this to > bypass its checking. We'll need to have some scary comments indicating this > should not be used outside of service worker interception, etc. Thanks for the suggestion! I'll try to do that!

(In reply to Tom Tung [:tt] from comment #47) > (In reply to Ben Kelly [:bkelly] from comment #46) > > Tom, I think we need to add some "channel is synthesized, disable URL > > checks" load flag. The security manager would then need to look for this to > > bypass its checking. We'll need to have some scary comments indicating this > > should not be used outside of service worker interception, etc. > > Thanks for the suggestion! I'll try to do that! So, we spoke about this issue at the service worker face-to-face today. We actually came around to thinking that perhaps we should be blocking CORS cross-origin Response from being synthesized for a same-origin Request. This is different from what the fetch spec has said for a long time, though, and different from Anne's read in comment 44. I'm going to file a fetch spec issue about this, but its unclear to me which way its going to go. Do you want to continue investigating here or switch to something else while we sort the desired behavior out? Sorry for the confusion.

(In reply to Ben Kelly [:bkelly] from comment #48) > So, we spoke about this issue at the service worker face-to-face today. We > actually came around to thinking that perhaps we should be blocking CORS > cross-origin Response from being synthesized for a same-origin Request. > This is different from what the fetch spec has said for a long time, though, > and different from Anne's read in comment 44. > > I'm going to file a fetch spec issue about this, but its unclear to me which > way its going to go. > > Do you want to continue investigating here or switch to something else while > we sort the desired behavior out? Sorry for the confusion. I'll focus on the other issues (other broken test) found in this bug first. Thanks for the information!

Sending the patches finished so far for review. Hi Ben, This patch is the main changes for this bug. Basically, when the response's URL is diffenet from the request's URL, it makes the interceptedChannel do the internal redirect if response.redirected is false and do the real redirect if response.redirected is true. For the internal redirect, I follow the current logic which use the REDIRECT_INTERNAL flag to open a channel. For the real redirect, I choose the REDIRECT_TEMPORARY flag [1] to open a channel, since the redirect is caused by a service worker. The upcoming request should re-validate if the service worker is somehow terminated. Would you mind helping me to review this patch? Thanks! Note: feel free to drop the request if the final decision for synthesizing a CORS cross-origin request to a same-origin request may change the decision for doing this bug. [1] https://searchfox.org/mozilla-central/source/netwerk/base/nsIChannelEventSink.idl#29

This patch is for fixing the broken netwerk xpcshell test due to having addtional argument after applying P1

This patch is to fix the test which checks the response.url won't change even the service worker redirects its request to another. The behavior changes since P1, so fix the test.

This patch is to add a flag on LoadInfo, so that we can decide not to change the tainting if the response is a synthesized by a service worker. Note: I'm not sure about if I need to pass this flag through IPC. I do it for now. For current architecture, I _think_ we won't. However, I'm not sure whether we still check the channel/update the tainting on content process (hijack on chrome process).

Tom, I'll probably wait to review here until after we get some clarification on the spec. I'll leave the flags for now, though. Also, here is the spec issue I filed: https://github.com/whatwg/fetch/issues/629

Tom, sorry I haven't had a chance to do reviews here yet. I just never had enough time to sit and think while at TPAC and while traveling. I'll take a look tomorrow. I think we do have some general idea of where we want to go from a spec perspective, though. Let me describe that since I think some of it may be actionable before I review this here. To refresh the core question is what to do if a `evt.respondWith()` gets a `Response.type === 'cors'` while `evt.request.mode === 'same-origin'`. Our initial approach was to propagate the `Response.url` which ended up triggering failures in nsSecurityManager due to the same-origin policy. We discussed this a bit at the TPAC face-to-face and in this issue: https://github.com/whatwg/fetch/issues/629 I believe what we all want to do now is to actually fail the network request in this case. So this mostly matches what our nsSecurityManager is doing already. The question is, can we safely make a CORS response passed for a same-origin mode request fail without breaking existing sites? To that end, I was wondering if you could file another bug to track telemetry or a use counter for this case? Ideally we would land this soon and maybe even uplift to beta. The use counter would be ideal since it measured per document loaded (I think), but it also needs to an nsIDocument to reference. So you would have to get that out of the LoadInfo or nsIChannel. I'm not sure if a dedicated Worker script has a document reference like this. I really want to know if worker scripts are running into this since its one of the more common causes of a same-origin mode Request. If we can't do the use counters reasonably, maybe we just fall back to telemetry that gives us a count of CORS Response with same-origin mode Request vs total count of synthesized responses. We can then at least get a percentage of the total network requests even if its not normalized to the number of pages loaded. While that is collecting data, we could then proceed with implementing the Response.url propagation and the CORS rejection code. Until we get a verdict from the data we could add a temporary quirk in the code controlled by a pref. If the pref has the quirk enabled then we would do the equivalent of this when we hit the CORS response with same-origin mode Request: var quirkResponse = new Response(corsResponse.body, corsResponse); Basically synthesize a new Response object just like the service worker script could do. This will result in an empty string Response.url and may lose some non-standard headers. This Response would then pass the Response validation code as its no longer CORS and it no longer has a URL to be propagated. Hopefully, once we have data we can then disable the pref to turn off the quirk. We could also do this if the other browsers agree to do it at the same time, even if it is breaking. Also, I realized that there is another case where we do not want to propagate the Response.url. The service worker and html spec are actually written in this weird way that causes the Request.url to always be used for `Request.mode === 'navigation'`. If the mode indicates its a navigation then we probably do not want to do the fake redirect to propagate the URL. We don't need to worry about the Response.redirected flag here because these requests will also be `Request.redirect === 'manual'` which should fail on a redirected Response. What do you think? Sorry this issue kind of turned into a mess.

(In reply to Ben Kelly [:bkelly] from comment #55) Keep the ni flag because I need a bit more time to digest this...

Comment on attachment 8926220 [details] [diff] [review] Propagate the URLs from respondWith() if the response's URL is different from the request's and the response has been redirected Review of attachment 8926220 [details] [diff] [review]: ----------------------------------------------------------------- Overall this looks really good. I think it just needs to be updated to handle the navigation case I forgot to mention originally. Also, I think we need to add the quirk/rejection code for the "CORS response and same-origin mode request" case here: https://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.cpp#609 That one could reasonably be done as a follow-up, though. ::: dom/workers/ServiceWorkerEvents.cpp @@ -278,5 @@ > StartResponse(nsMainThreadPtrHandle<nsIInterceptedChannel>& aChannel, > InternalResponse* aInternalResponse, > const ChannelInfo& aWorkerChannelInfo, > const nsACString& aScriptSpec, > - const nsACString& aResponseURLSpec, Sorry, I think I changed requirements on you such that we need to keep passing the URL here. In RespondWithHandler::ResolvedCallback() we need to check for a navigation request and pass empty string in that case. Something like: nsCString url; if (mRequestMode != RequestMode::Navigate) { responseURL = ir->GetUnfilteredURL(); } And then pass responseURL to StartResponse() constructor. This is to handle the case where someone synthesizes a Response with a different URL for a document navigation request. In that case we want to make sure the resulting window location is the original Request.url and does not use the Response.url. @@ +661,5 @@ > // When an opaque response is encountered, we need the original channel's principal > // to reflect the final URL. Non-opaque responses are either same-origin or CORS-enabled > // cross-origin responses, which are treated as same-origin by consumers. > + if (response->Type() == ResponseType::Opaque && > + NS_WARN_IF(ir->GetUnfilteredURL().IsEmpty())) { This is basically an extra safety check to make sure our invariant that opaque responses always have a URL does not break. I think we should probably check this for CORS responses as well. It would also be useful to add an assertion here. Maybe something like this: if (NS_WARN_IF((response->Type() == ResponseType::Opaque || response->Type() == ResponseType::CORS) && ir->GetUnfilteredURL().IsEmpty()))) { MOZ_DIAGNOSTIC_ASSERT(false, "CORS or opaque Response without a URL"); return; } ::: netwerk/protocol/http/HttpChannelChild.cpp @@ +1734,5 @@ > LOG(("HttpChannelChild::BeginNonIPCRedirect [this=%p]\n", this)); > > + const uint32_t redirectFlag = > + aRealRedirect ? nsIChannelEventSink::REDIRECT_TEMPORARY > + : nsIChannelEventSink::REDIRECT_INTERNAL; Maybe add a comment that we are using REDIRECT_TEMPORARY here, but the exact type does not matter as long as its not REDIRECT_INTERNAL. ::: netwerk/protocol/http/InterceptedHttpChannel.cpp @@ +236,1 @@ > // the given cross-origin response URL. The resulting channel will then This is not always a cross-origin URL any more. Probably need to clarify this whole comment to indicate it can be called for many different URLs now. It allows content to see the final URL where appropriate and also helps us enforce cross-origin restrictions. Also, could we name this RedirectForResponseURL() or something? I just want to somehow clarify its different from a 30x redirect. ::: netwerk/protocol/http/InterceptedHttpChannel.h @@ +114,5 @@ > FollowSyntheticRedirect(); > > + // If the response's URL is different from the request's, do a real redirect > + // while the response has not been redirected yet. Otherwise, do an internal > + // redirect. nit: Maybe clarify this a bit. Maybe something like: // If the response's URL is different from the request's then do a // redirect. If Response.redirected is false we do an internal redirect. // Otherwise, if Response.redirect is true to a non-internal redirect // so end consumers detect the redirected state.

Comment on attachment 8926229 [details] [diff] [review] Freeze the tinting if a service worker responds with a synthesize repsonse Review of attachment 8926229 [details] [diff] [review]: ----------------------------------------------------------------- I don't think we should need this one any more now that we are planning to reject/quirk the "CORS response on same-origin mode request" case. Do you agree?

(In reply to Ben Kelly [:bkelly] from comment #55) Thanks for providing information, the feeback and the reviews! I'll try to implement this after adding the telemetry/use counter. I feel like we should test the behaviors under different combinations of request.mode, request.redirect and response.type.

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

Address the comment.

Comment on attachment 8926229 [details] [diff] [review] Freeze the tinting if a service worker responds with a synthesize repsonse Remove the patch per comment #58

(In reply to Ben Kelly [:bkelly] from comment #57) > Also, I think we need to add the quirk/rejection code for the "CORS response > and same-origin mode request" case here: > > https://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerEvents. > cpp#609 > > That one could reasonably be done as a follow-up, though. I'll file a follow-up bug to track it.

(In reply to Tom Tung [:tt] from comment #63) > I'll file a follow-up bug to track it. I actually was hoping for a follow-up patch in this bug. I don't think we can wait for this since we need it to avoid having to change the security manager.

(In reply to Ben Kelly [:bkelly] from comment #58) > Comment on attachment 8926229 [details] [diff] [review] > Freeze the tinting if a service worker responds with a synthesize repsonse > > Review of attachment 8926229 [details] [diff] [review]: > ----------------------------------------------------------------- > > I don't think we should need this one any more now that we are planning to > reject/quirk the "CORS response on same-origin mode request" case. Do you > agree? Sorry Ben, I feel like I still not understand about this. So, the original reason for doing this patch is: - SW intercepts an cross origin URL with "no-cors" mode and responds by fetching another URL with "cors" mode and its headers Which response.type is the content excepted to have? It's "opaque" or "cors"? Then, we decide to have this patch (freeze the tatinting so that it won't be increase after the redirect) as your suggestion in comment #36. So that we could get the response with its type is "cors" because it won't be increased by nsContentSecurityManager. So, I don't understand why we don't need this patch? BTW, just to make sure I understand correctly, we are going to have a perf and it's enabled by default. If there is an CORS response on same-origin mode request, we should make a quirkResponse for it, right? Also, I have one more question about the perf. I find out we need to get the value perf on the main thread. However, while we are in the code for respondWith(), we are on the worker thread. Currently, I have two naive thoughts to solve this problem. One is to read the perf while the service worker is registering/updating, but we cannot be aware of the change of the pref immediately. The other one is to jump into the main thread synchronously while we are in the condition for the CORS response on same-origin mode request. However, I'm a a bit afraid this might slow down the service worker. Do you have any suggestion about that? Thanks!

Ben, I address the comment. Also, I change the arguemnt "aRealRedirect"'s name to "aResponseRedirected" for being more clearly. Could you help me to review it? Thanks!

This patch is to modify the test browser_devtools_serviceworker_interception.js. This test used to catch the channel by the request's URL for the most of the cases. However, since we decide to propagate the response's URL to the content, we need to change this test to catch the channel by the expected response's URL. Besides, this makes the final URI of the outer and inner channel be the same, so we need to catch the second channel which fires the notification.

Sorry, there is a typo in the patch.

(In reply to Tom Tung [:tt] from comment #73) Sorry, I was wrong. It avoids the security check by passing the original channel URI.

I get an another issue for synthsizing response while it's a same-origin request and a cors-mode response on fetch-response-taint.https.html. Note there is six failure but all of them fails on expected mode is not cors mode. I put one of the failures as follows: FAIL | /service-workers/service-worker/fetch-response-taint.https.html | fetching url:"https://web-platform.test:8443/?url=https%3A%2F%2Fwww1.web-platform.test%3A8443%2Fservice-workers%2Fservice-worker%2Fresources%2Ffetch-access-control.py%3FACAOrigin%3D*&mode=cors&credentials=omit&" mode:"same-origin" credentials:"omit" should succeed. Basically, if we create a quirk response just like the following script: var quirkResponse = new Response(corsResponse.body, corsResponse); It will make the type of the response be "default". However, this makes the final response become a response to a basic type [1]. If the description in LoadTainting is still correct [2], I think we should make its type be the "cors". [1] https://searchfox.org/mozilla-central/source/dom/fetch/InternalResponse.cpp#295-296 [2] https://searchfox.org/mozilla-central/source/netwerk/base/LoadTainting.h#20

Ben, this patch removes the wpt ini file, after the service worker propagates the response URLs to the content when intercepting a request.

(In reply to Tom Tung [:tt] from comment #76) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=7d733c015f56cc952c6d9d985ca3b1f6b9fb2278 This push-to-try applies the attached patches in this bug, the patch for freezing tainting (Attachment #8926229 [details] [diff]), and the patch for the simple implementation of the quirk response.

(In reply to Tom Tung [:tt] from comment #65) > Also, I have one more question about the perf. I find out that I can expose the pref to workers by adding one more item here [1]. [1] https://searchfox.org/mozilla-central/source/dom/workers/WorkerPrefs.h

Ben, this patch is mainly to add a pref and implement the quirk response. For the implementation, I'm a little afraid of whether the patch does the same as the scrip |var response = new Response(corRes.body, corRes)| does. (copy the body & header just like clone, but only copy the mStatus & mStatusText for InternalResponse metadata) Besides, I also copy the type of internalResponse to make type of quirkResponse be cors. For the pref, I enable it by default for all the platform. Furthermore, I expose it to worker so that the service worker can access it while it's in worker thread.

(In reply to Tom Tung [:tt] from comment #81) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=ae287b0b3d97699f816062be0f0ae8a393df1200 This try push is applied the P1 ~ P6 and the Attachment #8926229 [details] [diff]. I expect it to pass all the service worker related tests.

Tom, I'm afraid I won't have time to do these reviews today. I spent all day fixing some crashes I introduced before the holiday and catching up on other mail/reviews. I'll do these first thing tomorrow. Sorry for the delay.

(In reply to Tom Tung [:tt] from comment #65) > (In reply to Ben Kelly [:bkelly] from comment #58) > > I don't think we should need this one any more now that we are planning to > > reject/quirk the "CORS response on same-origin mode request" case. Do you > > agree? > > Sorry Ben, I feel like I still not understand about this. So, the original > reason for doing this patch is: > - SW intercepts an cross origin URL with "no-cors" mode and responds by > fetching another URL with "cors" mode and its headers > > Which response.type is the content excepted to have? It's "opaque" or > "cors"? > > Then, we decide to have this patch (freeze the tatinting so that it won't be > increase after the redirect) as your suggestion in comment #36. So that we > could get the response with its type is "cors" because it won't be increased > by nsContentSecurityManager. > > So, I don't understand why we don't need this patch? You're correct. I got confused. I thought this was for overriding the same-origin restriction in the security manager. Can you re-flag? > One is to read the perf while the service worker is registering/updating, > but we cannot be aware of the change of the pref immediately. The other one > is to jump into the main thread synchronously while we are in the condition > for the CORS response on same-origin mode request. However, I'm a a bit > afraid this might slow down the service worker. > > Do you have any suggestion about that? Thanks! Since we only need a boolean, I think you can use the WorkerPrefs.h stuff: https://searchfox.org/mozilla-central/source/dom/workers/WorkerPrefs.h#27 https://searchfox.org/mozilla-central/source/dom/canvas/ImageBitmap.cpp#1566 Does that help?

(In reply to Ben Kelly [:bkelly] from comment #84) > Since we only need a boolean, I think you can use the WorkerPrefs.h stuff: > > https://searchfox.org/mozilla-central/source/dom/workers/WorkerPrefs.h#27 > https://searchfox.org/mozilla-central/source/dom/canvas/ImageBitmap.cpp#1566 > > Does that help? Note that in Bug 1419771 which should land once :baku addresses weird Windows build failures, WorkerPrefs will no longer do things for boolean prefs, and instead DOMPreferences will come into existence and use the cool new Preferences::Add*Cache methods. So if you can wait a day or two, you should have those.

(In reply to Ben Kelly [:bkelly] from comment #84) Thanks for the hints and feedback! > You're correct. I got confused. I thought this was for overriding the > same-origin restriction in the security manager. > > Can you re-flag? Sure > Since we only need a boolean, I think you can use the WorkerPrefs.h stuff: > > https://searchfox.org/mozilla-central/source/dom/workers/WorkerPrefs.h#27 > https://searchfox.org/mozilla-central/source/dom/canvas/ImageBitmap.cpp#1566 > > Does that help? Thanks! I used it to expose the pref to the worker in P6 (attachment 8932052 [details] [diff] [review]) after investigating. That verifies I found a right method to use :P (In reply to Andrew Sutherland [:asuth] from comment #85) > Note that in Bug 1419771 which should land once :baku addresses weird > Windows build failures, WorkerPrefs will no longer do things for boolean > prefs, and instead DOMPreferences will come into existence and use the cool > new Preferences::Add*Cache methods. So if you can wait a day or two, you > should have those. Thanks for the information! I'll try to use DOMPreferences.

Comment on attachment 8926229 [details] [diff] [review] Freeze the tinting if a service worker responds with a synthesize repsonse Re-flag the patch. The original comment for this patch is in comment #53

Split the original P6 patch into two pieces. This one is for the implementation of the quirk response while the other is for the add a pref for it. I'll send the the other one to review after bug 1419771. Note: the original comment is in comment #80.

Comment on attachment 8931554 [details] [diff] [review] Bug 1222008 - P1: Propagate the URLs from respondWith() if the response's URL is different from the request's. r?bkelly Review of attachment 8931554 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/InterceptedHttpChannel.h @@ +114,5 @@ > FollowSyntheticRedirect(); > > + // If the response's URL is different from the request's then do a service > + // worker redirect. If Response.redirected is false we do an internal > + // redirect. Otherwise, if Response.redirect is true to a non-internal nit: s/to a/do a/ @@ +120,2 @@ > nsresult > + RedirectForResponseURL(nsIURI* aResponseURI, const bool aResponseRedirected); nit: I don't think the const is necessary on the bool here.

Comment on attachment 8932304 [details] [diff] [review] Bug 1222008 - P6a: Implement the quirk response Review of attachment 8932304 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/fetch/Response.cpp @@ +344,5 @@ > + mInternalResponse->Clone(body ? InternalResponse::eDontCloneInputStream > + : InternalResponse::eCloneInputStream, > + true /* quirkResponse */); > + > + RefPtr<Response> response = new Response(mOwner, ir, mSignal); I guess I was trying to suggest we do exactly what script can do here. So call `Response::Constructor(body, origResponse)`. I think this should just do the right thing. I guess we might have to handle a ReadableStream body vs a native body slightly differently. I don't think we need to clone the body, though. The Response::Constructor() should set the url and response type appropriately. It should look like a synthetic response created by the script. ::: dom/workers/ServiceWorkerEvents.cpp @@ +673,5 @@ > if (mRequestMode == RequestMode::Same_origin && > response->Type() == ResponseType::Cors) { > Telemetry::ScalarAdd(Telemetry::ScalarID::SW_CORS_RES_FOR_SO_REQ_COUNT, 1); > > + response = response->QuirkResponse(aCx, rv); Will the patch that adds the pref make us reject here if the quirk is disabled? I think thats what we want to do.

(In reply to Ben Kelly [:bkelly] from comment #90) Thanks for the reveiw and the feedback! But, I still have a question about expected repsonse type of quirk response, and I put it in below. > ::: dom/fetch/Response.cpp > @@ +344,5 @@ > > + mInternalResponse->Clone(body ? InternalResponse::eDontCloneInputStream > > + : InternalResponse::eCloneInputStream, > > + true /* quirkResponse */); > > + > > + RefPtr<Response> response = new Response(mOwner, ir, mSignal); > > I guess I was trying to suggest we do exactly what script can do here. So > call `Response::Constructor(body, origResponse)`. I think this should just > do the right thing. I guess we might have to handle a ReadableStream body > vs a native body slightly differently. I don't think we need to clone the > body, though. > > The Response::Constructor() should set the url and response type > appropriately. It should look like a synthetic response created by the > script. I'll try to call Response::Constructor(body, origResponse) on ServiceWorkerEvents to make the quirk response. However, if we only call Response::Constructor(body, origResponse)[1] to make the quirk response, then it might bring us to another issue. I checked the Response::Constructor()[1] and it doesn't set the response type from aInit (ResponseInit). It just create a new InternalResponse with aInit.mStatus and aInit.mStatusText. Thus, the response type of the synthesized response will be ResponseType::Default[2]. So that the final response type which the content script will get is ResponseType::Basic and that makes us not be able to pass the test fetch-response-taint.https.html (as I mentioned in comment #75). In short, which response type should the content script expect to get for the quirk response? Is it ResponseType::Cors or ResponseType::Basic? If it's ResponseType::Cors, then we need to set the response type except calling Response::Constructor(). If it's ResponseType::Basic, then we need to modify the test. [1] https://searchfox.org/mozilla-central/source/dom/fetch/Response.cpp#163 [2] https://searchfox.org/mozilla-central/source/dom/fetch/InternalResponse.cpp#30 > ::: dom/workers/ServiceWorkerEvents.cpp > @@ +673,5 @@ > > if (mRequestMode == RequestMode::Same_origin && > > response->Type() == ResponseType::Cors) { > > Telemetry::ScalarAdd(Telemetry::ScalarID::SW_CORS_RES_FOR_SO_REQ_COUNT, 1); > > > > + response = response->QuirkResponse(aCx, rv); > > Will the patch that adds the pref make us reject here if the quirk is > disabled? I think thats what we want to do. Yes, I'll have a check before this line like: + if (!DOMPreferences::QuirkResponseEnabled()) { + return; + } However, right now, I expose the pref to the worker it via WorkerProf which is about to be removed in bug 1419771. Thus, I'll send the P6b (the patch for add the pref, and expose to worker) once the bug 1419771 is fixed.

> I'll try to call Response::Constructor(body, origResponse) on > ServiceWorkerEvents to make the quirk response. > > However, if we only call Response::Constructor(body, origResponse)[1] to > make the quirk response, then it might bring us to another issue. > > I checked the Response::Constructor()[1] and it doesn't set the response > type from aInit (ResponseInit). It just create a new InternalResponse with > aInit.mStatus and aInit.mStatusText. Thus, the response type of the > synthesized response will be ResponseType::Default[2]. So that the final > response type which the content script will get is ResponseType::Basic and > that makes us not be able to pass the test fetch-response-taint.https.html > (as I mentioned in comment #75). Well, I think the test is going to have to change. Ideally we will reject in this case. Can we just mark it expected failure for now? Is the test flexible enough to just mark this one case expected failure? > In short, which response type should the content script expect to get for > the quirk response? Is it ResponseType::Cors or ResponseType::Basic? If it's > ResponseType::Cors, then we need to set the response type except calling > Response::Constructor(). If it's ResponseType::Basic, then we need to modify > the test. I think we should either reject or make it Basic with the quirk. Neither one completely matches the spec or the test right now. Hopefully we end up at rejecting and change the test. FWIW, I checked telemetry since you landed the probe. Currently we have ~2 million intercepted responses here: https://mzl.la/2Bvqkdj But not a single CORS-response-for-same-origin-request yet. The telemetry probe doesn't have any values in the site yet AFAICT. Obviously we need to give it more time, though. Getting it uplifted to beta would help. > However, right now, I expose the pref to the worker it via WorkerProf which > is about to be removed in bug 1419771. Thus, I'll send the P6b (the patch > for add the pref, and expose to worker) once the bug 1419771 is fixed. Ok. As long as we get the pref and rejection in before 59 merges to beta I think its ok.

I also asked the blink folks if they have any data yet: https://github.com/whatwg/fetch/issues/629#issuecomment-347741117 Maybe we should just go with rejection by default for now and punt the quirk/pref to a follow-on bug.

(In reply to Ben Kelly [:bkelly] from comment #92) Thanks for the quick reply! > Can we just mark it expected failure for now? Is the test flexible enough > to just mark this one case expected failure? Yes, I think we can. I'll mark it expected failure. > I think we should either reject or make it Basic with the quirk. Neither > one completely matches the spec or the test right now. Hopefully we end up > at rejecting and change the test. I see. (In reply to Ben Kelly [:bkelly] from comment #93) > I also asked the blink folks if they have any data yet: > > https://github.com/whatwg/fetch/issues/629#issuecomment-347741117 > > Maybe we should just go with rejection by default for now and punt the > quirk/pref to a follow-on bug. I could do that.

Update the patch

Ben, this patch becomes pretty simple after deciding to reject the promise by default. Could you help me to review this? Thanks!

Comment on attachment 8926229 [details] [diff] [review] Freeze the tinting if a service worker responds with a synthesize repsonse I find out that I forgot to set this patch not to be obsolete...

This patch is for expecting wpt tests failure since we decide to reject the promise

This patch is for expecting failure of loading a same-origin XML Doc but the sw synthesize a cors response.

Comment on attachment 8932786 [details] [diff] [review] Bug 1222008 - P6: Reject the cors synthesized response for same-origin request. r=bkelly Review of attachment 8932786 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. Thanks! ::: dom/workers/ServiceWorkerEvents.cpp @@ +674,5 @@ > response->Type() == ResponseType::Cors) { > Telemetry::ScalarAdd(Telemetry::ScalarID::SW_CORS_RES_FOR_SO_REQ_COUNT, 1); > > + // XXXtt: Will have a pref to enable the quirk response in bug 1419684. > + return; Please include a cancellation message so developers understand why its fail. Something like this: https://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.cpp#632

Comment on attachment 8926229 [details] [diff] [review] Freeze the tinting if a service worker responds with a synthesize repsonse Review of attachment 8926229 [details] [diff] [review]: ----------------------------------------------------------------- Looks good and I understand it better now. Thanks! ::: netwerk/base/LoadInfo.h @@ +183,5 @@ > bool mMixedContentWouldBlock : 1; > bool mIsHSTSPriming: 1; > bool mIsHSTSPrimingUpgrade: 1; > + > + bool mIsSynthesizeServiceWorkerTainting; Can you move this up above the bitfields and next to the other full `bool` methods? Right after mLoadTriggeredFromExternal. I think putting it here will create some struct packing wastage.

Comment on attachment 8932789 [details] [diff] [review] Bug 1222008 - P8: Expected failure for wpt tests since we mean to reject the cors response for same-origin request. r?bkelly Review of attachment 8932789 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/web-platform/meta/service-workers/service-worker/worker-interception.https.html.ini @@ +1,3 @@ > +[worker-interception.https.html] > + [Verify worker script intercepted by cors response succeeds] > + expected: FAIL Can you add an annotations like this to the failing test cases here? bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1419684 Or another bug to come back and fix the test once we finalize the spec? I guess maybe the spec issue link here would be good.

Comment on attachment 8932790 [details] [diff] [review] Bug 1222008 - P9: Expected failure for loading a same origin XML Document but being responded with a cors response. r?bkelly Review of attachment 8932790 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/test/serviceworkers/fetch/index.html @@ +139,5 @@ > var xmlDoc = document.implementation.createDocument(null, null, null); > xmlDoc.load('load_cross_origin_xml_document_cors.xml'); > xmlDoc.onload = function(evt) { > var content = new XMLSerializer().serializeToString(evt.target); > + // XXXtt: should be allowed by opening the pref after bug 1419684. Maybe just replace this comment with a link to the fetch spec issue.

Ben, could you help me to check the cancellation message for rejecting service workers to synthesize a cors response for a same-origin request? It's the inter-diff patch for P6 (attachment 8932786 [details] [diff] [review]). Thanks!

Comment on attachment 8933148 [details] [diff] [review] diff for P6 Review of attachment 8933148 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. ::: dom/locales/en-US/chrome/dom/dom.properties @@ +195,5 @@ > RTCPeerConnectionGetStreamsWarning=RTCPeerConnection.getLocalStreams/getRemoteStreams are deprecated. Use RTCPeerConnection.getSenders/getReceivers instead. > # LOCALIZATION NOTE: Do not translate "ServiceWorker". %S is a URL. > InterceptionFailedWithURL=Failed to load ‘%S’. A ServiceWorker intercepted the request and encountered an unexpected error. > +# LOCALIZATION NOTE: Do not translate "ServiceWorker", "cors", "Response", "same-origin" or "Request". > +CorsResponseForSameOriginRequest=ServcieWorker is not allowed to synthesize a cors Response for a same-origin Request. s/Servcie/Service I think we should probably log the request.url here. See what BadOpaqueInterceptionRequestModeWithURL does below.

(In reply to Ben Kelly [:bkelly] from comment #109) > I think we should probably log the request.url here. See what > BadOpaqueInterceptionRequestModeWithURL does below. Should we log the response.url as well? Or just the request.url?

Both URLs sounds good. Thanks.

Address the comment

(In reply to Tom Tung [:tt] from comment #112) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=1008383a7f0aefaab058804227f966f499c5569f If the result of this try goes green, then I will push them to inbound (In reply to Tom Tung [:tt] from comment #114) > Created attachment 8933213 [details] [diff] [review] > Bug 1222008 - P7: Freeze the tainting if a service worker responds with a > synthesize response. r=bkelly Note that I also change the variable name from "IsSynthesizeServiceWorkerTainting" to "ServiceWorkerTaintingSynthesized"

Sorry, I updated the wrong patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5aff04d76e0818e9b1f27ec5a76ba9fd6793607 Bug 1222008 - P1: Propagate the URLs from respondWith() if the response's URL is different from the request's. r=bkelly https://hg.mozilla.org/integration/mozilla-inbound/rev/cc17f19f55986bad0c1dd70939b949152c0de9b7 Bug 1222008 - P2: Fix the netwerk xpcshell test since adding an argument to startSynthesizedResponse(). r=bkelly https://hg.mozilla.org/integration/mozilla-inbound/rev/1beda261a70875f1155d7ea0f3bcb7d329de1789 Bug 1222008 - P3: Fix the test_fetch_basic_http_sw_reroute.html since we decide to propagate the response's URLs to the content. r=bkelly https://hg.mozilla.org/integration/mozilla-inbound/rev/efa0d71107dfcc5b30fac1e5dd72f10e7226e6a8 Bug 1222008 - P4: Fix the interception test since we decide to propagate the response's URLs to the content. r=bkelly https://hg.mozilla.org/integration/mozilla-inbound/rev/f112fa183600e6b8e14485147a2b20cef5a0565d Bug 1222008 - P5: Remove wpt ini file since passing the test. r=bkelly https://hg.mozilla.org/integration/mozilla-inbound/rev/c29e6b8b00b618f67b83c20527b449b98834e1eb Bug 1222008 - P6: Reject the cors synthesized response for same-origin request. r=bkelly https://hg.mozilla.org/integration/mozilla-inbound/rev/8e66dd4b9241a14d7cb5be77404011569d498dcb Bug 1222008 - P7: Freeze the tainting if a service worker responds with a synthesize response. r=bkelly https://hg.mozilla.org/integration/mozilla-inbound/rev/4a7ed01915d23df60989b6b38b2916889fb79914 Bug 1222008 - P8: Expected failure for wpt tests since we mean to reject the cors response for same-origin request. r=bkelly https://hg.mozilla.org/integration/mozilla-inbound/rev/22450268a82f78f84c1db8776f0059c1a1fe7d97 Bug 1222008 - P9: Expected failure for loading a same origin XML Document but being responded with a cors response. r=bkelly

https://hg.mozilla.org/mozilla-central/rev/e5aff04d76e0 https://hg.mozilla.org/mozilla-central/rev/cc17f19f5598 https://hg.mozilla.org/mozilla-central/rev/1beda261a708 https://hg.mozilla.org/mozilla-central/rev/efa0d71107df https://hg.mozilla.org/mozilla-central/rev/f112fa183600 https://hg.mozilla.org/mozilla-central/rev/c29e6b8b00b6 https://hg.mozilla.org/mozilla-central/rev/8e66dd4b9241 https://hg.mozilla.org/mozilla-central/rev/4a7ed01915d2 https://hg.mozilla.org/mozilla-central/rev/22450268a82f

Liz, this is another one that should be in the 59 developer notes. Not sure the best way to get it in there now. Can you help me? It can probably be listed as two different items: * Service worker interception now propagates the FetchEvent.respondWith()'s Response.url to the outer network request. (bug 1222008) * FetchEvent.respondWith() triggers a NetworkError if the FetchEvent.request.mode is "same-origin" and the Response.type is "cors". (bug 1222008)

I'm going to manually add this to the developer release notes here: https://developer.mozilla.org/en-US/Firefox/Releases/59 Leaving dev-doc-needed because the documentation should still be updated on MDN I think.

I wrote this in the release notes: * When a service worker provides a Response to FetchEvent.respondWith(), the Response.url value will not be propagated to the intercepted network request as the final resolved URL. In the past the FetchEvent.request.url was used for this instead. This means, for example, if a service worker intercepts a stylesheet or worker script, then the provided Response.url will be used to resolve any relative @import() or importScripts() subresource loads. (bug 1222008) * FetchEvent.respondWith() will now trigger a network error if the FetchEvent.request.mode is "same-origin" and the provided Response.type is "cors". (bug 1222008)

(In reply to Ben Kelly [:bkelly] from comment #123) > I wrote this in the release notes: > > * When a service worker provides a Response to FetchEvent.respondWith(), the > Response.url value will not be propagated to the intercepted network request > as the final resolved URL. In the past the FetchEvent.request.url was used > for this instead. This means, for example, if a service worker intercepts a > stylesheet or worker script, then the provided Response.url will be used to > resolve any relative @import() or importScripts() subresource loads. (bug > 1222008) > > * FetchEvent.respondWith() will now trigger a network error if the > FetchEvent.request.mode is "same-origin" and the provided Response.type is > "cors". (bug 1222008) Hi Ben, Thanks for doing this. I've updated the release notes to include links to all the relevant ref pages. In terms of documenting this on the ref pages, I've added some information to: https://developer.mozilla.org/en-US/docs/Web/API/FetchEvent/respondWith (see the "Exceptions" and "Browser compatibility" sections - let me know if this reads OK). But I'm not really sure what to write about the first bullet you added. What page should be updated, and what should the text be?

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #124) > (In reply to Ben Kelly [:bkelly] from comment #123) > > I wrote this in the release notes: > > > > * When a service worker provides a Response to FetchEvent.respondWith(), the > > Response.url value will not be propagated to the intercepted network request > > as the final resolved URL. In the past the FetchEvent.request.url was used > > for this instead. This means, for example, if a service worker intercepts a > > stylesheet or worker script, then the provided Response.url will be used to > > resolve any relative @import() or importScripts() subresource loads. (bug > > 1222008) > > > > * FetchEvent.respondWith() will now trigger a network error if the > > FetchEvent.request.mode is "same-origin" and the provided Response.type is > > "cors". (bug 1222008) > > Hi Ben, > > Thanks for doing this. I've updated the release notes to include links to > all the relevant ref pages. In terms of documenting this on the ref pages, > I've added some information to: > > https://developer.mozilla.org/en-US/docs/Web/API/FetchEvent/respondWith > > (see the "Exceptions" and "Browser compatibility" sections - let me know if > this reads OK). That's not quite correct. There were existing combinations of Request.mode and Response.type that would trigger a network error. We just added an additional one specific to `request.mode === 'same-origin' && response.type === 'cors'`. The relevant spec issue is here if it helps: https://github.com/whatwg/fetch/issues/629 > But I'm not really sure what to write about the first bullet you added. What > page should be updated, and what should the text be? Yea, its hard to describe. Its really about how service worker interception interacts with the network request being intercepted. The impact depends on what is being intercepted. For most types of network requests this change has no impact because you can't observe the final URL. There are a few, though, where it does matter: 1. If fetch() is intercepted, then you can observe the final URL on the result Response.url. 2. If a worker script is intercepted, then the final URL is used to set self.location and used as the base URL for relative URLs in the worker script. 3. If a stylesheet is intercepted, then the final URL is used as the base URL for resolving relative @import loads. This was changed in the spec years ago: https://github.com/whatwg/fetch/pull/146#issuecomment-341710548 But we are only now implementing it. In other browsers its only partially implemented. For example, chrome will surface the URL in an intercepted fetch(), but does not use the final URL for worker scripts. (It doesn't use final URL for worker scripts doing a normal redirect either, so maybe its just an unrelated bug.) Note, navigation requests don't surface the final URL because of oddities of the navigation spec language. It ends up using the request URL there. Maybe this can go on FetchEvent.resolveWith() page as a special note?

Chris, chrome is also working on this under this feature: https://www.chromestatus.com/feature/5694278818856960

Hi Ben, I've had another go at this, having added some information on both points to the respondWith page: https://developer.mozilla.org/en-US/docs/Web/API/FetchEvent/respondWith Does this stuff sound OK? I've constructed it from your text. Feel free to edit as appropriate.

Thanks Chris. It looked mostly good, but I did make a few edits. I also added a paragraph about window navigation requests. Do you think you could help me linkify that paragraph?

(In reply to Ben Kelly [:bkelly] from comment #128) > Thanks Chris. It looked mostly good, but I did make a few edits. I also > added a paragraph about window navigation requests. Do you think you could > help me linkify that paragraph? This looks great; I've linkified it, and made a couple of additional edits. Thanks a lot for your help on this.