Shortcut cross-process frame transfers to avoid pending NavigationEntries.

Shortcut cross-process transfers to avoid relying on OpenURLFromTab.

Some navigations transfer to a different process during the network
stack, such as server redirects or cross-site subframes in OOPIF-enabled
modes.  These should not depend on going through WebContentsDelegate's
OpenURLFromTab, since many WebContentsDelegate implementations do not
handle this case properly (e.g., panels, DevTools).

This is one step toward further shortcutting, such as not using pending
NavigationEntries or NavigationEntries at all for transfers.

BUG=495161, 568357
TEST=OOPIFs work in panels and DevTools; test landing separately.

Committed: https://crrev.com/29460279715f05dff407beb3ec3a517dae329f81
Cr-Commit-Position: refs/heads/master@{#365443}

[Charlie Reis, 2015-12-14 20:56:40]

Description was changed from

==========
Shortcut cross-process frame transfers to avoid pending NavigationEntries.

Subframes should not create pending NavigationEntries, because they could
interfere with existing pending navigations in the main frame or other
frames.  Instead, the transfer logic can skip most of the OpenURL and
NavigationController code, since we just need to tell the new frame to
issue the same request.

BUG=536145, 495161
TEST=See bug for repro steps.
==========

to

==========
Shortcut cross-process frame transfers to avoid pending NavigationEntries.

Subframes should not create pending NavigationEntries, because they could
interfere with existing pending navigations in the main frame or other
frames.  Instead, the transfer logic can skip most of the OpenURL and
NavigationController code, since we just need to tell the new frame to
issue the same request.

BUG=495161
TEST=See bug for repro steps.
==========

[Charlie Reis, 2015-12-15 21:49:58]

Description was changed from

==========
Shortcut cross-process frame transfers to avoid pending NavigationEntries.

Subframes should not create pending NavigationEntries, because they could
interfere with existing pending navigations in the main frame or other
frames.  Instead, the transfer logic can skip most of the OpenURL and
NavigationController code, since we just need to tell the new frame to
issue the same request.

BUG=495161
TEST=See bug for repro steps.
==========

to

==========
Shortcut cross-process transfers to avoid relying on OpenURLFromTab.

Some navigations transfer to a different process during the network
stack, such as server redirects or cross-site subframes in OOPIF-enabled
modes.  These should not depend on going through WebContentsDelegate's
OpenURLFromTab, since many WebContentsDelegate implementations do not
handle this case properly (e.g., panels, DevTools).

This is one step toward further shortcutting, such as not using pending
NavigationEntries or NavigationEntries at all for transfers.

BUG=495161
TEST=OOPIFs work in panels and DevTools; test landing separately.
==========

[Charlie Reis, 2015-12-15 21:52:08]

Description was changed from

==========
Shortcut cross-process transfers to avoid relying on OpenURLFromTab.

Some navigations transfer to a different process during the network
stack, such as server redirects or cross-site subframes in OOPIF-enabled
modes.  These should not depend on going through WebContentsDelegate's
OpenURLFromTab, since many WebContentsDelegate implementations do not
handle this case properly (e.g., panels, DevTools).

This is one step toward further shortcutting, such as not using pending
NavigationEntries or NavigationEntries at all for transfers.

BUG=495161
TEST=OOPIFs work in panels and DevTools; test landing separately.
==========

to

==========
Shortcut cross-process transfers to avoid relying on OpenURLFromTab.

Some navigations transfer to a different process during the network
stack, such as server redirects or cross-site subframes in OOPIF-enabled
modes.  These should not depend on going through WebContentsDelegate's
OpenURLFromTab, since many WebContentsDelegate implementations do not
handle this case properly (e.g., panels, DevTools).

This is one step toward further shortcutting, such as not using pending
NavigationEntries or NavigationEntries at all for transfers.

BUG=495161, 568357
TEST=OOPIFs work in panels and DevTools; test landing separately.
==========

[Charlie Reis, 2015-12-15 21:54:50]

creis@chromium.org changed reviewers:
+ alexmos@chromium.org, davidben@chromium.org

[Charlie Reis, 2015-12-15 21:54:51]

Alex, can you review?  (I think you have the context from your previous
OpenURLFromTab CLs, as well as https://crbug.com/568357.)

David, can you review prerender_contents.cc?  It's one of two
WebContentsDelegates that actually expect OpenURLForTab to be called during a
transfer, so that it can cancel the prerender.  I've added a different delegate
method to allow it to do so (and I've kept OpenURLForTab for the RequestOpenURL
cases).  The other place that needed to look at / cancel transfers was in
cross_site_transfer_browsertest.cc.

[alexmos, 2015-12-15 23:44:02]

Looks great, just a couple of questions/nits.

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

https://codereview.chromium.org/1519943002/diff/140001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:615: if
(render_frame_host->web_ui()) {
Just to help me understand: previously, it seems this would've used the root's
RFH without --site-per-process (looking at navigator_impl's GetRenderManager),
but now it will use the current RFH.  Is this ok?

https://codereview.chromium.org/1519943002/diff/140001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:648: bool user_gesture) {
This looks unused now.  Any reason to still keep it as an arg?

https://codereview.chromium.org/1519943002/diff/140001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:667: if
(render_frame_host->web_ui()) {
Is it ok that this block is now duplicated between RequestTransferURL and
RequestOpenURL?  Maybe we could combine them in a helper in a future CL, so that
they won't get out of sync in the future?

[davidben, 2015-12-15 23:48:08]

prerender lgtm

[Charlie Reis, 2015-12-16 00:26:10]

Patchset #9 (id:160001) has been deleted

[Charlie Reis, 2015-12-16 00:27:30]

Thanks!

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

https://codereview.chromium.org/1519943002/diff/140001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:615: if
(render_frame_host->web_ui()) {
On 2015/12/15 23:44:02, alexmos wrote:
> Just to help me understand: previously, it seems this would've used the root's
> RFH without --site-per-process (looking at navigator_impl's GetRenderManager),
> but now it will use the current RFH.  Is this ok?

Nice catch-- GetRenderManager looks pretty fragile.  I suppose we were counting
on not getting here for subframes in default mode before.  It's only used in
RequestOpenURL and RequestTransferURL, so maybe we can kill the main frame
behavior.  I'll spin up a separate CL for the remaining uses.  (We definitely
should be using the same frame here, not the main frame.)

However, it looks like I also accidentally switched from
current_render_frame_host (last committed) to render_frame_host (which sent us
the IPC, and might not be last committed).

That said, using current_render_frame_host for this seems wrong to me, and is
likely a bug introduced in https://codereview.chromium.org/1472703004.  We don't
want a previous RFH to be able to send us an IPC after we've switched to WebUI
and thus get treated like WebUI below.

So, I think the new code is doing the right thing now.

https://codereview.chromium.org/1519943002/diff/140001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:648: bool user_gesture) {
On 2015/12/15 23:44:02, alexmos wrote:
> This looks unused now.  Any reason to still keep it as an arg?

Good catch.  It didn't seem to make it through to LoadURLParams in the old code,
so we didn't seem to need it.  I've removed it, which cleans up an old comment
in RFHM as well.

https://codereview.chromium.org/1519943002/diff/140001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:667: if
(render_frame_host->web_ui()) {
On 2015/12/15 23:44:02, alexmos wrote:
> Is it ok that this block is now duplicated between RequestTransferURL and
> RequestOpenURL?  Maybe we could combine them in a helper in a future CL, so
that
> they won't get out of sync in the future?

Good idea.  I'll abstract them out separately.

[Charlie Reis, 2015-12-16 00:28:20]

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

[commit-bot: I haz the power, 2015-12-16 00:29:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1519943002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1519943002/180001

[alexmos, 2015-12-16 00:40:04]

LGTM

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

https://codereview.chromium.org/1519943002/diff/140001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:615: if
(render_frame_host->web_ui()) {
On 2015/12/16 00:27:30, Charlie Reis wrote:
> On 2015/12/15 23:44:02, alexmos wrote:
> > Just to help me understand: previously, it seems this would've used the
root's
> > RFH without --site-per-process (looking at navigator_impl's
GetRenderManager),
> > but now it will use the current RFH.  Is this ok?
> 
> Nice catch-- GetRenderManager looks pretty fragile.  I suppose we were
counting
> on not getting here for subframes in default mode before.  It's only used in
> RequestOpenURL and RequestTransferURL, so maybe we can kill the main frame
> behavior.  I'll spin up a separate CL for the remaining uses.  (We definitely
> should be using the same frame here, not the main frame.)
> 
> However, it looks like I also accidentally switched from
> current_render_frame_host (last committed) to render_frame_host (which sent us
> the IPC, and might not be last committed).
> 
> That said, using current_render_frame_host for this seems wrong to me, and is
> likely a bug introduced in https://codereview.chromium.org/1472703004.  We
don't
> want a previous RFH to be able to send us an IPC after we've switched to WebUI
> and thus get treated like WebUI below.
> 
> So, I think the new code is doing the right thing now.

Yes, this makes sense to me now, and killing the main frame behavior in a
followup CL sounds great.  Nice catch on the previous vs. current switch in
1472703004.

[Charlie Reis, 2015-12-16 00:43:44]

The CQ bit was unchecked by creis@chromium.org

[Charlie Reis, 2015-12-16 00:43:48]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2015-12-16 00:43:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1519943002/#ps180001
(title: "Remove dead param")

[commit-bot: I haz the power, 2015-12-16 00:45:04]

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

[commit-bot: I haz the power, 2015-12-16 02:40:51]

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

[commit-bot: I haz the power, 2015-12-16 02:40:52]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_android_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[Charlie Reis, 2015-12-16 04:07:28]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2015-12-16 04:08:54]

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

[commit-bot: I haz the power, 2015-12-16 04:38:29]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2015-12-16 04:39:09]

Description was changed from

==========
Shortcut cross-process transfers to avoid relying on OpenURLFromTab.

Some navigations transfer to a different process during the network
stack, such as server redirects or cross-site subframes in OOPIF-enabled
modes.  These should not depend on going through WebContentsDelegate's
OpenURLFromTab, since many WebContentsDelegate implementations do not
handle this case properly (e.g., panels, DevTools).

This is one step toward further shortcutting, such as not using pending
NavigationEntries or NavigationEntries at all for transfers.

BUG=495161, 568357
TEST=OOPIFs work in panels and DevTools; test landing separately.
==========

to

==========
Shortcut cross-process transfers to avoid relying on OpenURLFromTab.

Some navigations transfer to a different process during the network
stack, such as server redirects or cross-site subframes in OOPIF-enabled
modes.  These should not depend on going through WebContentsDelegate's
OpenURLFromTab, since many WebContentsDelegate implementations do not
handle this case properly (e.g., panels, DevTools).

This is one step toward further shortcutting, such as not using pending
NavigationEntries or NavigationEntries at all for transfers.

BUG=495161, 568357
TEST=OOPIFs work in panels and DevTools; test landing separately.

Committed: https://crrev.com/29460279715f05dff407beb3ec3a517dae329f81
Cr-Commit-Position: refs/heads/master@{#365443}
==========

[commit-bot: I haz the power, 2015-12-16 04:39:10]

Patchset 9 (id:??) landed as
https://crrev.com/29460279715f05dff407beb3ec3a517dae329f81
Cr-Commit-Position: refs/heads/master@{#365443}