Introduce a new ResourceRequesterInfo type for NavigationPreload.

Introduce a new ResourceRequesterInfo type for NavigationPreload.

Currently we use the ResourceRequesterInfo of the original navigation request in
ServiceWorkerFetchDispatcher to send service worker navigation preload
requests. But when BrowserSideNavigation (PlzNavigate) is enabled, the
ResourceRequesterInfo of the original navigation request is not renderer type,
but browser side navigation type. And we can't send the navigation preload
request.

To solve this problem, this CL introduces a new ResourceRequesterInfo type for
service worker navigation preload requests.
ResourceRequesterInfo::CreateForNavigationPreload() creates the new type info
from the original navigation request info.

This CL depends on https://codereview.chromium.org/2481093003/ which introduced
ResourceRequesterInfo.

BUG=649558
Committed: https://crrev.com/7aac46c0e9451b8214b21205638697f704cc0180
Cr-Commit-Position: refs/heads/master@{#436876}

[horo, 2016-11-14 09:05:15]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-14 09:05:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-14 10:38:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-14 10:38:28]

Dry run: This issue passed the CQ dry run.

[horo, 2016-11-17 11:26:04]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 11:26:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-11-17 11:48:18]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 11:48:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 12:24:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 12:24:45]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[horo, 2016-11-18 09:21:40]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 09:21:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-18 10:34:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-18 10:34:17]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[horo, 2016-11-29 16:09:38]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 16:09:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 17:48:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 17:48:35]

Dry run: This issue passed the CQ dry run.

[horo, 2016-11-30 05:16:16]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 05:16:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-11-30 06:49:42]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 06:50:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-11-30 07:34:05]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 07:34:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-11-30 07:41:36]

Description was changed from

==========
NavigationPreload with PlzNavigate support using ResourceRequesterInfo

This CL depends on https://codereview.chromium.org/2481093003/.

BUG=649558
==========

to

==========
Introduce a new ResourceRequesterInfo type for NavigationPreload.

Currently we use the ResourceRequesterInfo of the original navigation request in
ServiceWorkerFetchDispatcher to send service worker navigation preload
requests. But when BrowserSideNavigation is enabled, the ResourceRequesterInfo
of the original navigation request is not renderer type, but browser side
navigation type. And we can't send the navigation preload request.

To solve this problem, this CL introduces a new ResourceRequesterInfo type for
service worker navigation preload requests.
ResourceRequesterInfo::CreateForNavigationPreload() creates the new type info
from the original navigation request info.

This CL depends on https://codereview.chromium.org/2481093003/.

BUG=649558
==========

[horo, 2016-11-30 07:45:18]

Patchset #5 (id:80001) has been deleted

[horo, 2016-11-30 07:45:21]

Patchset #4 (id:60001) has been deleted

[horo, 2016-11-30 07:45:23]

Patchset #3 (id:40001) has been deleted

[horo, 2016-11-30 07:45:26]

Patchset #2 (id:20001) has been deleted

[horo, 2016-11-30 07:45:28]

Patchset #1 (id:1) has been deleted

[horo, 2016-11-30 07:59:20]

Description was changed from

==========
Introduce a new ResourceRequesterInfo type for NavigationPreload.

Currently we use the ResourceRequesterInfo of the original navigation request in
ServiceWorkerFetchDispatcher to send service worker navigation preload
requests. But when BrowserSideNavigation is enabled, the ResourceRequesterInfo
of the original navigation request is not renderer type, but browser side
navigation type. And we can't send the navigation preload request.

To solve this problem, this CL introduces a new ResourceRequesterInfo type for
service worker navigation preload requests.
ResourceRequesterInfo::CreateForNavigationPreload() creates the new type info
from the original navigation request info.

This CL depends on https://codereview.chromium.org/2481093003/.

BUG=649558
==========

to

==========
Introduce a new ResourceRequesterInfo type for NavigationPreload.

Currently we use the ResourceRequesterInfo of the original navigation request in
ServiceWorkerFetchDispatcher to send service worker navigation preload
requests. But when BrowserSideNavigation is enabled, the ResourceRequesterInfo
of the original navigation request is not renderer type, but browser side
navigation type. And we can't send the navigation preload request.

To solve this problem, this CL introduces a new ResourceRequesterInfo type for
service worker navigation preload requests.
ResourceRequesterInfo::CreateForNavigationPreload() creates the new type info
from the original navigation request info.

This CL depends on https://codereview.chromium.org/2481093003/ which
introduced ResourceRequesterInfo.

BUG=649558
==========

[horo, 2016-11-30 07:59:33]

Description was changed from

==========
Introduce a new ResourceRequesterInfo type for NavigationPreload.

Currently we use the ResourceRequesterInfo of the original navigation request in
ServiceWorkerFetchDispatcher to send service worker navigation preload
requests. But when BrowserSideNavigation is enabled, the ResourceRequesterInfo
of the original navigation request is not renderer type, but browser side
navigation type. And we can't send the navigation preload request.

To solve this problem, this CL introduces a new ResourceRequesterInfo type for
service worker navigation preload requests.
ResourceRequesterInfo::CreateForNavigationPreload() creates the new type info
from the original navigation request info.

This CL depends on https://codereview.chromium.org/2481093003/ which
introduced ResourceRequesterInfo.

BUG=649558
==========

to

==========
Introduce a new ResourceRequesterInfo type for NavigationPreload.

Currently we use the ResourceRequesterInfo of the original navigation request in
ServiceWorkerFetchDispatcher to send service worker navigation preload
requests. But when BrowserSideNavigation is enabled, the ResourceRequesterInfo
of the original navigation request is not renderer type, but browser side
navigation type. And we can't send the navigation preload request.

To solve this problem, this CL introduces a new ResourceRequesterInfo type for
service worker navigation preload requests.
ResourceRequesterInfo::CreateForNavigationPreload() creates the new type info
from the original navigation request info.

This CL depends on https://codereview.chromium.org/2481093003/ which introduced
ResourceRequesterInfo.

BUG=649558
==========

[horo, 2016-11-30 08:17:06]

Patchset #2 (id:120001) has been deleted

[horo, 2016-11-30 08:17:10]

Patchset #1 (id:100001) has been deleted

[horo, 2016-11-30 08:17:54]

Description was changed from

==========
Introduce a new ResourceRequesterInfo type for NavigationPreload.

Currently we use the ResourceRequesterInfo of the original navigation request in
ServiceWorkerFetchDispatcher to send service worker navigation preload
requests. But when BrowserSideNavigation is enabled, the ResourceRequesterInfo
of the original navigation request is not renderer type, but browser side
navigation type. And we can't send the navigation preload request.

To solve this problem, this CL introduces a new ResourceRequesterInfo type for
service worker navigation preload requests.
ResourceRequesterInfo::CreateForNavigationPreload() creates the new type info
from the original navigation request info.

This CL depends on https://codereview.chromium.org/2481093003/ which introduced
ResourceRequesterInfo.

BUG=649558
==========

to

==========
Introduce a new ResourceRequesterInfo type for NavigationPreload.

Currently we use the ResourceRequesterInfo of the original navigation request in
ServiceWorkerFetchDispatcher to send service worker navigation preload
requests. But when BrowserSideNavigation (PlzNavigate) is enabled, the
ResourceRequesterInfo of the original navigation request is not renderer type,
but browser side navigation type. And we can't send the navigation preload
request.

To solve this problem, this CL introduces a new ResourceRequesterInfo type for
service worker navigation preload requests.
ResourceRequesterInfo::CreateForNavigationPreload() creates the new type info
from the original navigation request info.

This CL depends on https://codereview.chromium.org/2481093003/ which introduced
ResourceRequesterInfo.

BUG=649558
==========

[horo, 2016-11-30 08:19:08]

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

[horo, 2016-11-30 08:19:09]

yhirano@
Could you please review this?

[commit-bot: I haz the power, 2016-11-30 08:58:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 08:58:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[horo, 2016-11-30 09:50:28]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 09:50:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 11:00:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 11:00:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[horo, 2016-11-30 12:45:31]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 12:45:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 14:06:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 14:06:36]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-12-01 09:40:02]

https://codereview.chromium.org/2497223002/diff/140001/content/browser/loader...
File content/browser/loader/resource_requester_info.h (right):

https://codereview.chromium.org/2497223002/diff/140001/content/browser/loader...
content/browser/loader/resource_requester_info.h:124: // navigation type
requester.
This comment should be fixed.

https://codereview.chromium.org/2497223002/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2497223002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:384:
DCHECK((IsBrowserSideNavigationEnabled() &&
I would prefer an explicit if-statement.

If (IsBrowserSideNavigationEnabled()) {
  DCHECK(requester_info->IsBrowserSideNavigation());
} else {
  DCHECK(requester_info->IsRenderer());
  if (!requester_info->filter())
    return;
}

[horo, 2016-12-01 10:36:54]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 10:37:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-12-01 10:37:35]

https://codereview.chromium.org/2497223002/diff/140001/content/browser/loader...
File content/browser/loader/resource_requester_info.h (right):

https://codereview.chromium.org/2497223002/diff/140001/content/browser/loader...
content/browser/loader/resource_requester_info.h:124: // navigation type
requester.
On 2016/12/01 09:40:02, yhirano wrote:
> This comment should be fixed.

Done.

https://codereview.chromium.org/2497223002/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2497223002/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:384:
DCHECK((IsBrowserSideNavigationEnabled() &&
On 2016/12/01 09:40:02, yhirano wrote:
> I would prefer an explicit if-statement.
> 
> If (IsBrowserSideNavigationEnabled()) {
>   DCHECK(requester_info->IsBrowserSideNavigation());
> } else {
>   DCHECK(requester_info->IsRenderer());
>   if (!requester_info->filter())
>     return;
> }

Done.

[yhirano, 2016-12-01 10:40:39]

lgtm

https://codereview.chromium.org/2497223002/diff/160001/content/browser/loader...
File content/browser/loader/resource_requester_info.h (right):

https://codereview.chromium.org/2497223002/diff/160001/content/browser/loader...
content/browser/loader/resource_requester_info.h:123: // this method is
available for renderer type requester and browser side
nit: A, B and C instead of A and B and C

[horo, 2016-12-01 10:43:54]

Thank you!

https://codereview.chromium.org/2497223002/diff/160001/content/browser/loader...
File content/browser/loader/resource_requester_info.h (right):

https://codereview.chromium.org/2497223002/diff/160001/content/browser/loader...
content/browser/loader/resource_requester_info.h:123: // this method is
available for renderer type requester and browser side
On 2016/12/01 10:40:39, yhirano wrote:
> nit: A, B and C instead of A and B and C

Done.

[horo, 2016-12-01 10:43:56]

Thank you!

[horo, 2016-12-01 10:45:33]

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

[horo, 2016-12-01 10:45:34]

mmenke@
Could you please review content/browser/loader/*?

[horo, 2016-12-01 12:11:10]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 12:11:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 13:54:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 13:54:38]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-12-01 15:26:11]

https://codereview.chromium.org/2497223002/diff/200001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2497223002/diff/200001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1281:
DCHECK(requester_info->IsRenderer());
Think this is worth an explanation (Preloads have child_id's of -1 and
monotonically increasing request IDs allocated by MakeRequestID).

https://codereview.chromium.org/2497223002/diff/200001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1293:
DCHECK(requester_info->IsRenderer());
Explanation?  This isn't obvious.

https://codereview.chromium.org/2497223002/diff/200001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1386:
DCHECK(requester_info->IsRenderer());
I don't think this is right?  We call out to "registered interceptor for the
headers passed in", and let them choose to cancel a request.  I don't see why
they could cancel non-renderer requests.

https://codereview.chromium.org/2497223002/diff/200001/content/browser/loader...
File content/browser/loader/resource_requester_info.cc (right):

https://codereview.chromium.org/2497223002/diff/200001/content/browser/loader...
content/browser/loader/resource_requester_info.cc:28:
service_worker_context->resource_context()->GetRequestContext();
resource_context_out->GetRequestContext()?

[mmenke, 2016-12-01 15:26:52]

https://codereview.chromium.org/2497223002/diff/200001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2497223002/diff/200001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1386:
DCHECK(requester_info->IsRenderer());
On 2016/12/01 15:26:11, mmenke wrote:
> I don't think this is right?  We call out to "registered interceptor for the
> headers passed in", and let them choose to cancel a request.  I don't see why
> they could cancel non-renderer requests.

Why they couldn't, rather

[horo, 2016-12-02 03:39:32]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 03:39:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-12-02 03:41:53]

https://codereview.chromium.org/2497223002/diff/200001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2497223002/diff/200001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1281:
DCHECK(requester_info->IsRenderer());
On 2016/12/01 15:26:11, mmenke wrote:
> Think this is worth an explanation (Preloads have child_id's of -1 and
> monotonically increasing request IDs allocated by MakeRequestID).

Done.

https://codereview.chromium.org/2497223002/diff/200001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1293:
DCHECK(requester_info->IsRenderer());
On 2016/12/01 15:26:11, mmenke wrote:
> Explanation?  This isn't obvious.

Done.

https://codereview.chromium.org/2497223002/diff/200001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1386:
DCHECK(requester_info->IsRenderer());
On 2016/12/01 15:26:51, mmenke wrote:
> On 2016/12/01 15:26:11, mmenke wrote:
> > I don't think this is right?  We call out to "registered interceptor for the
> > headers passed in", and let them choose to cancel a request.  I don't see
why
> > they could cancel non-renderer requests.
> 
> Why they couldn't, rather

Ah, yes. This could be a navigation preload request.
Changed to check "if (requester_info->IsRenderer())" and added tests.

https://codereview.chromium.org/2497223002/diff/200001/content/browser/loader...
File content/browser/loader/resource_requester_info.cc (right):

https://codereview.chromium.org/2497223002/diff/200001/content/browser/loader...
content/browser/loader/resource_requester_info.cc:28:
service_worker_context->resource_context()->GetRequestContext();
On 2016/12/01 15:26:11, mmenke wrote:
> resource_context_out->GetRequestContext()?

Done.

[commit-bot: I haz the power, 2016-12-02 06:28:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 06:28:22]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[horo, 2016-12-02 08:04:55]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 08:05:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 10:38:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 10:38:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mmenke, 2016-12-02 15:11:46]

LGTM.

https://codereview.chromium.org/2497223002/diff/220001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2497223002/diff/220001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1391: if
(requester_info->IsRenderer()) {
Should we be killing wherever this request did come from?  Also, should we have
a test for this case?

[horo, 2016-12-02 15:29:18]

Thank you.

https://codereview.chromium.org/2497223002/diff/220001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2497223002/diff/220001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1391: if
(requester_info->IsRenderer()) {
On 2016/12/02 15:11:45, mmenke wrote:
> Should we be killing wherever this request did come from?  Also, should we
have
> a test for this case?

I think the purpose of this code is to protect the browser process from
exploited renderer processes.
Navigation preload requests are initiated by the browser process, so we don't
need to kill the renderer process.

SecurityExploitBrowserTest.InvalidOriginHeaders is testing this renderer process
kill.
https://chromium.googlesource.com/chromium/src/+blame/master/content/browser/...

[horo, 2016-12-02 15:29:25]

The CQ bit was checked by horo@chromium.org

[horo, 2016-12-02 15:29:25]

The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2497223002/#ps220001
(title: "incorporated mmenke's comment")

[commit-bot: I haz the power, 2016-12-02 15:29:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2016-12-02 15:31:16]

On 2016/12/02 15:29:18, horo wrote:
> Thank you.
> 
>
https://codereview.chromium.org/2497223002/diff/220001/content/browser/loader...
> File content/browser/loader/resource_dispatcher_host_impl.cc (right):
> 
>
https://codereview.chromium.org/2497223002/diff/220001/content/browser/loader...
> content/browser/loader/resource_dispatcher_host_impl.cc:1391: if
> (requester_info->IsRenderer()) {
> On 2016/12/02 15:11:45, mmenke wrote:
> > Should we be killing wherever this request did come from?  Also, should we
> have
> > a test for this case?
> 
> I think the purpose of this code is to protect the browser process from
> exploited renderer processes.
> Navigation preload requests are initiated by the browser process, so we don't
> need to kill the renderer process.
> 
> SecurityExploitBrowserTest.InvalidOriginHeaders is testing this renderer
process
> kill.
>
https://chromium.googlesource.com/chromium/src/+blame/master/content/browser/...

Hrm... I had assumed these were coming from a ServiceWorker, given they they're
coming from ServiceWorkerFetchDispatcher.  What does
ServiceWorkerFetchDispatcher actually do, then?

[commit-bot: I haz the power, 2016-12-02 15:36:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 15:36:40]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[horo, 2016-12-02 16:21:33]

On 2016/12/02 15:31:16, mmenke wrote:
> On 2016/12/02 15:29:18, horo wrote:
> > Thank you.
> > 
> >
>
https://codereview.chromium.org/2497223002/diff/220001/content/browser/loader...
> > File content/browser/loader/resource_dispatcher_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2497223002/diff/220001/content/browser/loader...
> > content/browser/loader/resource_dispatcher_host_impl.cc:1391: if
> > (requester_info->IsRenderer()) {
> > On 2016/12/02 15:11:45, mmenke wrote:
> > > Should we be killing wherever this request did come from?  Also, should we
> > have
> > > a test for this case?
> > 
> > I think the purpose of this code is to protect the browser process from
> > exploited renderer processes.
> > Navigation preload requests are initiated by the browser process, so we
don't
> > need to kill the renderer process.
> > 
> > SecurityExploitBrowserTest.InvalidOriginHeaders is testing this renderer
> process
> > kill.
> >
>
https://chromium.googlesource.com/chromium/src/+blame/master/content/browser/...
> 
> Hrm... I had assumed these were coming from a ServiceWorker, given they
they're
> coming from ServiceWorkerFetchDispatcher.  What does
> ServiceWorkerFetchDispatcher actually do, then?

ServiceWorkerFetchDispatcher sends FetchEvents to the service worker for the all
resource requests (navigation requests + sub resource requests) from the page
which is controlled by the service worker.
If the navigation preload feature was enabled, ServiceWorkerFetchDispatcher
sends the navigation preload requests to the server before sending the
FetchEvents to the service worker for navigation requests.
And also if PlzNavigate is enabled, the original navigation requests are
initiated by the browser process.

[horo, 2016-12-02 16:26:40]

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

[horo, 2016-12-02 16:26:40]

avi@
Could you please review this CL?
This CL added "content/public/browser/resource_context.h" and
"content/public/common/browser_side_navigation_policy.h" to
content/browser/loader/DEPS.

The presubmit error says:
> ** Presubmit ERRORS **
> You need LGTM from owners of depends-on paths in DEPS that were modified in
this CL:
>     '+content/public/browser/resource_context.h',
>     '+content/public/common/browser_side_navigation_policy.h',

[horo, 2016-12-05 23:58:41]

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

[horo, 2016-12-05 23:58:42]

Ah, avi@ is ooo.

kinuko@
Could you please review this CL?

On 2016/12/02 16:26:40, horo wrote:
> avi@
> Could you please review this CL?
> This CL added "content/public/browser/resource_context.h" and
> "content/public/common/browser_side_navigation_policy.h" to
> content/browser/loader/DEPS.
> 
> The presubmit error says:
> > ** Presubmit ERRORS **
> > You need LGTM from owners of depends-on paths in DEPS that were modified in
> this CL:
> >     '+content/public/browser/resource_context.h',
> >     '+content/public/common/browser_side_navigation_policy.h',

[kinuko, 2016-12-06 03:34:29]

lgtm

[horo, 2016-12-06 03:44:05]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2016-12-06 03:44:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-06 03:52:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 03:52:40]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[horo, 2016-12-06 03:57:37]

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

[horo, 2016-12-06 03:57:38]

clamy@
Could you please review this?

> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for these files:
>   
testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter

[clamy, 2016-12-06 12:15:28]

Thanks! Lgtm

[horo, 2016-12-06 12:57:25]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2016-12-06 12:57:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-06 16:10:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 16:10:16]

Failed to apply patch for
testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter:
While running git apply --index -p1;
  error: patch failed:
testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter:3
  error:
testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter:
patch does not apply

Patch:      
testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter
Index:
testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter
diff --git
a/testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter
b/testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter
index
1af38ca413d3d58c9ebfc9af775467a8704a35ae..6fcbceac2af786ece86d2ad8ad02ff7f054cbca0
100644
---
a/testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter
+++
b/testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter
@@ -3,9 +3,6 @@
 -RenderFrameHostManagerTest.RestoreSubframeFileAccessForHistoryNavigation
 -RequestDataResourceDispatcherHostBrowserTest.*
 
-# NavigationPreload dosn't work well with PlzNavigate yet.
https://crbug.com/649558
--ServiceWorkerBrowserTest/ServiceWorkerNavigationPreloadTest.*
-
 # Browser-initiated fragment navigations are handled improperly.
https://crbug.com/663777
 -NavigationControllerBrowserTest.SamePageBrowserInitiated

[horo, 2016-12-06 16:21:00]

The CQ bit was checked by horo@chromium.org

[horo, 2016-12-06 16:21:01]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
yhirano@chromium.org, clamy@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2497223002/#ps240001
(title: "rebase")

[commit-bot: I haz the power, 2016-12-06 16:21:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-06 18:08:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 18:08:10]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[horo, 2016-12-06 22:36:12]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2016-12-06 22:37:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 02:21:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 02:21:43]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[horo, 2016-12-07 03:01:35]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2016-12-07 03:02:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 05:55:12]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1481079695131500,
"parent_rev": "39d22e7fa436966cf7ba61c30b3ad0c8694ffee6", "commit_rev":
"3656764b846866ee79c167905f46438b7e2dab1d"}

[commit-bot: I haz the power, 2016-12-07 05:55:38]

Description was changed from

==========
Introduce a new ResourceRequesterInfo type for NavigationPreload.

Currently we use the ResourceRequesterInfo of the original navigation request in
ServiceWorkerFetchDispatcher to send service worker navigation preload
requests. But when BrowserSideNavigation (PlzNavigate) is enabled, the
ResourceRequesterInfo of the original navigation request is not renderer type,
but browser side navigation type. And we can't send the navigation preload
request.

To solve this problem, this CL introduces a new ResourceRequesterInfo type for
service worker navigation preload requests.
ResourceRequesterInfo::CreateForNavigationPreload() creates the new type info
from the original navigation request info.

This CL depends on https://codereview.chromium.org/2481093003/ which introduced
ResourceRequesterInfo.

BUG=649558
==========

to

==========
Introduce a new ResourceRequesterInfo type for NavigationPreload.

Currently we use the ResourceRequesterInfo of the original navigation request in
ServiceWorkerFetchDispatcher to send service worker navigation preload
requests. But when BrowserSideNavigation (PlzNavigate) is enabled, the
ResourceRequesterInfo of the original navigation request is not renderer type,
but browser side navigation type. And we can't send the navigation preload
request.

To solve this problem, this CL introduces a new ResourceRequesterInfo type for
service worker navigation preload requests.
ResourceRequesterInfo::CreateForNavigationPreload() creates the new type info
from the original navigation request info.

This CL depends on https://codereview.chromium.org/2481093003/ which introduced
ResourceRequesterInfo.

BUG=649558
==========

[commit-bot: I haz the power, 2016-12-07 05:55:41]

Committed patchset #6 (id:240001)

[commit-bot: I haz the power, 2016-12-07 05:57:57]

Description was changed from

==========
Introduce a new ResourceRequesterInfo type for NavigationPreload.

Currently we use the ResourceRequesterInfo of the original navigation request in
ServiceWorkerFetchDispatcher to send service worker navigation preload
requests. But when BrowserSideNavigation (PlzNavigate) is enabled, the
ResourceRequesterInfo of the original navigation request is not renderer type,
but browser side navigation type. And we can't send the navigation preload
request.

To solve this problem, this CL introduces a new ResourceRequesterInfo type for
service worker navigation preload requests.
ResourceRequesterInfo::CreateForNavigationPreload() creates the new type info
from the original navigation request info.

This CL depends on https://codereview.chromium.org/2481093003/ which introduced
ResourceRequesterInfo.

BUG=649558
==========

to

==========
Introduce a new ResourceRequesterInfo type for NavigationPreload.

Currently we use the ResourceRequesterInfo of the original navigation request in
ServiceWorkerFetchDispatcher to send service worker navigation preload
requests. But when BrowserSideNavigation (PlzNavigate) is enabled, the
ResourceRequesterInfo of the original navigation request is not renderer type,
but browser side navigation type. And we can't send the navigation preload
request.

To solve this problem, this CL introduces a new ResourceRequesterInfo type for
service worker navigation preload requests.
ResourceRequesterInfo::CreateForNavigationPreload() creates the new type info
from the original navigation request info.

This CL depends on https://codereview.chromium.org/2481093003/ which introduced
ResourceRequesterInfo.

BUG=649558

Committed: https://crrev.com/7aac46c0e9451b8214b21205638697f704cc0180
Cr-Commit-Position: refs/heads/master@{#436876}
==========

[commit-bot: I haz the power, 2016-12-07 05:57:59]

Patchset 6 (id:??) landed as
https://crrev.com/7aac46c0e9451b8214b21205638697f704cc0180
Cr-Commit-Position: refs/heads/master@{#436876}