Merge CrossSiteResourceHandler and NavigationResourceThrottle

Merge CrossSiteResourceHandler and NavigationResourceThrottle

This CL merges CrossSiteResourceHandler and NavigationResourceThrottle.
It also removes the CrossSiteResourceRequest in favor of using
NavigationHandle. This allows WebContentsObserver::ReadyToCommit
navigation to be called in the non-PlzNavigate architecture.

A detailed description of the change can be found at https://docs.google.com/a/chromium.org/document/d/1Ze5_RNvUQV4ltWvFU5MY3L_TRIAux88iLadQIPHTMqk/edit?usp=sharing.

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

Committed: https://crrev.com/f40a0349f8575ccd544d6844dccd56119b87170c
Cr-Commit-Position: refs/heads/master@{#421216}

[clamy, 2016-09-07 13:54:58]

Description was changed from

==========
Merge CrossSiteResourceHandler and NavigationResourceThrottle

This CL merges CrossSiteResourceHandler and NavigationResourceThrottle.
It also removes the CrossSiteResourceRequest in favor of using
NavigationHandle. This allows WebContentsObserver::ReadyToCommit
navigation to be called in the non-PlzNavigate architecture.

BUG=621856
==========

to

==========
Merge CrossSiteResourceHandler and NavigationResourceThrottle

This CL merges CrossSiteResourceHandler and NavigationResourceThrottle.
It also removes the CrossSiteResourceRequest in favor of using
NavigationHandle. This allows WebContentsObserver::ReadyToCommit
navigation to be called in the non-PlzNavigate architecture.

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

[clamy, 2016-09-07 13:55:07]

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

[commit-bot: I haz the power, 2016-09-07 13:55:19]

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

[clamy, 2016-09-07 13:55:47]

Description was changed from

==========
Merge CrossSiteResourceHandler and NavigationResourceThrottle

This CL merges CrossSiteResourceHandler and NavigationResourceThrottle.
It also removes the CrossSiteResourceRequest in favor of using
NavigationHandle. This allows WebContentsObserver::ReadyToCommit
navigation to be called in the non-PlzNavigate architecture.

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

to

==========
Merge CrossSiteResourceHandler and NavigationResourceThrottle

This CL merges CrossSiteResourceHandler and NavigationResourceThrottle.
It also removes the CrossSiteResourceRequest in favor of using
NavigationHandle. This allows WebContentsObserver::ReadyToCommit
navigation to be called in the non-PlzNavigate architecture.

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

[clamy, 2016-09-07 13:55:47]

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

[clamy, 2016-09-07 13:57:36]

@nasko & creis: PTAL
This is the merge of CrossSiteResourceHandler & NavigationResourceThrottle that
allows to call ReadyToCommitNavigation in the current architecture. Note that I
also got rid of CrossSiteResourceRequest, since it turned out that we had a
NavigationHandle accessible in the same place with the same info.

[commit-bot: I haz the power, 2016-09-07 14:18:39]

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

[commit-bot: I haz the power, 2016-09-07 14:18:39]

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

[clamy, 2016-09-07 14:43:22]

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

[commit-bot: I haz the power, 2016-09-07 14:43:39]

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-09-07 15:38:04]

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

[commit-bot: I haz the power, 2016-09-07 15:38:04]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Charlie Reis, 2016-09-07 23:29:06]

On 2016/09/07 13:57:36, clamy wrote:
> @nasko & creis: PTAL
> This is the merge of CrossSiteResourceHandler & NavigationResourceThrottle
that
> allows to call ReadyToCommitNavigation in the current architecture. Note that
I
> also got rid of CrossSiteResourceRequest, since it turned out that we had a
> NavigationHandle accessible in the same place with the same info.

Exciting!  I'm eager to take a look, but I just got hit with a critical security
bug today.  I'll review as soon as I get a chance.

[nasko, 2016-09-08 00:35:36]

I started looking at the CL, however it will take some time to review it all.
I'm sending some initial comments I have on the code outside of the core
content/ changes.

https://codereview.chromium.org/2321543002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2321543002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:463:
ExtensionRegistry::Get(browser_context)->enabled_extensions(),
Does io_data->GetExtensionInfoMap()->extensions() only include enabled
extensions? I just want to make sure those are equivalent, since there are other
types in ExtensionRegistry.

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:197: // Should a transfer
navigation happen, |transfer_callback| will be run on
nit: Move to previous line.

https://codereview.chromium.org/2321543002/diff/20001/content/public/browser/...
File content/public/browser/content_browser_client.cc (right):

https://codereview.chromium.org/2321543002/diff/20001/content/public/browser/...
content/public/browser/content_browser_client.cc:111: BrowserContext*
browser_context, const GURL& current_url,
nit: Each parameter on new line.

[nasko, 2016-09-08 23:45:40]

I think this CL might be easier to understand overall if we have a version of
Charlie's cross-process navigation diagram for the new code.
I've done a pass through all of it, but still have hard time reasoning through
all of it.

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:591: bool
NavigationHandleImpl::CheckForTransfer() {
Should we DCHECK that render_frame_host_ is valid?

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:632: render_frame_host_,
GetURL());
Why call GetURL()? Should just use url_ to be consistent with the code above.

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:635: // Complete the
transfer if needed.
This comment is confusing to me. The following code will not complete a
transfer, but rather start the process of transferring the navigation, right?

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:243: void
TransferredToRenderFrameHost(RenderFrameHostImpl* render_frame_host);
The name and the comment implies that the transfer has happened, being past
tense. If I understand it correctly, we call this when we get the response from
the server and are trying to decide whether to transfer or not. When we pick a
different RenderFrameHost to transfer to, this is when we call it. Maybe as
simple as renaming it to TransferToRenderFrameHost, which indicates we want to
transfer to it going forward.

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:278: bool
GetFinalRenderFrameHost();
A GetType method should return Type, not bool. Let's try to find a better name
for this method.

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:282: bool
CheckForTransfer();
A bool return value from CheckForTransfer reads to me that when returning true
it needs to transfer. Maybe the name of the method should be changed to
something else.

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:340: // is_stream.
Do you plan to resolve this TODO before committing this CL or do it in a
separate one?

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (left):

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:409: // No need to navigate again. 
Just resume the deferred request.
Why do we not need this anymore?

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:290: // entry for
transfer navigations.  See https://crbug.com/495161.
Charlie has fixed https://crbug.com/495161 and we no longer need pending entries
for transfer navigations. Maybe remove that while cleaning up this area of the
code?

https://codereview.chromium.org/2321543002/diff/20001/content/browser/loader/...
File content/browser/loader/cross_site_resource_handler.cc (left):

https://codereview.chromium.org/2321543002/diff/20001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.cc:315:
info->set_cross_site_handler(this);
We should be able to remove set_cross_site_handler from the
ResourceRequestInfoImpl, right?

https://codereview.chromium.org/2321543002/diff/20001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.cc:388:
request()->LogBlockedBy("CrossSiteResourceHandler");
Can we port this over to the NavigationResourceHandler? It will be nice to have
logs when it defers navigational requests. Maybe a separate CL to avoid
convoluting this one, either before or after, as it is a bit of an orthogonal
change to the merge.

https://codereview.chromium.org/2321543002/diff/20001/content/browser/loader/...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/2321543002/diff/20001/content/browser/loader/...
content/browser/loader/navigation_resource_throttle.cc:350: void
NavigationResourceThrottle::InitiateTransfer() {
Can we put a DCHECK_CURRENTLY_ON to make it easier to understand which thread
this method is supposed to be called on?

https://codereview.chromium.org/2321543002/diff/20001/content/browser/loader/...
content/browser/loader/navigation_resource_throttle.cc:360: void
NavigationResourceThrottle::OnTransferDone() {
Same here, add DCHECK_CURRENTLY_ON.

https://codereview.chromium.org/2321543002/diff/20001/content/browser/loader/...
File content/browser/loader/navigation_resource_throttle.h (right):

https://codereview.chromium.org/2321543002/diff/20001/content/browser/loader/...
content/browser/loader/navigation_resource_throttle.h:56: void OnTransferDone();
nit: OnTransferComplete?

https://codereview.chromium.org/2321543002/diff/20001/content/browser/loader/...
File content/browser/loader/resource_loader.h (right):

https://codereview.chromium.org/2321543002/diff/20001/content/browser/loader/...
content/browser/loader/resource_loader.h:145: scoped_refptr<ResourceResponse>
response_;
Can we have a comment on this member? Especially since before it was kept only
for short period of time and now it looks like it is bound to the lifetime of
this object.

[Charlie Reis, 2016-09-09 05:17:14]

On 2016/09/08 23:45:40, nasko (slow) wrote:
> I think this CL might be easier to understand overall if we have a version of
> Charlie's cross-process navigation diagram for the new code.
> I've done a pass through all of it, but still have hard time reasoning through
> all of it.

I agree.  There's a lot of critical policy logic for the process model in
CrossSiteResourceHandler, and after a quick skim I see it deleted here but don't
understand how it works in the new approach.  Given how important (and subtle)
the old code is, I think a doc explaining how the new approach would help
immensely with understanding how it works after the change.

For reference, here's the diagram Nasko alluded to:
https://docs.google.com/drawings/d/15RDAahlW6ZWI2SyETsSFzVm25QhfiMaX_AHH1dlE3...

Doing an updated version of that (along with a doc explaining how the new
approach works) would really help.  Thanks!

[clamy, 2016-09-09 15:04:25]

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

[commit-bot: I haz the power, 2016-09-09 15:04:57]

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

[clamy, 2016-09-09 15:05:15]

Description was changed from

==========
Merge CrossSiteResourceHandler and NavigationResourceThrottle

This CL merges CrossSiteResourceHandler and NavigationResourceThrottle.
It also removes the CrossSiteResourceRequest in favor of using
NavigationHandle. This allows WebContentsObserver::ReadyToCommit
navigation to be called in the non-PlzNavigate architecture.

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

to

==========
Merge CrossSiteResourceHandler and NavigationResourceThrottle

This CL merges CrossSiteResourceHandler and NavigationResourceThrottle.
It also removes the CrossSiteResourceRequest in favor of using
NavigationHandle. This allows WebContentsObserver::ReadyToCommit
navigation to be called in the non-PlzNavigate architecture.

A detailed description of the change can be found at
https://docs.google.com/a/chromium.org/document/d/1Ze5_RNvUQV4ltWvFU5MY3L_TRI....

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

[clamy, 2016-09-09 15:06:42]

Thanks! I've also written a doc describing the changes (see link in CL
description). It includes an updated version of the cross-site navigation
diagram.
Note that I'm also still debugging why the WebNavigation cross process test is
failing.

https://codereview.chromium.org/2321543002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2321543002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:463:
ExtensionRegistry::Get(browser_context)->enabled_extensions(),
On 2016/09/08 00:35:36, nasko (slow) wrote:
> Does io_data->GetExtensionInfoMap()->extensions() only include enabled
> extensions? I just want to make sure those are equivalent, since there are
other
> types in ExtensionRegistry.

I think so? I was planning on getting someone from the extension team to review
this part so I can get it right. It's not very clear from the code what's in the
extension map. In the meantime I've replaced it with the set of installed
extensions.

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:591: bool
NavigationHandleImpl::CheckForTransfer() {
On 2016/09/08 23:45:39, nasko (slow) wrote:
> Should we DCHECK that render_frame_host_ is valid?

Done.

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:632: render_frame_host_,
GetURL());
On 2016/09/08 23:45:39, nasko (slow) wrote:
> Why call GetURL()? Should just use url_ to be consistent with the code above.

Done.

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:635: // Complete the
transfer if needed.
On 2016/09/08 23:45:39, nasko (slow) wrote:
> This comment is confusing to me. The following code will not complete a
> transfer, but rather start the process of transferring the navigation, right?

Done.

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:197: // Should a transfer
navigation happen, |transfer_callback| will be run on
On 2016/09/08 00:35:36, nasko (slow) wrote:
> nit: Move to previous line.

Done.

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:243: void
TransferredToRenderFrameHost(RenderFrameHostImpl* render_frame_host);
On 2016/09/08 23:45:39, nasko (slow) wrote:
> The name and the comment implies that the transfer has happened, being past
> tense. If I understand it correctly, we call this when we get the response
from
> the server and are trying to decide whether to transfer or not. When we pick a
> different RenderFrameHost to transfer to, this is when we call it. Maybe as
> simple as renaming it to TransferToRenderFrameHost, which indicates we want to
> transfer to it going forward.

Done.

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:278: bool
GetFinalRenderFrameHost();
On 2016/09/08 23:45:39, nasko (slow) wrote:
> A GetType method should return Type, not bool. Let's try to find a better name
> for this method. 

How about FindFinaleFrameHost?

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:282: bool
CheckForTransfer();
On 2016/09/08 23:45:39, nasko (slow) wrote:
> A bool return value from CheckForTransfer reads to me that when returning true
> it needs to transfer. Maybe the name of the method should be changed to
> something else. 

How about MaybeTransferAndProceed? It's not great (so suggestions are welcome),
but I think it conveys better the idea that you should not proceed if you get
false.

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:340: // is_stream.
On 2016/09/08 23:45:39, nasko (slow) wrote:
> Do you plan to resolve this TODO before committing this CL or do it in a
> separate one?

No I plan to do it in a separate one. Since this is for transfers, I don't need
to pass them in PlzNavigate. However we should do it in the future for the XFO
throttle to work.

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (left):

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:409: // No need to navigate again. 
Just resume the deferred request.
On 2016/09/08 23:45:39, nasko (slow) wrote:
> Why do we not need this anymore?

Because if we transfer to self, we won't pause the navigation in the
NavigationResourceThrottle. See the doc I've included for more info.

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:290: // entry for
transfer navigations.  See https://crbug.com/495161.
On 2016/09/08 23:45:39, nasko (slow) wrote:
> Charlie has fixed https://crbug.com/495161 and we no longer need pending
entries
> for transfer navigations. Maybe remove that while cleaning up this area of the
> code?

Done.

https://codereview.chromium.org/2321543002/diff/20001/content/browser/loader/...
File content/browser/loader/cross_site_resource_handler.cc (left):

https://codereview.chromium.org/2321543002/diff/20001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.cc:315:
info->set_cross_site_handler(this);
On 2016/09/08 23:45:39, nasko (slow) wrote:
> We should be able to remove set_cross_site_handler from the
> ResourceRequestInfoImpl, right?

Done. I thought I had done it in this patch, but apparently missed it.

https://codereview.chromium.org/2321543002/diff/20001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.cc:388:
request()->LogBlockedBy("CrossSiteResourceHandler");
On 2016/09/08 23:45:39, nasko (slow) wrote:
> Can we port this over to the NavigationResourceHandler? It will be nice to
have
> logs when it defers navigational requests. Maybe a separate CL to avoid
> convoluting this one, either before or after, as it is a bit of an orthogonal
> change to the merge.

We already have logging when the NavigationResourceThrottle blocks and unblocks
the request (it's done in ThrottlingResourceHandler).

https://codereview.chromium.org/2321543002/diff/20001/content/browser/loader/...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/2321543002/diff/20001/content/browser/loader/...
content/browser/loader/navigation_resource_throttle.cc:350: void
NavigationResourceThrottle::InitiateTransfer() {
On 2016/09/08 23:45:39, nasko (slow) wrote:
> Can we put a DCHECK_CURRENTLY_ON to make it easier to understand which thread
> this method is supposed to be called on?

Done.

https://codereview.chromium.org/2321543002/diff/20001/content/browser/loader/...
content/browser/loader/navigation_resource_throttle.cc:360: void
NavigationResourceThrottle::OnTransferDone() {
On 2016/09/08 23:45:39, nasko (slow) wrote:
> Same here, add DCHECK_CURRENTLY_ON.

Done.

https://codereview.chromium.org/2321543002/diff/20001/content/browser/loader/...
File content/browser/loader/navigation_resource_throttle.h (right):

https://codereview.chromium.org/2321543002/diff/20001/content/browser/loader/...
content/browser/loader/navigation_resource_throttle.h:56: void OnTransferDone();
On 2016/09/08 23:45:39, nasko (slow) wrote:
> nit: OnTransferComplete?

Done.

https://codereview.chromium.org/2321543002/diff/20001/content/browser/loader/...
File content/browser/loader/resource_loader.h (right):

https://codereview.chromium.org/2321543002/diff/20001/content/browser/loader/...
content/browser/loader/resource_loader.h:145: scoped_refptr<ResourceResponse>
response_;
On 2016/09/08 23:45:39, nasko (slow) wrote:
> Can we have a comment on this member? Especially since before it was kept only
> for short period of time and now it looks like it is bound to the lifetime of
> this object.

This object is no longer there since jam's patch removed cert store (we only
kept it to update the certificate).

https://codereview.chromium.org/2321543002/diff/20001/content/public/browser/...
File content/public/browser/content_browser_client.cc (right):

https://codereview.chromium.org/2321543002/diff/20001/content/public/browser/...
content/public/browser/content_browser_client.cc:111: BrowserContext*
browser_context, const GURL& current_url,
On 2016/09/08 00:35:36, nasko (slow) wrote:
> nit: Each parameter on new line.

Done.

[commit-bot: I haz the power, 2016-09-09 15:51:25]

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

[commit-bot: I haz the power, 2016-09-09 15:51:26]

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_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Charlie Reis, 2016-09-09 16:49:26]

On 2016/09/09 15:06:42, clamy wrote:
> Thanks! I've also written a doc describing the changes (see link in CL
> description). It includes an updated version of the cross-site navigation
> diagram.

Thanks!  That's exactly what I needed.  I'll find time to go through it in
depth.

[nasko, 2016-09-09 23:30:28]

I've done a mostly mechanical pass and read the design doc. I'll do another pass
on Monday to ensure that my understanding of the design based on the document is
matching the code.

https://codereview.chromium.org/2321543002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2321543002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:463:
ExtensionRegistry::Get(browser_context)->enabled_extensions(),
On 2016/09/09 15:06:41, clamy wrote:
> On 2016/09/08 00:35:36, nasko (slow) wrote:
> > Does io_data->GetExtensionInfoMap()->extensions() only include enabled
> > extensions? I just want to make sure those are equivalent, since there are
> other
> > types in ExtensionRegistry.
> 
> I think so? I was planning on getting someone from the extension team to
review
> this part so I can get it right. It's not very clear from the code what's in
the
> extension map. In the meantime I've replaced it with the set of installed
> extensions.

I poked Devlin and his response was that only enabled_extensions() would be in
the map of the IO thread.

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:278: bool
GetFinalRenderFrameHost();
On 2016/09/09 15:06:41, clamy wrote:
> On 2016/09/08 23:45:39, nasko (slow) wrote:
> > A GetType method should return Type, not bool. Let's try to find a better
name
> > for this method. 
> 
> How about FindFinaleFrameHost?

sgtm

https://codereview.chromium.org/2321543002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:282: bool
CheckForTransfer();
On 2016/09/09 15:06:41, clamy wrote:
> On 2016/09/08 23:45:39, nasko (slow) wrote:
> > A bool return value from CheckForTransfer reads to me that when returning
true
> > it needs to transfer. Maybe the name of the method should be changed to
> > something else. 
> 
> How about MaybeTransferAndProceed? It's not great (so suggestions are
welcome),
> but I think it conveys better the idea that you should not proceed if you get
> false.

I think it is definitely better. If I think of anything else, I'll propose it. I
also bet Charlie will have good input on the names of those methods.

https://codereview.chromium.org/2321543002/diff/20001/content/browser/loader/...
File content/browser/loader/cross_site_resource_handler.cc (left):

https://codereview.chromium.org/2321543002/diff/20001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.cc:388:
request()->LogBlockedBy("CrossSiteResourceHandler");
On 2016/09/09 15:06:41, clamy wrote:
> On 2016/09/08 23:45:39, nasko (slow) wrote:
> > Can we port this over to the NavigationResourceHandler? It will be nice to
> have
> > logs when it defers navigational requests. Maybe a separate CL to avoid
> > convoluting this one, either before or after, as it is a bit of an
orthogonal
> > change to the merge.
> 
> We already have logging when the NavigationResourceThrottle blocks and
unblocks
> the request (it's done in ThrottlingResourceHandler).

Acknowledged.

https://codereview.chromium.org/2321543002/diff/20001/content/browser/loader/...
File content/browser/loader/resource_loader.h (right):

https://codereview.chromium.org/2321543002/diff/20001/content/browser/loader/...
content/browser/loader/resource_loader.h:145: scoped_refptr<ResourceResponse>
response_;
On 2016/09/09 15:06:41, clamy wrote:
> On 2016/09/08 23:45:39, nasko (slow) wrote:
> > Can we have a comment on this member? Especially since before it was kept
only
> > for short period of time and now it looks like it is bound to the lifetime
of
> > this object.
> 
> This object is no longer there since jam's patch removed cert store (we only
> kept it to update the certificate).

Acknowledged.

https://codereview.chromium.org/2321543002/diff/60001/content/browser/loader/...
File content/browser/loader/DEPS (right):

https://codereview.chromium.org/2321543002/diff/60001/content/browser/loader/...
content/browser/loader/DEPS:343: #TODO: To be removed.
Can you add a bit of context for this? Why/when this needs to be removed? What
is the reason it needs to be removed.

https://codereview.chromium.org/2321543002/diff/60001/content/browser/loader/...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/2321543002/diff/60001/content/browser/loader/...
content/browser/loader/navigation_resource_throttle.cc:364: // If the results of
the checks on the UI thread are known, unblock the
Is it guaranteed that the result from the UI thread will never be DEFER?

[clamy, 2016-09-12 15:09:36]

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

[commit-bot: I haz the power, 2016-09-12 15:09:47]

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

[clamy, 2016-09-12 15:31:38]

Thanks!

https://codereview.chromium.org/2321543002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2321543002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:463:
ExtensionRegistry::Get(browser_context)->enabled_extensions(),
On 2016/09/09 23:30:27, nasko (slow) wrote:
> On 2016/09/09 15:06:41, clamy wrote:
> > On 2016/09/08 00:35:36, nasko (slow) wrote:
> > > Does io_data->GetExtensionInfoMap()->extensions() only include enabled
> > > extensions? I just want to make sure those are equivalent, since there are
> > other
> > > types in ExtensionRegistry.
> > 
> > I think so? I was planning on getting someone from the extension team to
> review
> > this part so I can get it right. It's not very clear from the code what's in
> the
> > extension map. In the meantime I've replaced it with the set of installed
> > extensions.
> 
> I poked Devlin and his response was that only enabled_extensions() would be in
> the map of the IO thread. 

Ok. Went back to that.

https://codereview.chromium.org/2321543002/diff/60001/content/browser/loader/...
File content/browser/loader/DEPS (right):

https://codereview.chromium.org/2321543002/diff/60001/content/browser/loader/...
content/browser/loader/DEPS:343: #TODO: To be removed.
On 2016/09/09 23:30:27, nasko (slow) wrote:
> Can you add a bit of context for this? Why/when this needs to be removed? What
> is the reason it needs to be removed.

Added an explanation. The issue is that we have to use a test function for main
requests, but that will go away when PlzNavigate lands.

https://codereview.chromium.org/2321543002/diff/60001/content/browser/loader/...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/2321543002/diff/60001/content/browser/loader/...
content/browser/loader/navigation_resource_throttle.cc:364: // If the results of
the checks on the UI thread are known, unblock the
On 2016/09/09 23:30:27, nasko (slow) wrote:
> Is it guaranteed that the result from the UI thread will never be DEFER?

Yes. See the DCHECK line 331 (UI thread binds OnUIChecksPerformed directly).

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

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

[commit-bot: I haz the power, 2016-09-12 16:00:07]

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...)

[Charlie Reis, 2016-09-16 21:19:24]

This is looking pretty good.  I'm kind of taking it on faith (and green tests)
that NavigationResourceThrottle accomplishes the same properties as intercepting
the navigation in CSRH, because it's hard to confirm that they're equivalent
from code inspection.  (The design doc did help.)

Note: Are the Android test failures real?

Random question: Do you know if it would be any easier to transfer processes on
error pages in the new approach than before?  We have a big problem today that a
DNS error (or invalid extension URL, etc) ends up in the current process, when
the actual page at that URL would have required a process swap.  There wasn't a
good way to fix that with CSRH, and I'm curious if it's possible now.  (No need
to do that in this CL, but I'm hoping we can find a way forward now.  I noticed
we don't get it for free by patching in this CL locally.)

https://codereview.chromium.org/2321543002/diff/100001/content/browser/devtoo...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/2321543002/diff/100001/content/browser/devtoo...
content/browser/devtools/render_frame_devtools_agent_host.cc:565: if
(!IsBrowserSideNavigationEnabled())
I'm a bit surprised by this, since one motivation of the CL is to get
ReadyToCommitNavigation to work in all modes.  I agree that this avoids tacking
additional changes onto this CL, but will this code eventually want to handle
ReadyToCommitNavigation in non-PlzNavigate mode?  If so, let's add a TODO to
make the intention clear.

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:88:
redirect_chain_.push_back(url);
Why is this after the DidStartNavigation call?  I guess it's not exposed
publicly, but if we did expose it later, delegates could query it from
DidStartNavigation.

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:246: // it's ready to
commit. Determine the final RenderFrameHost handling it.
nit: Change last sentence to: Determine which RenderFrameHost should render the
response, based on its site (after any redirects).

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:428: // for the navigation.
Let's use the same comment as in Resume().

Why do we need this in both places?  I'm not sure how they relate to each other,
and why we need both. (Maybe the comment can clarify?)

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:468:
render_frame_host_->GetSiteInstance()->GetId()) {
It's a bit unexpected to call this method if no transfer is happening.  Can we
check for that before calling it in RFHM and just ensure that this is only
called when a transfer is in progress?

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:469: // This is an actual
transfer. Inform the NavigationResourceThrottle.
Please elaborate on why the IO thread needs to know.  There's a comment in RFHM
about marking the request as transferring, but that's lacking a bit of context
as well.

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:475: void
NavigationHandleImpl::TransferToRenderFrameHost(
This is very different from TransferToSiteInstance, even though the names sound
the same.  I think we should just keep it as set_render_frame_host (keeping it
all in the .h file) and continue documenting that it's used for transfers.

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:576: bool
NavigationHandleImpl::FindFinalRenderFrameHost() {
This method feels misnamed, because it doesn't do any of the RFH selection. 
This one would actually be a better candidate to call MaybeTransferAndProceed.

It appears that this is mainly a wrapper for MaybeTransferAndProceed below,
ensuring that we call ReadyToCommitNavigation if it returns true.  If so, we
could rename the one below to MaybeTransferAndProceedInternal (which has
precedent in some method names in NavigationControllerImpl).

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:593: // decided before
calling WillProcessResponse.
Where does this happen?  It would have been nice to move away from
CrossSiteResourceHandler to something shared with PlzNavigate, rather than a
different flow that will need to change again.  Why does PlzNavigate pick its
RFH at a different time than this new approach?

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:235: const GlobalRequestID&
request_id() const { return request_id_; }
Please comment about the timeline that this is valid.  How soon after the
request is created is it set?  Is it always set, or only for transfers?  (I'm
talking with Rohit about a case that might want to use this.)

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:275: // RenderFrameHost for
the navigation. Returns true if the navigation should
Can you mention a case when it would return false?  I'm not sure I understand
why this would abort at first glance.  (That sounds more like something
MaybeTransferAndProceed would do.)

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:357: bool
should_replace_current_entry_;
I'm curious, why did these become necessary in the new approach?

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (left):

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:466: if
(!transfer_navigation_handle_)
Why isn't this possible anymore?  The comment indicates there may be security
checks that might have cleared the handle before we get here.

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:2213: // In case this is
a transfer navigation, inform the Navigationhandle of which
nit: NavigationHandle

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:2216: // pending RFH.
Why does the IO thread care about the destruction of the pending RFH?  (We
should clarify in the comment.)

https://codereview.chromium.org/2321543002/diff/100001/content/browser/loader...
File content/browser/loader/cross_site_resource_handler.cc (left):

https://codereview.chromium.org/2321543002/diff/100001/content/browser/loader...
content/browser/loader/cross_site_resource_handler.cc:104: if
(manager->IsRendererTransferNeededForNavigation(rfh, real_url))
Sanity check: This used to prevent us from doing cross-process navigations for
cross-site link clicks, which would happen if we went through RFHM on every
navigation.  I'm guessing that's handled by the equivalent call in
NavigationHandleImpl::MaybeTransferAndProceed() now?

https://codereview.chromium.org/2321543002/diff/100001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.h (right):

https://codereview.chromium.org/2321543002/diff/100001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.h:42: CONTENT_EXPORT static
void SetUIChecksAlwaysSuccedForTesting(
Succed -> Succeed

https://codereview.chromium.org/2321543002/diff/100001/content/browser/loader...
File content/browser/loader/resource_loader.h (right):

https://codereview.chromium.org/2321543002/diff/100001/content/browser/loader...
content/browser/loader/resource_loader.h:134: base::Closure
on_transfer_complete_;
nit: on_transfer_complete_callback_

[clamy, 2016-09-20 13:31:11]

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

[commit-bot: I haz the power, 2016-09-20 13:31:34]

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-09-20 14:41:10]

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

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

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

[clamy, 2016-09-20 15:57:00]

On 2016/09/16 21:19:24, Charlie Reis (slow) wrote:
> This is looking pretty good.  I'm kind of taking it on faith (and green tests)
> that NavigationResourceThrottle accomplishes the same properties as
intercepting
> the navigation in CSRH, because it's hard to confirm that they're equivalent
> from code inspection.  (The design doc did help.)
> 
> Note: Are the Android test failures real?

Yes I'm still investigating (it takes so much longer to do so on Android :/).

> 
> Random question: Do you know if it would be any easier to transfer processes
on
> error pages in the new approach than before?  We have a big problem today that
a
> DNS error (or invalid extension URL, etc) ends up in the current process, when
> the actual page at that URL would have required a process swap.  There wasn't
a
> good way to fix that with CSRH, and I'm curious if it's possible now.  (No
need
> to do that in this CL, but I'm hoping we can find a way forward now.  I
noticed
> we don't get it for free by patching in this CL locally.)

Even with the change, I think it would be somewhat hard. To do so, we would have
to make the NavigationResourceThrottle a ResourceHandler so that we have access
to the OnResponseCompleted call that we could then defer. I'm not event sure it
would fully work though, as there may expectations in content/loader about how
OnResponseCompleted behaves that we would violate. Note: in PlzNavigate, this
already works.

[clamy, 2016-09-20 15:57:22]

https://codereview.chromium.org/2321543002/diff/100001/content/browser/devtoo...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/2321543002/diff/100001/content/browser/devtoo...
content/browser/devtools/render_frame_devtools_agent_host.cc:565: if
(!IsBrowserSideNavigationEnabled())
On 2016/09/16 21:19:25, Charlie Reis (slow) wrote:
> I'm a bit surprised by this, since one motivation of the CL is to get
> ReadyToCommitNavigation to work in all modes.  I agree that this avoids
tacking
> additional changes onto this CL, but will this code eventually want to handle
> ReadyToCommitNavigation in non-PlzNavigate mode?  If so, let's add a TODO to
> make the intention clear.

Done.

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:88:
redirect_chain_.push_back(url);
On 2016/09/16 21:19:25, Charlie Reis (slow) wrote:
> Why is this after the DidStartNavigation call?  I guess it's not exposed
> publicly, but if we did expose it later, delegates could query it from
> DidStartNavigation.

Done.

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:246: // it's ready to
commit. Determine the final RenderFrameHost handling it.
On 2016/09/16 21:19:25, Charlie Reis (slow) wrote:
> nit: Change last sentence to: Determine which RenderFrameHost should render
the
> response, based on its site (after any redirects).

Done.

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:428: // for the navigation.
On 2016/09/16 21:19:25, Charlie Reis (slow) wrote:
> Let's use the same comment as in Resume().
> 
> Why do we need this in both places?  I'm not sure how they relate to each
other,
> and why we need both. (Maybe the comment can clarify?)

Done. I've slightly modified the first part of the comments to make it clearer
what the 2 cases are:
- if no NavigationThrottle defers during WillProcessResponse, then we call
FindFinalRenderFrameHost from here.
- otherwise we call it from Resume when the navigation is resumed.

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:468:
render_frame_host_->GetSiteInstance()->GetId()) {
On 2016/09/16 21:19:26, Charlie Reis (slow) wrote:
> It's a bit unexpected to call this method if no transfer is happening.  Can we
> check for that before calling it in RFHM and just ensure that this is only
> called when a transfer is in progress?

Done.

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:469: // This is an actual
transfer. Inform the NavigationResourceThrottle.
On 2016/09/16 21:19:25, Charlie Reis (slow) wrote:
> Please elaborate on why the IO thread needs to know.  There's a comment in
RFHM
> about marking the request as transferring, but that's lacking a bit of context
> as well.

I have elaborated in the comments.

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:475: void
NavigationHandleImpl::TransferToRenderFrameHost(
On 2016/09/16 21:19:26, Charlie Reis (slow) wrote:
> This is very different from TransferToSiteInstance, even though the names
sound
> the same.  I think we should just keep it as set_render_frame_host (keeping it
> all in the .h file) and continue documenting that it's used for transfers.

Done.

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:576: bool
NavigationHandleImpl::FindFinalRenderFrameHost() {
On 2016/09/16 21:19:25, Charlie Reis (slow) wrote:
> This method feels misnamed, because it doesn't do any of the RFH selection. 
> This one would actually be a better candidate to call MaybeTransferAndProceed.
> 
> It appears that this is mainly a wrapper for MaybeTransferAndProceed below,
> ensuring that we call ReadyToCommitNavigation if it returns true.  If so, we
> could rename the one below to MaybeTransferAndProceedInternal (which has
> precedent in some method names in NavigationControllerImpl).

Done.

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:593: // decided before
calling WillProcessResponse.
On 2016/09/16 21:19:26, Charlie Reis (slow) wrote:
> Where does this happen?  It would have been nice to move away from
> CrossSiteResourceHandler to something shared with PlzNavigate, rather than a
> different flow that will need to change again.  Why does PlzNavigate pick its
> RFH at a different time than this new approach?

We already had code written to chose a RFH to commit navigation with
PlzNavigate, and it is quite tied to the speculative RFH. I have added a TODO to
try to use the code here instead. I don't want to do it in this patch, since
it's adding more complexity to an already complex patch.

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:235: const GlobalRequestID&
request_id() const { return request_id_; }
On 2016/09/16 21:19:26, Charlie Reis (slow) wrote:
> Please comment about the timeline that this is valid.  How soon after the
> request is created is it set?  Is it always set, or only for transfers?  (I'm
> talking with Rohit about a case that might want to use this.)

Done. Added a DCHECK as well. As explained in the todo, this could potentially
be initialized earlier (though there may be some issues in PlzNavigate).

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:275: // RenderFrameHost for
the navigation. Returns true if the navigation should
On 2016/09/16 21:19:26, Charlie Reis (slow) wrote:
> Can you mention a case when it would return false?  I'm not sure I understand
> why this would abort at first glance.  (That sounds more like something
> MaybeTransferAndProceed would do.)

Updated the comment following the naming change suggested in the .cc file.

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:357: bool
should_replace_current_entry_;
On 2016/09/16 21:19:26, Charlie Reis (slow) wrote:
> I'm curious, why did these become necessary in the new approach?

Most of them are data that was contained in the CrossSiteTransferringRequest.
Since I'm removing this class, I need to include them in the NavigationHandle,
which now plays a role very similar to the CrossSiteTransferringRequest.

is_download_ and is_stream_ are needed to check whether we should attempt to
transfer or not.

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (left):

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:466: if
(!transfer_navigation_handle_)
On 2016/09/16 21:19:26, Charlie Reis (slow) wrote:
> Why isn't this possible anymore?  The comment indicates there may be security
> checks that might have cleared the handle before we get here.

We now only call OnCrossSiteResponse from the NavigationHandle, so it has to be
here. This is meant to replace the CHECK(cross_site_transferring_request).

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:2213: // In case this is
a transfer navigation, inform the Navigationhandle of which
On 2016/09/16 21:19:26, Charlie Reis (slow) wrote:
> nit: NavigationHandle

Done.

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:2216: // pending RFH.
On 2016/09/16 21:19:26, Charlie Reis (slow) wrote:
> Why does the IO thread care about the destruction of the pending RFH?  (We
> should clarify in the comment.)

Done.

https://codereview.chromium.org/2321543002/diff/100001/content/browser/loader...
File content/browser/loader/cross_site_resource_handler.cc (left):

https://codereview.chromium.org/2321543002/diff/100001/content/browser/loader...
content/browser/loader/cross_site_resource_handler.cc:104: if
(manager->IsRendererTransferNeededForNavigation(rfh, real_url))
On 2016/09/16 21:19:25, Charlie Reis (slow) wrote:
> Sanity check: This used to prevent us from doing cross-process navigations for
> cross-site link clicks, which would happen if we went through RFHM on every
> navigation.  I'm guessing that's handled by the equivalent call in
> NavigationHandleImpl::MaybeTransferAndProceed() now?

Yes we call it in NavigationHandleImpl::MaybeTransferAndProceedInternal.

https://codereview.chromium.org/2321543002/diff/100001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.h (right):

https://codereview.chromium.org/2321543002/diff/100001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.h:42: CONTENT_EXPORT static
void SetUIChecksAlwaysSuccedForTesting(
On 2016/09/16 21:19:26, Charlie Reis (slow) wrote:
> Succed -> Succeed

Done.

https://codereview.chromium.org/2321543002/diff/100001/content/browser/loader...
File content/browser/loader/resource_loader.h (right):

https://codereview.chromium.org/2321543002/diff/100001/content/browser/loader...
content/browser/loader/resource_loader.h:134: base::Closure
on_transfer_complete_;
On 2016/09/16 21:19:26, Charlie Reis (slow) wrote:
> nit: on_transfer_complete_callback_

Done.

[Charlie Reis, 2016-09-21 03:31:48]

Thanks for the explanations!  LGTM with nits.  We'll want to keep an eye out for
any unexpected behavior changes, but I'm excited to have so much less code and
fewer thread hops for process transfers.

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:593: // decided before
calling WillProcessResponse.
On 2016/09/20 15:57:22, clamy wrote:
> On 2016/09/16 21:19:26, Charlie Reis (slow) wrote:
> > Where does this happen?  It would have been nice to move away from
> > CrossSiteResourceHandler to something shared with PlzNavigate, rather than a
> > different flow that will need to change again.  Why does PlzNavigate pick
its
> > RFH at a different time than this new approach?
> 
> We already had code written to chose a RFH to commit navigation with
> PlzNavigate, and it is quite tied to the speculative RFH. I have added a TODO
to
> try to use the code here instead. I don't want to do it in this patch, since
> it's adding more complexity to an already complex patch.

Yes, no need to move PlzNavigate code in this CL.  I was mainly wondering if we
could use the existing PlzNavigate code instead of introducing something new and
different, but it sounds like that's not possible due to the speculative RFH
code.  That's fine.

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:357: bool
should_replace_current_entry_;
On 2016/09/20 15:57:22, clamy wrote:
> On 2016/09/16 21:19:26, Charlie Reis (slow) wrote:
> > I'm curious, why did these become necessary in the new approach?
> 
> Most of them are data that was contained in the CrossSiteTransferringRequest.
> Since I'm removing this class, I need to include them in the NavigationHandle,
> which now plays a role very similar to the CrossSiteTransferringRequest.

CrossSiteTransferringRequest only had the GlobalRequestID.  I was curious about
should_replace_current_entry_ and redirect_chain_, but it looks like those used
to be posted from CrossSiteResourceHandler to RFHM::OnCrossSiteResponse, and now
we pass them in from here.  Seems reasonable.

> 
> is_download_ and is_stream_ are needed to check whether we should attempt to
> transfer or not.

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (left):

https://codereview.chromium.org/2321543002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:466: if
(!transfer_navigation_handle_)
On 2016/09/20 15:57:22, clamy wrote:
> On 2016/09/16 21:19:26, Charlie Reis (slow) wrote:
> > Why isn't this possible anymore?  The comment indicates there may be
security
> > checks that might have cleared the handle before we get here.
> 
> We now only call OnCrossSiteResponse from the NavigationHandle, so it has to
be
> here. This is meant to replace the CHECK(cross_site_transferring_request).

Acknowledged.

https://codereview.chromium.org/2321543002/diff/140001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2321543002/diff/140001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:247: // WillStartRequest
will be called before starting a requets on the IO thread.
nit: request

https://codereview.chromium.org/2321543002/diff/140001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:290: // proceed.
I think it's worth mentioning a case that the transfer results in the
destruction of the NavHandle, since it's not obvious what would cause that at
first glance.  If I understand correctly,
WebContentsDelegate::ShouldTransferNavigation could return false to cause that
to happen, right?

https://codereview.chromium.org/2321543002/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/2321543002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:2220: // RFH but will
presist until it is picked up by the new RFH.
nit: persist

https://codereview.chromium.org/2321543002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:2223:
new_instance->GetId() !=
It should be safe to compare the SiteInstances directly without calling GetId().

[clamy, 2016-09-21 14:36:06]

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

[commit-bot: I haz the power, 2016-09-21 14:36:25]

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-09-21 14:48:14]

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

[commit-bot: I haz the power, 2016-09-21 14:48:15]

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

[clamy, 2016-09-21 15:02:45]

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

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

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

[clamy, 2016-09-21 15:04:06]

Thanks! I've investigated the Android issue and found the rootcause.

In RenderFrameImpl, we have 2 paths to load data urls. If we have a base url, we
call LoadData directly, if we don't we load the request normally. In the
WebURLLoaderImpl, we try to handle the data url locally, otherwise we send it to
the browser. Now with interstitials, we don't provide a base url, so we go
through the normal request load path. Every platform but Android handles the
data url locally. This is because Android sends all data urls to the browser so
that WebView knows about them or they can be intercepted by apps that have
registered handlers for them. And with this change, since we don't have a
NavigationHandle registered for them, the navigation is cancelled.

So in the case the URL we're navigating to is a data url, I'm not canceling the
navigation anymore if we don't find a NavigationHandle for it. Still, we may
want to check at one point whether we still want to make all data urls in the
renderer go to the browser on Android.

https://codereview.chromium.org/2321543002/diff/140001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2321543002/diff/140001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:247: // WillStartRequest
will be called before starting a requets on the IO thread.
On 2016/09/21 03:31:47, Charlie Reis (slow) wrote:
> nit: request

Done.

https://codereview.chromium.org/2321543002/diff/140001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:290: // proceed.
On 2016/09/21 03:31:47, Charlie Reis (slow) wrote:
> I think it's worth mentioning a case that the transfer results in the
> destruction of the NavHandle, since it's not obvious what would cause that at
> first glance.  If I understand correctly,
> WebContentsDelegate::ShouldTransferNavigation could return false to cause that
> to happen, right?

Yes. Mentioned that case.

https://codereview.chromium.org/2321543002/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/2321543002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:2220: // RFH but will
presist until it is picked up by the new RFH.
On 2016/09/21 03:31:47, Charlie Reis (slow) wrote:
> nit: persist

Done.

https://codereview.chromium.org/2321543002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:2223:
new_instance->GetId() !=
On 2016/09/21 03:31:47, Charlie Reis (slow) wrote:
> It should be safe to compare the SiteInstances directly without calling
GetId().

Done.

[clamy, 2016-09-21 15:05:12]

clamy@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[clamy, 2016-09-21 15:06:49]

clamy@chromium.org changed reviewers:
+ mmenke@chromium.org, sky@chromium.org

[clamy, 2016-09-21 15:10:22]

@mmenke: PTAL at the changes in content/browser/loader (especially
ResourceLoader) & chrome/browser/prerender
@sky: PTAL at the changes in chrome/browser/ui
@rdcronin: PTAL at the changes in ChromeContentBrowserClientExtensionPart.
Please note that the method changed is no longer called on the IO thread but on
the UI thread.

[sky, 2016-09-21 16:25:27]

LGTM

[Charlie Reis, 2016-09-21 16:47:08]

Hmm, I have some questions about the fix for Android, since I don't yet see why
it's correct.

https://codereview.chromium.org/2321543002/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2321543002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2286:
FrameMsg_UILoadMetricsReportType::NO_REPORT, GURL(), GURL(), LOFI_OFF,
I don't understand this change.  Was it wrong before?  I'm wondering if we
should land it in a separate CL, in case it causes a regression independent of
the rest.

https://codereview.chromium.org/2321543002/diff/180001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/2321543002/diff/180001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.cc:148: bool
proceed_if_handle_not_found) {
This needs some documentation somewhere.  I don't understand how this is a
problem specific to data URLs-- why do they not have a NavigationHandle?  It
seems like it might be possible for other cases that we don't currently have
test coverage for.  Can you clarify if it's actually limited to data URLs?

(For example, there's also
RendererPreferences::browser_handles_all_top_level_requests, which sounds like
it could have a similar effect to Android sending all data URLs?)

[commit-bot: I haz the power, 2016-09-21 17:01:43]

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

[commit-bot: I haz the power, 2016-09-21 17:01:44]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-09-21 17:10:24]

content/browser/loader LGTM
chrome/browser/prerender LGTM

I'm deferring to creis on the NavigationResourceThrottle and RDH_unittest
changes.  If you want me to take a careful look at them, I can, I just suspect
it will take me a lot longer than creis, and I won't do as good a job, since I
don't currently know navigation that well.

https://codereview.chromium.org/2321543002/diff/180001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/2321543002/diff/180001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.cc:154: }
nit:  Remove braces/

https://codereview.chromium.org/2321543002/diff/180001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/2321543002/diff/180001/content/browser/loader...
content/browser/loader/resource_loader.cc:239:
on_transfer_complete_callback_.Reset();
Suggest base::ResetAndReturn(on_transfer_complete_callback_).Run();

While we won't be marking the same request as transferring twice, re-entrantly,
just seems more robust.

[Devlin, 2016-09-21 17:11:17]

extensions lgtm

[clamy, 2016-09-22 16:27:24]

Thanks!
@creis: a few more clarifications about the Android interstitial issue.

https://codereview.chromium.org/2321543002/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2321543002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2286:
FrameMsg_UILoadMetricsReportType::NO_REPORT, GURL(), GURL(), LOFI_OFF,
On 2016/09/21 16:47:08, Charlie Reis (slow) wrote:
> I don't understand this change.  Was it wrong before?  I'm wondering if we
> should land it in a separate CL, in case it causes a regression independent of
> the rest.

Are you referring to patchset 9? I tried 2 approach:
- the one in patchset 9 which is to provide a base url in the
CommonNavigationParams, so that we call LoadData in
RenderFrameImpl::NavigateInternal, instead of creating a normal request. Note
that error page are also loaded via LoadData, so that seemed reasonnable.
- the one in patchset 10, where I became worried that we may have issues with
other types of data urls and modified the NavigationResourceThrottle code. But I
now realized that the situation is specific to interstitials, because it comes
from the Navigator not being a NavigatorImpl in the interstitial navigation
case.

Now that I think about it, it seems the change in patchset 9 is better, wdyt?
Note that I can land it separately prior to this patch.

https://codereview.chromium.org/2321543002/diff/180001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/2321543002/diff/180001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.cc:148: bool
proceed_if_handle_not_found) {
On 2016/09/21 16:47:08, Charlie Reis (slow) wrote:
> This needs some documentation somewhere.  I don't understand how this is a
> problem specific to data URLs-- why do they not have a NavigationHandle?  It
> seems like it might be possible for other cases that we don't currently have
> test coverage for.  Can you clarify if it's actually limited to data URLs?
> 
> (For example, there's also
> RendererPreferences::browser_handles_all_top_level_requests, which sounds like
> it could have a similar effect to Android sending all data URLs?)

Actually this is limited to interstitials: it's because with interstitials the
NavigatorImpl is replaced by the interstitial page itself, so the code in
NavigatorImpl::OnDidStartProvisionalLoad that creates the handle is not
triggered. Now that I think about it some more, it's probably better to just
ensure that interstitials are always loaded as a data url (patchset 9) rather
than this fix. Wdyt?

[nasko, 2016-09-22 22:44:55]

Everything but the Android interstitial page handling l-g-t-m. I would like us
to read a better solution before committing this CL though.

https://codereview.chromium.org/2321543002/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2321543002/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:506: // renderer. Instead
it will persist until being picked up by the transfer
nit: Let's be precise. Either "renderer process" or
"RenderFrame|RenderFrameHost".

https://codereview.chromium.org/2321543002/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:624: // PlzNavigate: the
final RenderFrameHost handling this navigation has been
nit: Empty line before the comment.

https://codereview.chromium.org/2321543002/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:626: //
NavigationRequest::OnresponseStarted.
nit: OnResponseStarted

https://codereview.chromium.org/2321543002/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:663: // "cross-site"
chrome:// frames, and that doesn't yet work cross- process.
nit: cross-process.

https://codereview.chromium.org/2321543002/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2321543002/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:246: // non-transfer code.
There may some issue in PlzNavigate, where
nit: "There may some issue in PlzNavigate" doesn't read very well. "There may be
some issues in PlzNavigate"?

https://codereview.chromium.org/2321543002/diff/180001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/2321543002/diff/180001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.cc:148: bool
proceed_if_handle_not_found) {
On 2016/09/22 16:27:23, clamy wrote:
> On 2016/09/21 16:47:08, Charlie Reis (slow) wrote:
> > This needs some documentation somewhere.  I don't understand how this is a
> > problem specific to data URLs-- why do they not have a NavigationHandle?  It
> > seems like it might be possible for other cases that we don't currently have
> > test coverage for.  Can you clarify if it's actually limited to data URLs?
> > 
> > (For example, there's also
> > RendererPreferences::browser_handles_all_top_level_requests, which sounds
like
> > it could have a similar effect to Android sending all data URLs?)
> 
> Actually this is limited to interstitials: it's because with interstitials the
> NavigatorImpl is replaced by the interstitial page itself, so the code in
> NavigatorImpl::OnDidStartProvisionalLoad that creates the handle is not
> triggered. Now that I think about it some more, it's probably better to just
> ensure that interstitials are always loaded as a data url (patchset 9) rather
> than this fix. Wdyt?

I am not a fan of this approach either. Putting all data URLs in the same bucket
as interstitials is a recipe for problems.

https://codereview.chromium.org/2321543002/diff/180001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.cc:161: if
(!render_frame_host) {
I don't quite understand this. In the case of interstitials, why would we not
find the RFH that is rendering it? We navigate it from the browser process and
presumably because we don't have base/history URL associated with it, we send it
to the browser process to load on Android. Doesn't that resource request get
tagged with the right process id/frame id?
If we manage to get a RenderFrameHost (which I think we should), we can use
InterstitialPage::FromRenderFrameHost to check if it is rendering an
interstitial and let it proceed directly.

In general it also seems busted to allow Android code to know about interstitial
page loads. Can we give it a base URL such that we know it is interstitial vs
any other data URL?

[Charlie Reis, 2016-09-23 05:46:55]

Thanks for the explanations!  That helps, and sorry for the misunderstanding on
the inter-patchset diff.  Some thoughts about it below.

https://codereview.chromium.org/2321543002/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2321543002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2286:
FrameMsg_UILoadMetricsReportType::NO_REPORT, GURL(), GURL(), LOFI_OFF,
On 2016/09/22 16:27:23, clamy wrote:
> On 2016/09/21 16:47:08, Charlie Reis (slow) wrote:
> > I don't understand this change.  Was it wrong before?  I'm wondering if we
> > should land it in a separate CL, in case it causes a regression independent
of
> > the rest.
> 
> Are you referring to patchset 9? I tried 2 approach:
> - the one in patchset 9 which is to provide a base url in the
> CommonNavigationParams, so that we call LoadData in
> RenderFrameImpl::NavigateInternal, instead of creating a normal request. Note
> that error page are also loaded via LoadData, so that seemed reasonnable.
> - the one in patchset 10, where I became worried that we may have issues with
> other types of data urls and modified the NavigationResourceThrottle code. But
I
> now realized that the situation is specific to interstitials, because it comes
> from the Navigator not being a NavigatorImpl in the interstitial navigation
> case.
> 
> Now that I think about it, it seems the change in patchset 9 is better, wdyt?
> Note that I can land it separately prior to this patch.

Oh, I misunderstood!  I was looking at the diff to patchset 9, thinking it was
the diff to patchset 8.  Now I see you were just putting things back.

(As discussed in navigation_resource_throttle.cc, I'm hesitant to do the change
from patchset 9.  Those parameters are meant for LoadBaseURLForDataURL and have
strange behavior, and I don't want other features to depend on them.)

https://codereview.chromium.org/2321543002/diff/180001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/2321543002/diff/180001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.cc:148: bool
proceed_if_handle_not_found) {
On 2016/09/22 22:44:55, nasko (slow) wrote:
> On 2016/09/22 16:27:23, clamy wrote:
> > On 2016/09/21 16:47:08, Charlie Reis (slow) wrote:
> > > This needs some documentation somewhere.  I don't understand how this is a
> > > problem specific to data URLs-- why do they not have a NavigationHandle? 
It
> > > seems like it might be possible for other cases that we don't currently
have
> > > test coverage for.  Can you clarify if it's actually limited to data URLs?
> > > 
> > > (For example, there's also
> > > RendererPreferences::browser_handles_all_top_level_requests, which sounds
> like
> > > it could have a similar effect to Android sending all data URLs?)
> > 
> > Actually this is limited to interstitials: it's because with interstitials
the
> > NavigatorImpl is replaced by the interstitial page itself, so the code in
> > NavigatorImpl::OnDidStartProvisionalLoad that creates the handle is not
> > triggered. Now that I think about it some more, it's probably better to just
> > ensure that interstitials are always loaded as a data url (patchset 9)
rather
> > than this fix. Wdyt?
> 
> I am not a fan of this approach either. Putting all data URLs in the same
bucket
> as interstitials is a recipe for problems.

Let's avoid using the parameters that are meant for LoadBaseURLForDataURL, which
is an Android Webview feature with unfortunate behavior.

I'm happy to understand the root of the problem now, though: interstitials that
don't have NavigatorImpl, and thus don't have NavigationHandles.  Any chance we
could make InterstitialPageNavigatorImpl create NavigationHandles to avoid this
bug entirely?

If not, I could be convinced that proceed_if_handle_not_found is ok, though I'd
like to keep the fix for interstitials closer to interstitial code if possible.

[clamy, 2016-09-23 15:48:33]

@creis, nasko: I've created another patch at
https://codereview.chromium.org/2364943002/ to create NavigationHandles for
interstitials.

[clamy, 2016-09-26 12:18:03]

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

[commit-bot: I haz the power, 2016-09-26 12:18:17]

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

[clamy, 2016-09-26 12:20:33]

Thanks!

https://codereview.chromium.org/2321543002/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2321543002/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:506: // renderer. Instead
it will persist until being picked up by the transfer
On 2016/09/22 22:44:55, nasko (slow) wrote:
> nit: Let's be precise. Either "renderer process" or
> "RenderFrame|RenderFrameHost".

Done.

https://codereview.chromium.org/2321543002/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:624: // PlzNavigate: the
final RenderFrameHost handling this navigation has been
On 2016/09/22 22:44:55, nasko (slow) wrote:
> nit: Empty line before the comment.

Done.

https://codereview.chromium.org/2321543002/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:626: //
NavigationRequest::OnresponseStarted.
On 2016/09/22 22:44:55, nasko (slow) wrote:
> nit: OnResponseStarted

Done.

https://codereview.chromium.org/2321543002/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:663: // "cross-site"
chrome:// frames, and that doesn't yet work cross- process.
On 2016/09/22 22:44:55, nasko (slow) wrote:
> nit: cross-process.

Done.

https://codereview.chromium.org/2321543002/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2321543002/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:246: // non-transfer code.
There may some issue in PlzNavigate, where
On 2016/09/22 22:44:55, nasko (slow) wrote:
> nit: "There may some issue in PlzNavigate" doesn't read very well. "There may
be
> some issues in PlzNavigate"?

Done.

https://codereview.chromium.org/2321543002/diff/180001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/2321543002/diff/180001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.cc:148: bool
proceed_if_handle_not_found) {
On 2016/09/23 05:46:55, Charlie Reis (slow) wrote:
> On 2016/09/22 22:44:55, nasko (slow) wrote:
> > On 2016/09/22 16:27:23, clamy wrote:
> > > On 2016/09/21 16:47:08, Charlie Reis (slow) wrote:
> > > > This needs some documentation somewhere.  I don't understand how this is
a
> > > > problem specific to data URLs-- why do they not have a NavigationHandle?

> It
> > > > seems like it might be possible for other cases that we don't currently
> have
> > > > test coverage for.  Can you clarify if it's actually limited to data
URLs?
> > > > 
> > > > (For example, there's also
> > > > RendererPreferences::browser_handles_all_top_level_requests, which
sounds
> > like
> > > > it could have a similar effect to Android sending all data URLs?)
> > > 
> > > Actually this is limited to interstitials: it's because with interstitials
> the
> > > NavigatorImpl is replaced by the interstitial page itself, so the code in
> > > NavigatorImpl::OnDidStartProvisionalLoad that creates the handle is not
> > > triggered. Now that I think about it some more, it's probably better to
just
> > > ensure that interstitials are always loaded as a data url (patchset 9)
> rather
> > > than this fix. Wdyt?
> > 
> > I am not a fan of this approach either. Putting all data URLs in the same
> bucket
> > as interstitials is a recipe for problems.
> 
> Let's avoid using the parameters that are meant for LoadBaseURLForDataURL,
which
> is an Android Webview feature with unfortunate behavior.
> 
> I'm happy to understand the root of the problem now, though: interstitials
that
> don't have NavigatorImpl, and thus don't have NavigationHandles.  Any chance
we
> could make InterstitialPageNavigatorImpl create NavigationHandles to avoid
this
> bug entirely?
> 
> If not, I could be convinced that proceed_if_handle_not_found is ok, though
I'd
> like to keep the fix for interstitials closer to interstitial code if
possible.

The fix for interstitials has been moved to
https://codereview.chromium.org/2364943002/.

https://codereview.chromium.org/2321543002/diff/180001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.cc:154: }
On 2016/09/21 17:10:24, mmenke wrote:
> nit:  Remove braces/

This code was moved in https://codereview.chromium.org/2364943002/.

https://codereview.chromium.org/2321543002/diff/180001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.cc:161: if
(!render_frame_host) {
On 2016/09/22 22:44:55, nasko (slow) wrote:
> I don't quite understand this. In the case of interstitials, why would we not
> find the RFH that is rendering it? We navigate it from the browser process and
> presumably because we don't have base/history URL associated with it, we send
it
> to the browser process to load on Android. Doesn't that resource request get
> tagged with the right process id/frame id?
> If we manage to get a RenderFrameHost (which I think we should), we can use
> InterstitialPage::FromRenderFrameHost to check if it is rendering an
> interstitial and let it proceed directly.
> 
> In general it also seems busted to allow Android code to know about
interstitial
> page loads. Can we give it a base URL such that we know it is interstitial vs
> any other data URL?

The fix for interstitials has been moved to
https://codereview.chromium.org/2364943002/.

https://codereview.chromium.org/2321543002/diff/180001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/2321543002/diff/180001/content/browser/loader...
content/browser/loader/resource_loader.cc:239:
on_transfer_complete_callback_.Reset();
On 2016/09/21 17:10:24, mmenke wrote:
> Suggest base::ResetAndReturn(on_transfer_complete_callback_).Run();
> 
> While we won't be marking the same request as transferring twice,
re-entrantly,
> just seems more robust.

Done.

[commit-bot: I haz the power, 2016-09-26 12:47:12]

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

[commit-bot: I haz the power, 2016-09-26 12:47:13]

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

[nasko, 2016-09-26 15:56:48]

Just a couple of nits on the rebase.

https://codereview.chromium.org/2321543002/diff/220001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2321543002/diff/220001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:672: // "cross-process"
chrome:// frames, and that doesn't yet work cross- process.
nit: I meant s/cross- process/cross-process/. Sorry, should've been more
explicit.

https://codereview.chromium.org/2321543002/diff/220001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2321543002/diff/220001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:255: // non-transfer code.
There may some issues in PlzNavigate, where
nit: "There may be some issues"?

[clamy, 2016-09-27 12:12:10]

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

[commit-bot: I haz the power, 2016-09-27 12:12:28]

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-09-27 12:44:32]

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

[commit-bot: I haz the power, 2016-09-27 12:44:34]

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

[clamy, 2016-09-27 13:43:24]

Thanks!

https://codereview.chromium.org/2321543002/diff/220001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2321543002/diff/220001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:672: // "cross-process"
chrome:// frames, and that doesn't yet work cross- process.
On 2016/09/26 15:56:48, nasko (slow) wrote:
> nit: I meant s/cross- process/cross-process/. Sorry, should've been more
> explicit.

Oh right. Apparently I had some issues properly fixing the comments that day
:-/.

https://codereview.chromium.org/2321543002/diff/220001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/2321543002/diff/220001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:255: // non-transfer code.
There may some issues in PlzNavigate, where
On 2016/09/26 15:56:48, nasko (slow) wrote:
> nit: "There may be some issues"?

Done.

[clamy, 2016-09-27 13:43:37]

The CQ bit was checked by clamy@chromium.org

[clamy, 2016-09-27 13:43:38]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
sky@chromium.org, rdevlin.cronin@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2321543002/#ps280001
(title: "Addressed nits")

[commit-bot: I haz the power, 2016-09-27 13:43:48]

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-09-27 15:23:45]

Description was changed from

==========
Merge CrossSiteResourceHandler and NavigationResourceThrottle

This CL merges CrossSiteResourceHandler and NavigationResourceThrottle.
It also removes the CrossSiteResourceRequest in favor of using
NavigationHandle. This allows WebContentsObserver::ReadyToCommit
navigation to be called in the non-PlzNavigate architecture.

A detailed description of the change can be found at
https://docs.google.com/a/chromium.org/document/d/1Ze5_RNvUQV4ltWvFU5MY3L_TRI....

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

to

==========
Merge CrossSiteResourceHandler and NavigationResourceThrottle

This CL merges CrossSiteResourceHandler and NavigationResourceThrottle.
It also removes the CrossSiteResourceRequest in favor of using
NavigationHandle. This allows WebContentsObserver::ReadyToCommit
navigation to be called in the non-PlzNavigate architecture.

A detailed description of the change can be found at
https://docs.google.com/a/chromium.org/document/d/1Ze5_RNvUQV4ltWvFU5MY3L_TRI....

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

[commit-bot: I haz the power, 2016-09-27 15:23:48]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-09-27 15:26:16]

Description was changed from

==========
Merge CrossSiteResourceHandler and NavigationResourceThrottle

This CL merges CrossSiteResourceHandler and NavigationResourceThrottle.
It also removes the CrossSiteResourceRequest in favor of using
NavigationHandle. This allows WebContentsObserver::ReadyToCommit
navigation to be called in the non-PlzNavigate architecture.

A detailed description of the change can be found at
https://docs.google.com/a/chromium.org/document/d/1Ze5_RNvUQV4ltWvFU5MY3L_TRI....

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

to

==========
Merge CrossSiteResourceHandler and NavigationResourceThrottle

This CL merges CrossSiteResourceHandler and NavigationResourceThrottle.
It also removes the CrossSiteResourceRequest in favor of using
NavigationHandle. This allows WebContentsObserver::ReadyToCommit
navigation to be called in the non-PlzNavigate architecture.

A detailed description of the change can be found at
https://docs.google.com/a/chromium.org/document/d/1Ze5_RNvUQV4ltWvFU5MY3L_TRI....

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

Committed: https://crrev.com/f40a0349f8575ccd544d6844dccd56119b87170c
Cr-Commit-Position: refs/heads/master@{#421216}
==========

[commit-bot: I haz the power, 2016-09-27 15:26:21]

Patchset 15 (id:??) landed as
https://crrev.com/f40a0349f8575ccd544d6844dccd56119b87170c
Cr-Commit-Position: refs/heads/master@{#421216}