Support Service Worker NavigationPreload with PlzNavigate.

Support Service Worker NavigationPreload with PlzNavigate.

When ServiceWorkerFetchDispatcher sends the navigation preload request, it gets
a ResourceMessageFilter from RsourceRequestInfoImpl::filter() to create a
URLLoaderFactoryImpl.

But if PlzNavigate is enabled, RsourceRequestInfoImpl::filter returns null.
So ServiceWorkerFetchDispatcher can't send the navigation preload request.

To solve this problem, this CL do the following:
- NavigationRequest::OnStartChecksComplete() gets the ResourceMessageFilter of 
  the navigating_frame_host's process and pass it to
  ServiceWorkerNavigationHandleCore on the UI thread.
- On the IO thread, ServiceWorkerNavigationHandleCore::DidPreCreateProviderHost()
  pass the ResourceMessageFilter to the ServiceWorkerProviderHost.
- So ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload() can get the
  filter from the ServiceWorkerProviderHost and create a URLLoaderFactoryImpl.

BUG=649558
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

[horo, 2016-10-31 11:58:23]

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

[commit-bot: I haz the power, 2016-10-31 11:58:37]

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-10-31 12:12:35]

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

[commit-bot: I haz the power, 2016-10-31 12:12:36]

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

[horo, 2016-10-31 14:06:55]

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

[commit-bot: I haz the power, 2016-10-31 14:07: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-10-31 14:51:49]

Description was changed from

==========
use weakptr

BUG=
==========

to

==========
use weakptr

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[horo, 2016-10-31 14:51:57]

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

[commit-bot: I haz the power, 2016-10-31 14:52:14]

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-10-31 17:03:15]

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

[commit-bot: I haz the power, 2016-10-31 17:03:16]

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

[horo, 2016-11-01 03:23:24]

Patchset #4 (id:60001) has been deleted

[horo, 2016-11-01 03:23:31]

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

[commit-bot: I haz the power, 2016-11-01 03:23:43]

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-01 04:24:04]

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

[commit-bot: I haz the power, 2016-11-01 04:24:05]

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

[horo, 2016-11-01 06:15:41]

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

[commit-bot: I haz the power, 2016-11-01 06:15:51]

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-01 06:22:58]

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

[commit-bot: I haz the power, 2016-11-01 06:23:11]

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-01 06:35:33]

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

[commit-bot: I haz the power, 2016-11-01 06:35:34]

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

[horo, 2016-11-01 07:11:48]

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

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

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-01 08:19:33]

Patchset #6 (id:120001) has been deleted

[horo, 2016-11-01 08:19:35]

Patchset #5 (id:100001) has been deleted

[horo, 2016-11-01 08:19:39]

Patchset #4 (id:80001) has been deleted

[horo, 2016-11-01 08:19:42]

Patchset #3 (id:40001) has been deleted

[horo, 2016-11-01 08:19:46]

Patchset #2 (id:20001) has been deleted

[horo, 2016-11-01 08:19:50]

Patchset #1 (id:1) has been deleted

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

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

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

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

[horo, 2016-11-01 08:44:08]

Description was changed from

==========
use weakptr

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Support Service Worker NavigationPreload with PlzNavigate.

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

When ServiceWorkerFetchDispatcher sends the navigation preload request, it gets
a ResourceMessageFilter from RsourceRequestInfoImpl::filter() to create a
URLLoaderFactoryImpl.

But if PlzNavigate is enabled, RsourceRequestInfoImpl::filter returns null.
So ServiceWorkerFetchDispatcher can't send the navigation preload request.

To solve this problem, this CL do the following:
- NavigationRequest::OnStartChecksComplete() gets the ResourceMessageFilter of 
  the navigating_frame_host's process and pass it to
  ServiceWorkerNavigationHandleCore on the UI thread.
- ResourceDispatcherHostImpl::BeginNavigationRequest() gets the weak pointer of
  the filter from the ServiceWorkerNavigationHandleCore on the IO thread and
  sets to RsourceRequestInfoImpl's filter_for_navigation_preload.
- So ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload() can get the
  filter from the RsourceRequestInfoImpl and create a URLLoaderFactoryImpl.

BUG=649558
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[horo, 2016-11-01 08:45:26]

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

[horo, 2016-11-01 08:45:27]

falken@
Please review this CL.

[falken, 2016-11-02 05:35:59]

(as talked offline horo@ is currently revising this CL so let me know when to
take a look)

[horo, 2016-11-02 07:57:39]

Description was changed from

==========
Support Service Worker NavigationPreload with PlzNavigate.

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

When ServiceWorkerFetchDispatcher sends the navigation preload request, it gets
a ResourceMessageFilter from RsourceRequestInfoImpl::filter() to create a
URLLoaderFactoryImpl.

But if PlzNavigate is enabled, RsourceRequestInfoImpl::filter returns null.
So ServiceWorkerFetchDispatcher can't send the navigation preload request.

To solve this problem, this CL do the following:
- NavigationRequest::OnStartChecksComplete() gets the ResourceMessageFilter of 
  the navigating_frame_host's process and pass it to
  ServiceWorkerNavigationHandleCore on the UI thread.
- ResourceDispatcherHostImpl::BeginNavigationRequest() gets the weak pointer of
  the filter from the ServiceWorkerNavigationHandleCore on the IO thread and
  sets to RsourceRequestInfoImpl's filter_for_navigation_preload.
- So ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload() can get the
  filter from the RsourceRequestInfoImpl and create a URLLoaderFactoryImpl.

BUG=649558
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Support Service Worker NavigationPreload with PlzNavigate.

When ServiceWorkerFetchDispatcher sends the navigation preload request, it gets
a ResourceMessageFilter from RsourceRequestInfoImpl::filter() to create a
URLLoaderFactoryImpl.

But if PlzNavigate is enabled, RsourceRequestInfoImpl::filter returns null.
So ServiceWorkerFetchDispatcher can't send the navigation preload request.

To solve this problem, this CL do the following:
- NavigationRequest::OnStartChecksComplete() gets the ResourceMessageFilter of 
  the navigating_frame_host's process and pass it to
  ServiceWorkerNavigationHandleCore on the UI thread.
- On IO thread, ServiceWorkerNavigationHandleCore::DidPreCreateProviderHost()
  pass the ResourceMessageFilter to the ServiceWorkerProviderHost.
- So ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload() can get the
  filter from the ServiceWorkerProviderHost and create a URLLoaderFactoryImpl.

BUG=649558
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[horo, 2016-11-02 07:57:51]

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

[commit-bot: I haz the power, 2016-11-02 07:58: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-11-02 07:59:36]

Patchset #2 (id:160001) has been deleted

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

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[horo, 2016-11-02 08:01:08]

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

[horo, 2016-11-02 08:01:36]

On 2016/11/02 05:35:59, falken wrote:
> (as talked offline horo@ is currently revising this CL so let me know when to
> take a look)

done.
Please review Patch Set 2 :)

[commit-bot: I haz the power, 2016-11-02 08:01: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-02 08:04:41]

Description was changed from

==========
Support Service Worker NavigationPreload with PlzNavigate.

When ServiceWorkerFetchDispatcher sends the navigation preload request, it gets
a ResourceMessageFilter from RsourceRequestInfoImpl::filter() to create a
URLLoaderFactoryImpl.

But if PlzNavigate is enabled, RsourceRequestInfoImpl::filter returns null.
So ServiceWorkerFetchDispatcher can't send the navigation preload request.

To solve this problem, this CL do the following:
- NavigationRequest::OnStartChecksComplete() gets the ResourceMessageFilter of 
  the navigating_frame_host's process and pass it to
  ServiceWorkerNavigationHandleCore on the UI thread.
- On IO thread, ServiceWorkerNavigationHandleCore::DidPreCreateProviderHost()
  pass the ResourceMessageFilter to the ServiceWorkerProviderHost.
- So ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload() can get the
  filter from the ServiceWorkerProviderHost and create a URLLoaderFactoryImpl.

BUG=649558
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Support Service Worker NavigationPreload with PlzNavigate.

When ServiceWorkerFetchDispatcher sends the navigation preload request, it gets
a ResourceMessageFilter from RsourceRequestInfoImpl::filter() to create a
URLLoaderFactoryImpl.

But if PlzNavigate is enabled, RsourceRequestInfoImpl::filter returns null.
So ServiceWorkerFetchDispatcher can't send the navigation preload request.

To solve this problem, this CL do the following:
- NavigationRequest::OnStartChecksComplete() gets the ResourceMessageFilter of 
  the navigating_frame_host's process and pass it to
  ServiceWorkerNavigationHandleCore on the UI thread.
- On the IO thread,
ServiceWorkerNavigationHandleCore::DidPreCreateProviderHost()
  pass the ResourceMessageFilter to the ServiceWorkerProviderHost.
- So ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload() can get the
  filter from the ServiceWorkerProviderHost and create a URLLoaderFactoryImpl.

BUG=649558
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[falken, 2016-11-02 08:16:29]

this looks good, i just didn't have time to read the tests yet. may be good to
start adding other OWNERs for content/ changes since tomorrow is a holiday.

[commit-bot: I haz the power, 2016-11-02 08:57:37]

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

[commit-bot: I haz the power, 2016-11-02 08:57:39]

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

[horo, 2016-11-02 09:42:27]

horo@chromium.org changed reviewers:
+ alexmos@chromium.org, piman@chromium.org

[horo, 2016-11-02 09:42:28]

piman@
Please review:
- content/browser/renderer_host/render_process_host_impl.*
- content/public/browser/render_process_host.h
- content/public/test/mock_render_process_host.*?

alexmos@
Please review:
- content/browser/frame_host/navigation_handle_impl.*
- content/browser/frame_host/navigation_request.cc

[falken, 2016-11-02 14:15:29]

lgtm. I gave some suggested comments, of course feel free to edit to your
liking.

Actually I'm curious why the browser test was added... I'd think this patch to
support PlzNavigate only needs to fix the existing tests. Did this test exercise
something added in this patch?

https://codereview.chromium.org/2460223003/diff/180001/content/browser/servic...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/2460223003/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_browsertest.cc:1952: 
This seems sufficiently complex that some comments would be useful.

// "Tests responding with the navigation preload response when the navigation
occurred after a redirect."

https://codereview.chromium.org/2460223003/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_browsertest.cc:1972: const char
kInScopeRedirectPageUrl[] = "/service_worker/redirect";
// Register redirects to the target URL. The service worker responds to the
target URL with the navigation preload response.

https://codereview.chromium.org/2460223003/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_browsertest.cc:1986: 
// Navigate to a same-origin, out of scope URL that redirects to the target URL.
The navigation preload request should be the single request to the target URL.

https://codereview.chromium.org/2460223003/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_browsertest.cc:1991: 
// Navigate to a same-origin, in-scope URL that redirects to the target URL. The
navigation preload request should be the single request to the target URL.

https://codereview.chromium.org/2460223003/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_browsertest.cc:1996: 
// Navigate to a cross-origin URL that redirects to the target URL. The
navigation preload request should be the single request to the target URL.

[piman, 2016-11-02 18:33:52]

lgtm

[nasko, 2016-11-02 18:48:53]

nasko@chromium.org changed reviewers:
+ clamy@chromium.org, nasko@chromium.org

[nasko, 2016-11-02 18:48:53]

nasko@chromium.org changed required reviewers:
+ clamy@chromium.org

[nasko, 2016-11-02 18:48:54]

Adding clamy@ as a required reviewer, as she has done the main PlzNavigate and
SW integration work.

[nasko, 2016-11-02 18:49:06]

nasko@chromium.org changed reviewers:
- nasko@chromium.org

[alexmos, 2016-11-03 00:55:59]

On 2016/11/02 09:42:28, horo (ooo Nov 3) wrote:
> piman@
> Please review:
> - content/browser/renderer_host/render_process_host_impl.*
> - content/public/browser/render_process_host.h
> - content/public/test/mock_render_process_host.*?
> 
> alexmos@
> Please review:
> - content/browser/frame_host/navigation_handle_impl.*
> - content/browser/frame_host/navigation_request.cc

frame_host looks good, but I'll defer the review to clamy@, since, as Nasko
says, she has the most context into this work.

[horo, 2016-11-03 02:13:43]

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

[commit-bot: I haz the power, 2016-11-03 02:14:12]

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-03 02:30:43]

> Actually I'm curious why the browser test was added... I'd think this patch to
> support PlzNavigate only needs to fix the existing tests. Did this test
> exercise
> something added in this patch?

In the in-scope redirect case, ServiceWorkerProviderHost's
resource_filter_for_navigation_preload() is called twice. So we can't clear
the pointer in ServiceWorkerProviderHost. I want to exercise this.

https://codereview.chromium.org/2460223003/diff/180001/content/browser/servic...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/2460223003/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_browsertest.cc:1952: 
On 2016/11/02 14:15:29, falken (ooo Nov 3) wrote:
> This seems sufficiently complex that some comments would be useful.
> 
> // "Tests responding with the navigation preload response when the navigation
> occurred after a redirect."

Done.

https://codereview.chromium.org/2460223003/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_browsertest.cc:1972: const char
kInScopeRedirectPageUrl[] = "/service_worker/redirect";
On 2016/11/02 14:15:29, falken (ooo Nov 3) wrote:
> // Register redirects to the target URL. The service worker responds to the
> target URL with the navigation preload response.

Done.

https://codereview.chromium.org/2460223003/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_browsertest.cc:1986: 
On 2016/11/02 14:15:28, falken (ooo Nov 3) wrote:
> // Navigate to a same-origin, out of scope URL that redirects to the target
URL.
> The navigation preload request should be the single request to the target URL.

Done.

https://codereview.chromium.org/2460223003/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_browsertest.cc:1991: 
On 2016/11/02 14:15:28, falken (ooo Nov 3) wrote:
> // Navigate to a same-origin, in-scope URL that redirects to the target URL.
The
> navigation preload request should be the single request to the target URL.

Done.

https://codereview.chromium.org/2460223003/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_browsertest.cc:1996: 
On 2016/11/02 14:15:29, falken (ooo Nov 3) wrote:
> // Navigate to a cross-origin URL that redirects to the target URL. The
> navigation preload request should be the single request to the target URL.

Done.

[commit-bot: I haz the power, 2016-11-03 04:09:21]

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

[commit-bot: I haz the power, 2016-11-03 04:09:22]

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

[horo, 2016-11-04 09:14:01]

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

[commit-bot: I haz the power, 2016-11-04 09:14:20]

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-04 09:16:20]

I changed my mind.
We should have RedirectAndRespondWithNavigationPreload test even without this
change.
So I splitted the test to https://codereview.chromium.org/2478103002.

[commit-bot: I haz the power, 2016-11-04 10:25:28]

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

[commit-bot: I haz the power, 2016-11-04 10:25:29]

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

[horo, 2016-11-04 11:03:56]

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

[commit-bot: I haz the power, 2016-11-04 11:04:14]

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-04 12:49:09]

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

[commit-bot: I haz the power, 2016-11-04 12:49:10]

Dry run: This issue passed the CQ dry run.

[clamy, 2016-11-04 15:35:27]

Would you have a design doc that describe what navigation preload is supposed to
do? From reading the code, I get the feeling that the way it has been
implemented is very tied to the current navigation architecture and works at
odds with how PlzNavigate is set up.

In particular, I'm quite unhappy about having to rely on the
ResourceMessageFilter of the speculative RFH this early in navigation,
considering that it may well go away in the middle of navigation. Apart from
this particular point, I'm wondering whether all this back and forth with the
renderer is really necessary with PlzNavigate.

I'm not sure IIUC, but if the goal is to preload a navigation, it is very doable
to do it in PlzNavigate without ever sending anything to a renderer. Basically,
if you ask the ResourceDispatcherHost to start a navigation request using
BeginNavigationRequest it will create a browser-side only navigation request.
The request created communicates with whoever created it via the
NavigationURLLoaderImplCore. I think it could be turned into an interface
implemented by the ServiceWorker preload navigationclass. When we determine in
the browser that a preload is necessary, we can just issue the navigation
request in ResourceDispatcherHost and be informed of it's progression through
this interface.

[horo, 2016-11-04 17:25:49]

On 2016/11/04 15:35:27, clamy (slow) wrote:
> Would you have a design doc that describe what navigation preload is supposed
to
> do? From reading the code, I get the feeling that the way it has been
> implemented is very tied to the current navigation architecture and works at
> odds with how PlzNavigate is set up.
> 
> In particular, I'm quite unhappy about having to rely on the
> ResourceMessageFilter of the speculative RFH this early in navigation,
> considering that it may well go away in the middle of navigation. Apart from
> this particular point, I'm wondering whether all this back and forth with the
> renderer is really necessary with PlzNavigate.
> 
> I'm not sure IIUC, but if the goal is to preload a navigation, it is very
doable
> to do it in PlzNavigate without ever sending anything to a renderer.
Basically,
> if you ask the ResourceDispatcherHost to start a navigation request using
> BeginNavigationRequest it will create a browser-side only navigation request.
> The request created communicates with whoever created it via the
> NavigationURLLoaderImplCore. I think it could be turned into an interface
> implemented by the ServiceWorker preload navigationclass. When we determine in
> the browser that a preload is necessary, we can just issue the navigation
> request in ResourceDispatcherHost and be informed of it's progression through
> this interface.

Sorry, I didn't write the design doc yet. (I will write next week...)
This is the intent to implement of Navigation preload.
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/PI3FG54L1ZU/B4...

This feature can be enabled by calling registration.navigationPreload.enable().
> addEventListener('activate', e => {
>   e.waitUntil(registration.navigationPreload.enable());
> });

If this feature is enabled, when a page navigation happens, the browser process
sends a network request ("navigation preload request") to the server before
sending the fetch event to the service worker.
(If the service worker is not started yet, this navigation preload request is
sent before starting the service worker. So we can mitigate the start up cost.)

ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload() sends the navigation
preload request using URLLoaderFactoryImpl.
We can create the response receiver (mojom::URLLoaderClient) in the service
worker after sending the network request by sending
mojom::URLLoaderClientRequest to the service worker. 
In the service worker, we can get the response of the navigation preload request
from FetchEvent.preloadResponse promise.
> addEventListener('fetch', e => {
>   if (e.request.mode == 'navigate')
>     e.respondWith(e.preloadResponse);
> });

When creating a URLLoaderFactoryImpl, we set a ResourceMessageFilter.
> URLLoaderFactoryImpl::Create(filter, mojo::GetProxy(&factory));
But this filter is not used to send IPCs to the renderer process.
This filter is used to cancel the network request when the filter is closed (by
ResourceDispatcherHostImpl::CancelRequestsForProcess()).

In this CL, the ResourceMessageFilter is acquired from the speculative RFH when
PlzNavigate is enabled.
If the process of the speculative RFH can go away in the middle of navigation,
this approach is not sufficient.
Could you please let me know in what situation the process can go away?

[nasko, 2016-11-04 18:38:58]

> In this CL, the ResourceMessageFilter is acquired from the speculative RFH
when
> PlzNavigate is enabled.
> If the process of the speculative RFH can go away in the middle of navigation,
> this approach is not sufficient.
> Could you please let me know in what situation the process can go away?

Please keep in mind that speculative RFH is just an implementation detail and
was the easiest way for us to precreate a process. However, it is a goal to
remove the speculative RFH once PlzNavigate starts shipping. We should *not* be
relying on it.

[horo, 2016-11-07 11:06:43]

On 2016/11/04 18:38:58, nasko wrote:
> > In this CL, the ResourceMessageFilter is acquired from the speculative RFH
> when
> > PlzNavigate is enabled.
> > If the process of the speculative RFH can go away in the middle of
navigation,
> > this approach is not sufficient.
> > Could you please let me know in what situation the process can go away?
> 
> Please keep in mind that speculative RFH is just an implementation detail and
> was the easiest way for us to precreate a process. However, it is a goal to
> remove the speculative RFH once PlzNavigate starts shipping. We should *not*
be
> relying on it.

OK. I understood.
I'll try to make URLLoaderFactoryImpl independent from ResourceMessageFilter,
and make it possible to send network requests without RPH.

[clamy, 2016-11-07 17:41:04]

On 2016/11/07 11:06:43, horo wrote:
> On 2016/11/04 18:38:58, nasko wrote:
> > > In this CL, the ResourceMessageFilter is acquired from the speculative RFH
> > when
> > > PlzNavigate is enabled.
> > > If the process of the speculative RFH can go away in the middle of
> navigation,
> > > this approach is not sufficient.
> > > Could you please let me know in what situation the process can go away?
> > 
> > Please keep in mind that speculative RFH is just an implementation detail
and
> > was the easiest way for us to precreate a process. However, it is a goal to
> > remove the speculative RFH once PlzNavigate starts shipping. We should *not*
> be
> > relying on it.
> 
> OK. I understood.
> I'll try to make URLLoaderFactoryImpl independent from ResourceMessageFilter,
> and make it possible to send network requests without RPH.

I don't think this is going to work, nor does it go in the right direction IMO.
The URLLoader is meant to transfer data between the browser and the renderers,
and is not needed to issue a navigation request from the browser when
PlzNavigate is enabled. Instead, I encourage you to use the interfaces we've
developped for PlzNavigate that work purely browser-side.

In particular:
1) Use ResourceDispatcherHostImpl::BeginNavigationRequest to start a
browser-initiated navigation request.
2) To do that, you'd need to have it take an interface (let's call it
NavigationResourceHandlerDelegate). This interface would be implemented by
NavigationURLloaderImplCore and a
ServiceWorkerNavigationResourceHandlerDelegate.
3) Through this interface, you gain access to the resource response & a stream
where the body is put. You can use it to just transfer data when the
ServiceWorker is ready and has made the request.
4) It also owns the request. So if you find that that the request should be
cancelled, you can just delete the NavigationResourceHandlerDelegate and this
will stop the request.

All of this stays browser-side and seems to me to be easier to implement than
relying on the URLResourceLoader which was not meant for this.

[horo, 2016-11-08 10:33:34]

On 2016/11/07 17:41:04, clamy wrote:
> On 2016/11/07 11:06:43, horo wrote:
> > On 2016/11/04 18:38:58, nasko wrote:
> > > > In this CL, the ResourceMessageFilter is acquired from the speculative
RFH
> > > when
> > > > PlzNavigate is enabled.
> > > > If the process of the speculative RFH can go away in the middle of
> > navigation,
> > > > this approach is not sufficient.
> > > > Could you please let me know in what situation the process can go away?
> > > 
> > > Please keep in mind that speculative RFH is just an implementation detail
> and
> > > was the easiest way for us to precreate a process. However, it is a goal
to
> > > remove the speculative RFH once PlzNavigate starts shipping. We should
*not*
> > be
> > > relying on it.
> > 
> > OK. I understood.
> > I'll try to make URLLoaderFactoryImpl independent from
ResourceMessageFilter,
> > and make it possible to send network requests without RPH.
> 
> I don't think this is going to work, nor does it go in the right direction
IMO.
> The URLLoader is meant to transfer data between the browser and the renderers,
> and is not needed to issue a navigation request from the browser when
> PlzNavigate is enabled. Instead, I encourage you to use the interfaces we've
> developped for PlzNavigate that work purely browser-side.
> 
> In particular:
> 1) Use ResourceDispatcherHostImpl::BeginNavigationRequest to start a
> browser-initiated navigation request.
> 2) To do that, you'd need to have it take an interface (let's call it
> NavigationResourceHandlerDelegate). This interface would be implemented by
> NavigationURLloaderImplCore and a
> ServiceWorkerNavigationResourceHandlerDelegate.
> 3) Through this interface, you gain access to the resource response & a stream
> where the body is put. You can use it to just transfer data when the
> ServiceWorker is ready and has made the request.
> 4) It also owns the request. So if you find that that the request should be
> cancelled, you can just delete the NavigationResourceHandlerDelegate and this
> will stop the request.
> 
> All of this stays browser-side and seems to me to be easier to implement than
> relying on the URLResourceLoader which was not meant for this.

I think we should not use content::Stream to pass the body of Navigation
Preload from the browser process to the service worker both in the performance
and the lifecycle management point of view.

When we read the data from the content::Stream in the service worker, we have
to use DocumentThreadableLoader which requires thread hops from the main thread
and the worker thread. So when the main thread of the renderer is busy (I
believe it is busy while loading the page), the data transfer may be blocked.

And also if we use content::Stream, we have to manage the lifecycle of the
stream in StreamRegistry. We need to release the stream handle once the renderer
has finished or when error occurs. Otherwise it is easy to cause memory leaks.
https://chromium.googlesource.com/chromium/src/+/master/content/browser/frame...


Currently we are using mojom::URLLoader for Navigation Preload.
https://chromium.googlesource.com/chromium/src/+/6e20b74/content/browser/serv...
mojom::URLLoader is using mojo::DataPipe to send the body. So we can read the
data in the service worker even if the main thread is busy.

And We can easily cancel and clear the request by deleting mojom::URLLoaderPtr
in the service worker. When mojom::URLLoaderPtr is deleted,
MojoAsyncResourceHandler which is a subclass of mojom::URLLoader will be deleted
in the browser.
https://chromium.googlesource.com/chromium/src/+/6e20b74/content/renderer/ser...


I'm creating a CL to introduce ResourceRequesterInfo to abstract the requester
of resource request. https://codereview.chromium.org/2481093003/
I think we can send navigation preload requests in this approach even if
PlzNavigate is enabled.

[horo, 2016-12-08 02:19:13]

Let's close this issue.
I landed https://codereview.chromium.org/2497223002/ which enables
NavigationPreload with PlzNavigate.