[3/3 blink] Support redirect option of Request and "opaqueredirect" response type.

[3/3 blink] Support redirect option of Request and "opaqueredirect" response type.

https://crbug.com/510650#c4 describes the details of the data flow.

1/3 blink: https://codereview.chromium.org/1265133002/
2/3 chromium: https://codereview.chromium.org/1271733002/
3/3 blink: This

BUG=510650, 517837
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200815

[falken, 2015-08-07 07:45:18]

falken@chromium.org changed reviewers:
+ falken@chromium.org

[falken, 2015-08-07 07:45:18]

I was expecting to see tests in this patch. Will you add them?

https://codereview.chromium.org/1280733002/diff/1/Source/modules/fetch/Reques...
File Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1280733002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.cpp:159: if (init.redirect == "follw") {
follow

[horo, 2015-08-07 10:04:13]

https://codereview.chromium.org/1280733002/diff/1/Source/modules/fetch/Reques...
File Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1280733002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.cpp:159: if (init.redirect == "follw") {
On 2015/08/07 07:45:18, falken wrote:
> follow

Done.

[horo, 2015-08-07 10:05:24]

On 2015/08/07 07:45:18, falken wrote:
> I was expecting to see tests in this patch. Will you add them?

Yes. I will add tests in this patch.
So I added "[WIP]" in the title of this cl now.

[horo, 2015-08-13 04:54:17]

horo@chromium.org changed reviewers:
+ yhirano@chromium.org

[horo, 2015-08-13 04:54:18]

falken@, yhirano@
Could you please review this patch?
I added layout tests.

[falken, 2015-08-13 06:42:30]

serviceworker lgtm

https://codereview.chromium.org/1280733002/diff/100001/LayoutTests/http/tests...
File LayoutTests/http/tests/fetch/script-tests/fetch.js (right):

https://codereview.chromium.org/1280733002/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/fetch/script-tests/fetch.js:150: }, 'No redirect fetch
completes normally even if redirect mode is manual');
This was hard to parse. Maybe "No-redirect fetch completes normally..."

https://codereview.chromium.org/1280733002/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/fetch/script-tests/fetch.js:162: }, 'No redirect fetch
completes normally even if redirect mode is error');
same here

https://codereview.chromium.org/1280733002/diff/100001/LayoutTests/http/tests...
File LayoutTests/http/tests/serviceworker/fetch-request-redirect.html (right):

https://codereview.chromium.org/1280733002/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/serviceworker/fetch-request-redirect.html:6: <script
src="resources/test-helpers.js?pipe=sub"></script>
pipe-sub should be on get-host-info

https://codereview.chromium.org/1280733002/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/serviceworker/fetch-request-redirect.html:96:
'&redirect-mode=error'),
manual

https://codereview.chromium.org/1280733002/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/serviceworker/fetch-request-redirect.html:159:
'Redirected iframe loading with with Request.redirect=follow '+
with with (and below)

https://codereview.chromium.org/1280733002/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/serviceworker/fetch-request-redirect.html:171:
'Redirected iframe loading with with Request.redirect=follow '+
redirect=manual

[horo, 2015-08-13 10:46:36]

https://codereview.chromium.org/1280733002/diff/100001/LayoutTests/http/tests...
File LayoutTests/http/tests/fetch/script-tests/fetch.js (right):

https://codereview.chromium.org/1280733002/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/fetch/script-tests/fetch.js:150: }, 'No redirect fetch
completes normally even if redirect mode is manual');
On 2015/08/13 06:42:30, falken wrote:
> This was hard to parse. Maybe "No-redirect fetch completes normally..."

Done.

https://codereview.chromium.org/1280733002/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/fetch/script-tests/fetch.js:162: }, 'No redirect fetch
completes normally even if redirect mode is error');
On 2015/08/13 06:42:30, falken wrote:
> same here

Done.

https://codereview.chromium.org/1280733002/diff/100001/LayoutTests/http/tests...
File LayoutTests/http/tests/serviceworker/fetch-request-redirect.html (right):

https://codereview.chromium.org/1280733002/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/serviceworker/fetch-request-redirect.html:6: <script
src="resources/test-helpers.js?pipe=sub"></script>
On 2015/08/13 06:42:30, falken wrote:
> pipe-sub should be on get-host-info

Done.

https://codereview.chromium.org/1280733002/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/serviceworker/fetch-request-redirect.html:96:
'&redirect-mode=error'),
On 2015/08/13 06:42:30, falken wrote:
> manual

Done.

https://codereview.chromium.org/1280733002/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/serviceworker/fetch-request-redirect.html:159:
'Redirected iframe loading with with Request.redirect=follow '+
On 2015/08/13 06:42:30, falken wrote:
> with with (and below)

Done.

https://codereview.chromium.org/1280733002/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/serviceworker/fetch-request-redirect.html:171:
'Redirected iframe loading with with Request.redirect=follow '+
On 2015/08/13 06:42:30, falken wrote:
> redirect=manual

Done.

[horo, 2015-08-13 10:46:36]

https://codereview.chromium.org/1280733002/diff/100001/LayoutTests/http/tests...
File LayoutTests/http/tests/fetch/script-tests/fetch.js (right):

https://codereview.chromium.org/1280733002/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/fetch/script-tests/fetch.js:150: }, 'No redirect fetch
completes normally even if redirect mode is manual');
On 2015/08/13 06:42:30, falken wrote:
> This was hard to parse. Maybe "No-redirect fetch completes normally..."

Done.

https://codereview.chromium.org/1280733002/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/fetch/script-tests/fetch.js:162: }, 'No redirect fetch
completes normally even if redirect mode is error');
On 2015/08/13 06:42:30, falken wrote:
> same here

Done.

https://codereview.chromium.org/1280733002/diff/100001/LayoutTests/http/tests...
File LayoutTests/http/tests/serviceworker/fetch-request-redirect.html (right):

https://codereview.chromium.org/1280733002/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/serviceworker/fetch-request-redirect.html:6: <script
src="resources/test-helpers.js?pipe=sub"></script>
On 2015/08/13 06:42:30, falken wrote:
> pipe-sub should be on get-host-info

Done.

https://codereview.chromium.org/1280733002/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/serviceworker/fetch-request-redirect.html:96:
'&redirect-mode=error'),
On 2015/08/13 06:42:30, falken wrote:
> manual

Done.

https://codereview.chromium.org/1280733002/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/serviceworker/fetch-request-redirect.html:159:
'Redirected iframe loading with with Request.redirect=follow '+
On 2015/08/13 06:42:30, falken wrote:
> with with (and below)

Done.

https://codereview.chromium.org/1280733002/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/serviceworker/fetch-request-redirect.html:171:
'Redirected iframe loading with with Request.redirect=follow '+
On 2015/08/13 06:42:30, falken wrote:
> redirect=manual

Done.

[horo, 2015-08-18 00:52:11]

yhirano@
Could you please review this patch?

[yhirano, 2015-08-18 07:56:49]

https://codereview.chromium.org/1280733002/diff/160001/LayoutTests/http/tests...
File LayoutTests/http/tests/serviceworker/fetch-request-redirect.html (right):

https://codereview.chromium.org/1280733002/diff/160001/LayoutTests/http/tests...
LayoutTests/http/tests/serviceworker/fetch-request-redirect.html:10: return new
Promise(function(resolve, reject) {
Is this different from |promise.catch(() => throw description)|?

https://codereview.chromium.org/1280733002/diff/160001/LayoutTests/http/tests...
LayoutTests/http/tests/serviceworker/fetch-request-redirect.html:18: function
assert_rejects(promise, description) {
ditto for |promise.then(() => throw description, () => {})|

https://codereview.chromium.org/1280733002/diff/160001/LayoutTests/http/tests...
LayoutTests/http/tests/serviceworker/fetch-request-redirect.html:97: 'Redirected
XHR with Request.redirect=manual should fail.'),
Sorry, I don't understand why this test case should fail. Can you tell me?

https://codereview.chromium.org/1280733002/diff/160001/LayoutTests/http/tests...
File
LayoutTests/http/tests/serviceworker/resources/fetch-request-redirect-iframe.html
(right):

https://codereview.chromium.org/1280733002/diff/160001/LayoutTests/http/tests...
LayoutTests/http/tests/serviceworker/resources/fetch-request-redirect-iframe.html:20:
img.onload = function() { resolve(); };
[optional] img.onload = resolve;
Ditto bellow.

https://codereview.chromium.org/1280733002/diff/160001/Source/core/loader/Doc...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1280733002/diff/160001/Source/core/loader/Doc...
Source/core/loader/DocumentThreadableLoader.cpp:274: responseReceived(resource,
redirectResponse, nullptr);
Please create and pass a valid handle when useStreamOnResponse is specified.

https://codereview.chromium.org/1280733002/diff/160001/Source/modules/fetch/F...
File Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/1280733002/diff/160001/Source/modules/fetch/F...
Source/modules/fetch/FetchManager.cpp:105: m_responseHttpStatusCode =
response.httpStatusCode();
Please keep the assertion.

[yhirano, 2015-08-18 08:28:20]

We talked about the same topic in the previous CL, but let me talk about the
response body.
Please see [1]. When you access green.html, you can see the image shown and
|onload| is called though the status code is 400.
I think the same thing should happen with redirect status code when
redirect=manual is set.

// In the controlled page:
<img src="img.png">

// In the serviceworker
self.addEventListener('fetch', e => e.respondWith(fetch(new Request(e.request,
{redirect: 'manual'}))));

--
When the server returns a 301 response with a valid image body for img.png, it
should be shown in the controlled page. i.e. We need to handle the response body
correctly.

1: https://codereview.chromium.org/1295173005/

[horo, 2015-08-18 09:47:21]

On 2015/08/18 08:28:20, yhirano wrote:
> We talked about the same topic in the previous CL, but let me talk about the
> response body.
> Please see [1]. When you access green.html, you can see the image shown and
> |onload| is called though the status code is 400.
> I think the same thing should happen with redirect status code when
> redirect=manual is set.
> 
> // In the controlled page:
> <img src="img.png">
> 
> // In the serviceworker
> self.addEventListener('fetch', e => e.respondWith(fetch(new Request(e.request,
> {redirect: 'manual'}))));

This will cause a network error while loading "img.png" in the page. 

https://fetch.spec.whatwg.org/#http-fetch
> 5.3 HTTP fetch
> 2. ...
> 2.2. If one of the following conditions is true, return a network error:
> ...
> request is not a navigation request and response's type is "opaqueredirect".

> --
> When the server returns a 301 response with a valid image body for img.png, it
> should be shown in the controlled page. i.e. We need to handle the response
body
> correctly.
> 
> 1: https://codereview.chromium.org/1295173005/

Are you saying "a 301 response with a valid image body without 'Location'
header"?
I think we can ignore the body of 301 response with 'Location' header because
the browser should follow the redirect.
.

[yhirano, 2015-08-18 12:46:26]

Thanks, I see that I was wrong.

[yhirano, 2015-08-18 12:47:08]

https://codereview.chromium.org/1280733002/diff/160001/LayoutTests/http/tests...
File LayoutTests/http/tests/serviceworker/fetch-request-redirect.html (right):

https://codereview.chromium.org/1280733002/diff/160001/LayoutTests/http/tests...
LayoutTests/http/tests/serviceworker/fetch-request-redirect.html:97: 'Redirected
XHR with Request.redirect=manual should fail.'),
On 2015/08/18 07:56:48, yhirano wrote:
> Sorry, I don't understand why this test case should fail. Can you tell me?

I see the reason now - because it is not a navigation request.

[horo, 2015-08-19 01:18:58]

Patchset #10 (id:180001) has been deleted

[horo, 2015-08-19 01:30:22]

Patchset #10 (id:200001) has been deleted

[horo, 2015-08-19 07:32:18]

Patchset #10 (id:220001) has been deleted

[horo, 2015-08-19 07:36:12]

https://codereview.chromium.org/1280733002/diff/160001/LayoutTests/http/tests...
File LayoutTests/http/tests/serviceworker/fetch-request-redirect.html (right):

https://codereview.chromium.org/1280733002/diff/160001/LayoutTests/http/tests...
LayoutTests/http/tests/serviceworker/fetch-request-redirect.html:10: return new
Promise(function(resolve, reject) {
On 2015/08/18 07:56:48, yhirano wrote:
> Is this different from |promise.catch(() => throw description)|?

Done.

https://codereview.chromium.org/1280733002/diff/160001/LayoutTests/http/tests...
LayoutTests/http/tests/serviceworker/fetch-request-redirect.html:18: function
assert_rejects(promise, description) {
On 2015/08/18 07:56:48, yhirano wrote:
> ditto for |promise.then(() => throw description, () => {})|

Done.

https://codereview.chromium.org/1280733002/diff/160001/LayoutTests/http/tests...
File
LayoutTests/http/tests/serviceworker/resources/fetch-request-redirect-iframe.html
(right):

https://codereview.chromium.org/1280733002/diff/160001/LayoutTests/http/tests...
LayoutTests/http/tests/serviceworker/resources/fetch-request-redirect-iframe.html:20:
img.onload = function() { resolve(); };
On 2015/08/18 07:56:48, yhirano wrote:
> [optional] img.onload = resolve;
> Ditto bellow.

Done.

https://codereview.chromium.org/1280733002/diff/160001/Source/core/loader/Doc...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1280733002/diff/160001/Source/core/loader/Doc...
Source/core/loader/DocumentThreadableLoader.cpp:274: responseReceived(resource,
redirectResponse, nullptr);
On 2015/08/18 07:56:49, yhirano wrote:
> Please create and pass a valid handle when useStreamOnResponse is specified.

Done.
I introduced EmptyDataHandle.

https://codereview.chromium.org/1280733002/diff/160001/Source/modules/fetch/F...
File Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/1280733002/diff/160001/Source/modules/fetch/F...
Source/modules/fetch/FetchManager.cpp:105: m_responseHttpStatusCode =
response.httpStatusCode();
On 2015/08/18 07:56:49, yhirano wrote:
> Please keep the assertion.

Done.

[yhirano, 2015-08-19 07:54:52]

https://codereview.chromium.org/1280733002/diff/240001/Source/core/loader/Doc...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1280733002/diff/240001/Source/core/loader/Doc...
Source/core/loader/DocumentThreadableLoader.cpp:317: if (m_redirectMode ==
WebURLRequest::FetchRedirectModeManual) {
Can't we use request.fetchRedirectMode() instead of m_redirectMode?

[horo, 2015-08-19 08:23:04]

https://codereview.chromium.org/1280733002/diff/240001/Source/core/loader/Doc...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1280733002/diff/240001/Source/core/loader/Doc...
Source/core/loader/DocumentThreadableLoader.cpp:317: if (m_redirectMode ==
WebURLRequest::FetchRedirectModeManual) {
On 2015/08/19 07:54:52, yhirano wrote:
> Can't we use request.fetchRedirectMode() instead of m_redirectMode?

No we can't.
This |request| is a new request for redirect which is created in
WebURLLoaderImpl::Context::OnReceivedRedirect().
And we don't set the RedirectMode of it, because the redirect mode of new
request for redirect must be "follow".

[yhirano, 2015-08-19 08:32:15]

lgtm

https://codereview.chromium.org/1280733002/diff/240001/Source/core/loader/Doc...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1280733002/diff/240001/Source/core/loader/Doc...
Source/core/loader/DocumentThreadableLoader.cpp:317: if (m_redirectMode ==
WebURLRequest::FetchRedirectModeManual) {
On 2015/08/19 08:23:04, horo wrote:
> On 2015/08/19 07:54:52, yhirano wrote:
> > Can't we use request.fetchRedirectMode() instead of m_redirectMode?
> 
> No we can't.
> This |request| is a new request for redirect which is created in
> WebURLLoaderImpl::Context::OnReceivedRedirect().
> And we don't set the RedirectMode of it, because the redirect mode of new
> request for redirect must be "follow".

Acknowledged.

[horo, 2015-08-19 08:39:27]

horo@chromium.org changed reviewers:
+ kinuko@chromium.org

[horo, 2015-08-19 08:39:27]

kinuko@
Could you please review Source/core/loader/DocumentThreadableLoader.*?

[kinuko, 2015-08-19 08:58:18]

lgtm

https://codereview.chromium.org/1280733002/diff/240001/Source/core/loader/Doc...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1280733002/diff/240001/Source/core/loader/Doc...
Source/core/loader/DocumentThreadableLoader.cpp:317: if (m_redirectMode ==
WebURLRequest::FetchRedirectModeManual) {
On 2015/08/19 08:32:15, yhirano wrote:
> On 2015/08/19 08:23:04, horo wrote:
> > On 2015/08/19 07:54:52, yhirano wrote:
> > > Can't we use request.fetchRedirectMode() instead of m_redirectMode?
> > 
> > No we can't.
> > This |request| is a new request for redirect which is created in
> > WebURLLoaderImpl::Context::OnReceivedRedirect().
> > And we don't set the RedirectMode of it, because the redirect mode of new
> > request for redirect must be "follow".
> 
> Acknowledged.

Adding a comment about it might help readers a bit.

[horo, 2015-08-19 09:47:02]

https://codereview.chromium.org/1280733002/diff/240001/Source/core/loader/Doc...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1280733002/diff/240001/Source/core/loader/Doc...
Source/core/loader/DocumentThreadableLoader.cpp:317: if (m_redirectMode ==
WebURLRequest::FetchRedirectModeManual) {
On 2015/08/19 08:58:18, kinuko wrote:
> On 2015/08/19 08:32:15, yhirano wrote:
> > On 2015/08/19 08:23:04, horo wrote:
> > > On 2015/08/19 07:54:52, yhirano wrote:
> > > > Can't we use request.fetchRedirectMode() instead of m_redirectMode?
> > > 
> > > No we can't.
> > > This |request| is a new request for redirect which is created in
> > > WebURLLoaderImpl::Context::OnReceivedRedirect().
> > > And we don't set the RedirectMode of it, because the redirect mode of new
> > > request for redirect must be "follow".
> > 
> > Acknowledged.
> 
> Adding a comment about it might help readers a bit.

Done.

[horo, 2015-08-19 09:52:29]

The CQ bit was checked by horo@chromium.org

[horo, 2015-08-19 09:52:29]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
kinuko@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/1280733002/#ps260001
(title: "add comment")

[commit-bot: I haz the power, 2015-08-19 09:52:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1280733002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1280733002/260001

[commit-bot: I haz the power, 2015-08-19 11:12:30]

Committed patchset #11 (id:260001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200815

[jsbell, 2015-08-19 17:13:55]

jsbell@chromium.org changed reviewers:
+ jsbell@chromium.org

[jsbell, 2015-08-19 17:13:56]

http/tests/serviceworker/fetch-request-redirect.html appears to be failing:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=htt...

FAIL Verify redirect mode of Fetch API and ServiceWorker FetchEvent.
assert_unreached: unexpected rejection: Redirected iframe loading with
Request.redirect=follow should succeed. Reached unreachable code

(I'm also seeing this locally)

[jsbell, 2015-08-19 17:54:37]

On 2015/08/19 17:13:56, jsbell wrote:
> http/tests/serviceworker/fetch-request-redirect.html appears to be failing:
> 
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=htt...
> 
> FAIL Verify redirect mode of Fetch API and ServiceWorker FetchEvent.
> assert_unreached: unexpected rejection: Redirected iframe loading with
> Request.redirect=follow should succeed. Reached unreachable code
> 
> (I'm also seeing this locally)

Looks like the timeout is firing unexpectedly. This makes sense - on a slow box
under debug or MSAN (etc), and with so much happening in parallel, one of the
iframes could take more than 500ms to load.

We should disable this test and figure out an alternate approach.