Set SkipServiceWorker flag in DocumentThreadableLoader::loadActualRequest().

Set SkipServiceWorker flag in DocumentThreadableLoader::loadActualRequest().

Before a SW controls the page, when the DTL sends a CORS preflight request, the SkipServiceWorker flag is set by RenderFrameImpl::willSendRequest().
But a new SW may be controlling the page when the DTL sends the actual request by calling clients.claim().
In such case, the request goes to the SW and causes several problems.

To avoid this problem, this cl set the flag in DTL::loadActualRequest().

BUG=604583, 610400
Committed: https://crrev.com/01fb3c2cf2bf84391c488f3a3dd5ef8c59cc0a51
Cr-Commit-Position: refs/heads/master@{#392609}

[horo, 2016-05-10 07:13:09]

Description was changed from

==========
Set SkipServiceWorker flag in DocumentThreadableLoader::loadActualRequest().


BUG=604583,610400
==========

to

==========
Set SkipServiceWorker flag in DocumentThreadableLoader::loadActualRequest().

Before a SW controls the page, when the DTL sends a CORS preflight request, the
SkipServiceWorker flag is set by RenderFrameImpl::willSendRequest().
But a new SW may be controlling the page when the DTL sends the actual request
by calling clients.claim().
In such case, the request goes to the SW and causes several problems.

To avoid this problem, this cl set the flag in DTL::loadActualRequest().

BUG=604583,610400
==========

[horo, 2016-05-10 07:17:05]

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

[horo, 2016-05-10 07:17:05]

tyoshino@
Could you please review this?

[tyoshino (SeeGerritForStatus), 2016-05-10 09:17:02]

https://codereview.chromium.org/1964823002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1964823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:813:
actualRequest.setSkipServiceWorker(true);
So, the approach is to have the series of requests following a request which has
been handled without SW?

As I chatted offline, I'm guessing that similar issue may happen in following
redirect chain.

This code change looks good, but shouldn't we just mark the request in
DocumentThreadableLoader::start() so that it and its child requests never go
through SW?

[horo, 2016-05-10 13:20:27]

https://codereview.chromium.org/1964823002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1964823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:813:
actualRequest.setSkipServiceWorker(true);
On 2016/05/10 09:17:01, tyoshino wrote:
> So, the approach is to have the series of requests following a request which
has
> been handled without SW?
> 
> As I chatted offline, I'm guessing that similar issue may happen in following
> redirect chain.
> 
> This code change looks good, but shouldn't we just mark the request in
> DocumentThreadableLoader::start() so that it and its child requests never go
> through SW?

In the redirect case, the request is passed from
WebURLLoaderImpl::Context::OnReceivedRedirect() to
DocumentThreadableLoader::redirectReceived(). The SkipServiceWorker flag of the
request must be true if the page was not controlled by a SW when the first
request was sent. So I think there is no problem in the redirect case.

(And also we can't simply set the flag in DocumentThreadableLoader::start()
because the flag must be false when the request is for SharedWorker script and
ServiceWorker script and the scripts imported from those workers.)

[tyoshino (SeeGerritForStatus), 2016-05-10 13:52:48]

lgtm

https://codereview.chromium.org/1964823002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1964823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:813:
actualRequest.setSkipServiceWorker(true);
On 2016/05/10 13:20:27, horo wrote:
> On 2016/05/10 09:17:01, tyoshino wrote:
> > So, the approach is to have the series of requests following a request which
> has
> > been handled without SW?
> > 
> > As I chatted offline, I'm guessing that similar issue may happen in
following
> > redirect chain.
> > 
> > This code change looks good, but shouldn't we just mark the request in
> > DocumentThreadableLoader::start() so that it and its child requests never go
> > through SW?
> 
> In the redirect case, the request is passed from
> WebURLLoaderImpl::Context::OnReceivedRedirect() to
> DocumentThreadableLoader::redirectReceived(). The SkipServiceWorker flag of
the
> request must be true if the page was not controlled by a SW when the first
> request was sent. So I think there is no problem in the redirect case.
> 
> (And also we can't simply set the flag in DocumentThreadableLoader::start()
> because the flag must be false when the request is for SharedWorker script and
> ServiceWorker script and the scripts imported from those workers.)
> 

ok

[horo, 2016-05-10 14:00:02]

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

[horo, 2016-05-10 14:00:03]

kouhei@
Could you please review this?

[kinuko, 2016-05-10 14:11:19]

drive-by lgtm

[horo, 2016-05-10 14:23:16]

Thank you.

[horo, 2016-05-10 14:23:21]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2016-05-10 14:23:30]

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

[kouhei (in TOK), 2016-05-10 14:29:30]

On 2016/05/10 14:00:03, horo wrote:
> kouhei@
> Could you please review this?

lgtm

[commit-bot: I haz the power, 2016-05-10 15:20:07]

Description was changed from

==========
Set SkipServiceWorker flag in DocumentThreadableLoader::loadActualRequest().

Before a SW controls the page, when the DTL sends a CORS preflight request, the
SkipServiceWorker flag is set by RenderFrameImpl::willSendRequest().
But a new SW may be controlling the page when the DTL sends the actual request
by calling clients.claim().
In such case, the request goes to the SW and causes several problems.

To avoid this problem, this cl set the flag in DTL::loadActualRequest().

BUG=604583,610400
==========

to

==========
Set SkipServiceWorker flag in DocumentThreadableLoader::loadActualRequest().

Before a SW controls the page, when the DTL sends a CORS preflight request, the
SkipServiceWorker flag is set by RenderFrameImpl::willSendRequest().
But a new SW may be controlling the page when the DTL sends the actual request
by calling clients.claim().
In such case, the request goes to the SW and causes several problems.

To avoid this problem, this cl set the flag in DTL::loadActualRequest().

BUG=604583,610400
==========

[commit-bot: I haz the power, 2016-05-10 15:20:08]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-05-10 15:22:20]

Description was changed from

==========
Set SkipServiceWorker flag in DocumentThreadableLoader::loadActualRequest().

Before a SW controls the page, when the DTL sends a CORS preflight request, the
SkipServiceWorker flag is set by RenderFrameImpl::willSendRequest().
But a new SW may be controlling the page when the DTL sends the actual request
by calling clients.claim().
In such case, the request goes to the SW and causes several problems.

To avoid this problem, this cl set the flag in DTL::loadActualRequest().

BUG=604583,610400
==========

to

==========
Set SkipServiceWorker flag in DocumentThreadableLoader::loadActualRequest().

Before a SW controls the page, when the DTL sends a CORS preflight request, the
SkipServiceWorker flag is set by RenderFrameImpl::willSendRequest().
But a new SW may be controlling the page when the DTL sends the actual request
by calling clients.claim().
In such case, the request goes to the SW and causes several problems.

To avoid this problem, this cl set the flag in DTL::loadActualRequest().

BUG=604583,610400

Committed: https://crrev.com/01fb3c2cf2bf84391c488f3a3dd5ef8c59cc0a51
Cr-Commit-Position: refs/heads/master@{#392609}
==========

[commit-bot: I haz the power, 2016-05-10 15:22:22]

Patchset 1 (id:??) landed as
https://crrev.com/01fb3c2cf2bf84391c488f3a3dd5ef8c59cc0a51
Cr-Commit-Position: refs/heads/master@{#392609}