Don't use pending NavigationEntries for navigation transfers.

Don't use pending NavigationEntries for navigation transfers.

Cross-process transfers for navigations that are about to commit
had been using NavigationControllerImpl::LoadURLWithParams (as
an artifact of previously going through OpenURL).  Most of that
code is unnecessary, in particular the fact that it creates a
new pending NavigationEntry.  That's problematic for subframe
transfers, where we should not affect any existing pending
NavigationEntry (e.g., for a slow main frame navigation).

This CL shortcuts the transfer by creating a NavigationEntry
without making it the pending one.  A future CL can eliminate
the entry entirely by validating and passing the parameters
to the right RenderFrameHost directly.

This also makes it possible to do cross-process navigations
in subframes of the initial blank page, fixing a crash.

BUG=495161, 584739
TEST=Create OOPIF on initial blank page of a tab.
TEST=Cross-process subframe navigation during slow main frame navigation.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/9bc97a7b5405403cd9878556162a6234ef4381bf
Cr-Commit-Position: refs/heads/master@{#384009}

[Charlie Reis, 2016-02-09 04:40:48]

Description was changed from

==========
Don't use pending NavigationEntries for transfer navigations.

BUG=495161
TEST=Cross-process subframe navigation during slow main frame navigation.
==========

to

==========
Don't use pending NavigationEntries for transfer navigations.

BUG=495161
TEST=Cross-process subframe navigation during slow main frame navigation.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-03-18 23:32:21]

Description was changed from

==========
Don't use pending NavigationEntries for transfer navigations.

BUG=495161
TEST=Cross-process subframe navigation during slow main frame navigation.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't use pending NavigationEntries for navigation transfers.

Cross-process transfers for navigations that are about to commit
had been using NavigationControllerImpl::LoadURLWithParams (as
an artifact of previously going through OpenURL).  Most of that
code is unnecessary, in particular the fact that it creates a
new pending NavigationEntry.  That's problematic for subframe
transfers, where we should not affect any existing pending
NavigationEntry (e.g., for a slow main frame navigation).

This CL shortcuts the transfer by creating a NavigationEntry
without making it the pending one.  A future CL can eliminate
the entry entirely by validating and passing the parameters
to the right RenderFrameHost directly.

BUG=495161
TEST=Cross-process subframe navigation during slow main frame navigation.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-03-21 16:50:35]

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

[Charlie Reis, 2016-03-21 16:50:35]

Alex, can you take a look?  We can go a step further by avoiding creating a
NavigationEntry entirely, but this is a nice step in that direction by cutting
out LoadURLWithParams (and thus setting the pending NavigationEntry on
NavigationController).

I tried writing a browsertest for it, but I think the change in behavior is only
visible between OnCrossSiteResponse and the commit, and I don't think we have a
way to wait for that.  (All pending entries get cleared at commit, so we can't
check that the main frame's pending entry is still around after commit.)

Anyway, the existing transfer tests should verify that we don't regress behavior
here, and this is mostly just a refactoring of that behavior.

[Charlie Reis, 2016-03-21 18:18:26]

Oh, and for reference, most of the new code is a stripped down version of
LoadURLWithParams, in case you want to compare.  I hope to replace it with the
new approach.

[alexmos, 2016-03-21 21:41:26]

Looks good -- just a couple of clarification questions from comparing the old
and new paths.

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

https://codereview.chromium.org/1683593002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:740:
controller_->LoadURLWithParams(load_url_params);
The old path used to return early if the target URL returned true from
HandleDebugURL and kEnableGpuBenchmarking wasn't set -- just want to make sure
that it's ok not to do this anymore?  (Perhaps we can't ever get a transfer for
a debug URL?)

https://codereview.chromium.org/1683593002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1683593002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:745: // with a subframe entry for
our destination.
Just curious, the old path through LoadURLWithParams doesn't seem to handle this
case.  Is this something you're fixing along the way?

https://codereview.chromium.org/1683593002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:749: GURL(url::kAboutBlankURL),
referrer, page_transition,
Should this pass referrer_to_use?  Same for two other uses below.

https://codereview.chromium.org/1683593002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:771:
entry->set_transferred_global_request_id(transferred_global_request_id);
Does entry->SetIsOverridingUserAgent() also need to be set?  The
LoadURLWithParams params had the default override_user_agent value of
UA_OVERRIDE_INHERIT, which means that this used to be set to the override value
from GetLastCommittedEntry().  I'm not sure where this is used though.

[Charlie Reis, 2016-03-30 04:21:01]

Description was changed from

==========
Don't use pending NavigationEntries for navigation transfers.

Cross-process transfers for navigations that are about to commit
had been using NavigationControllerImpl::LoadURLWithParams (as
an artifact of previously going through OpenURL).  Most of that
code is unnecessary, in particular the fact that it creates a
new pending NavigationEntry.  That's problematic for subframe
transfers, where we should not affect any existing pending
NavigationEntry (e.g., for a slow main frame navigation).

This CL shortcuts the transfer by creating a NavigationEntry
without making it the pending one.  A future CL can eliminate
the entry entirely by validating and passing the parameters
to the right RenderFrameHost directly.

BUG=495161
TEST=Cross-process subframe navigation during slow main frame navigation.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't use pending NavigationEntries for navigation transfers.

Cross-process transfers for navigations that are about to commit
had been using NavigationControllerImpl::LoadURLWithParams (as
an artifact of previously going through OpenURL).  Most of that
code is unnecessary, in particular the fact that it creates a
new pending NavigationEntry.  That's problematic for subframe
transfers, where we should not affect any existing pending
NavigationEntry (e.g., for a slow main frame navigation).

This CL shortcuts the transfer by creating a NavigationEntry
without making it the pending one.  A future CL can eliminate
the entry entirely by validating and passing the parameters
to the right RenderFrameHost directly.

This also makes it possible to do cross-process navigations
in subframes of the initial blank page, fixing a crash.

BUG=495161, 584739
TEST=Cross-process subframe navigation during slow main frame navigation.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-03-30 04:21:42]

Description was changed from

==========
Don't use pending NavigationEntries for navigation transfers.

Cross-process transfers for navigations that are about to commit
had been using NavigationControllerImpl::LoadURLWithParams (as
an artifact of previously going through OpenURL).  Most of that
code is unnecessary, in particular the fact that it creates a
new pending NavigationEntry.  That's problematic for subframe
transfers, where we should not affect any existing pending
NavigationEntry (e.g., for a slow main frame navigation).

This CL shortcuts the transfer by creating a NavigationEntry
without making it the pending one.  A future CL can eliminate
the entry entirely by validating and passing the parameters
to the right RenderFrameHost directly.

This also makes it possible to do cross-process navigations
in subframes of the initial blank page, fixing a crash.

BUG=495161, 584739
TEST=Cross-process subframe navigation during slow main frame navigation.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't use pending NavigationEntries for navigation transfers.

Cross-process transfers for navigations that are about to commit
had been using NavigationControllerImpl::LoadURLWithParams (as
an artifact of previously going through OpenURL).  Most of that
code is unnecessary, in particular the fact that it creates a
new pending NavigationEntry.  That's problematic for subframe
transfers, where we should not affect any existing pending
NavigationEntry (e.g., for a slow main frame navigation).

This CL shortcuts the transfer by creating a NavigationEntry
without making it the pending one.  A future CL can eliminate
the entry entirely by validating and passing the parameters
to the right RenderFrameHost directly.

This also makes it possible to do cross-process navigations
in subframes of the initial blank page, fixing a crash.

BUG=495161, 584739
TEST=Create OOPIF on initial blank page of a tab.
TEST=Cross-process subframe navigation during slow main frame navigation.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-03-30 15:26:08]

Thanks, and sorry for the delay!  Fixes up.

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

https://codereview.chromium.org/1683593002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:740:
controller_->LoadURLWithParams(load_url_params);
On 2016/03/21 21:41:26, alexmos wrote:
> The old path used to return early if the target URL returned true from
> HandleDebugURL and kEnableGpuBenchmarking wasn't set -- just want to make sure
> that it's ok not to do this anymore?  (Perhaps we can't ever get a transfer
for
> a debug URL?)

Right, we can't get a transfer for a debug URL.

https://codereview.chromium.org/1683593002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1683593002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:745: // with a subframe entry for
our destination.
On 2016/03/21 21:41:26, alexmos wrote:
> Just curious, the old path through LoadURLWithParams doesn't seem to handle
this
> case.  Is this something you're fixing along the way?

Yes, now we can create OOPIFs in the initial blank page.  And good point-- I
added a test for this part.

https://codereview.chromium.org/1683593002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:749: GURL(url::kAboutBlankURL),
referrer, page_transition,
On 2016/03/21 21:41:26, alexmos wrote:
> Should this pass referrer_to_use?  Same for two other uses below.

Good catch!

https://codereview.chromium.org/1683593002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:771:
entry->set_transferred_global_request_id(transferred_global_request_id);
On 2016/03/21 21:41:26, alexmos wrote:
> Does entry->SetIsOverridingUserAgent() also need to be set?  The
> LoadURLWithParams params had the default override_user_agent value of
> UA_OVERRIDE_INHERIT, which means that this used to be set to the override
value
> from GetLastCommittedEntry().  I'm not sure where this is used though.

Ah, nice find.  I must have assumed there was no easy way to set that value or
that it didn't apply here, but you're right.  Fixed.

https://codereview.chromium.org/1683593002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1683593002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:516:
WaitForLoadStopWithoutSuccessCheck(new_shell->web_contents());
Before the change in this CL, doing this crashes in LoadURLWithParams on the
GetLastCommittedEntry()->Clone() line (at least in OOPIF-enabled modes).  We
need the wait line to let the navigation get to the transfer and thus crash.

In fact, I realized this bug surfaces as crashes in
content::NavigationEntryImpl::CloneAndReplace, which is
https://crbug.com/584739.  So this should be a useful crash fix after all!

[alexmos, 2016-03-30 16:50:41]

LGTM

https://codereview.chromium.org/1683593002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/1683593002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:516:
WaitForLoadStopWithoutSuccessCheck(new_shell->web_contents());
On 2016/03/30 15:26:08, Charlie Reis wrote:
> Before the change in this CL, doing this crashes in LoadURLWithParams on the
> GetLastCommittedEntry()->Clone() line (at least in OOPIF-enabled modes).  We
> need the wait line to let the navigation get to the transfer and thus crash.
> 
> In fact, I realized this bug surfaces as crashes in
> content::NavigationEntryImpl::CloneAndReplace, which is
> https://crbug.com/584739.  So this should be a useful crash fix after all!

Nice!

[Charlie Reis, 2016-03-30 16:54:11]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2016-03-30 16:54:34]

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

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

Description was changed from

==========
Don't use pending NavigationEntries for navigation transfers.

Cross-process transfers for navigations that are about to commit
had been using NavigationControllerImpl::LoadURLWithParams (as
an artifact of previously going through OpenURL).  Most of that
code is unnecessary, in particular the fact that it creates a
new pending NavigationEntry.  That's problematic for subframe
transfers, where we should not affect any existing pending
NavigationEntry (e.g., for a slow main frame navigation).

This CL shortcuts the transfer by creating a NavigationEntry
without making it the pending one.  A future CL can eliminate
the entry entirely by validating and passing the parameters
to the right RenderFrameHost directly.

This also makes it possible to do cross-process navigations
in subframes of the initial blank page, fixing a crash.

BUG=495161, 584739
TEST=Create OOPIF on initial blank page of a tab.
TEST=Cross-process subframe navigation during slow main frame navigation.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't use pending NavigationEntries for navigation transfers.

Cross-process transfers for navigations that are about to commit
had been using NavigationControllerImpl::LoadURLWithParams (as
an artifact of previously going through OpenURL).  Most of that
code is unnecessary, in particular the fact that it creates a
new pending NavigationEntry.  That's problematic for subframe
transfers, where we should not affect any existing pending
NavigationEntry (e.g., for a slow main frame navigation).

This CL shortcuts the transfer by creating a NavigationEntry
without making it the pending one.  A future CL can eliminate
the entry entirely by validating and passing the parameters
to the right RenderFrameHost directly.

This also makes it possible to do cross-process navigations
in subframes of the initial blank page, fixing a crash.

BUG=495161, 584739
TEST=Create OOPIF on initial blank page of a tab.
TEST=Cross-process subframe navigation during slow main frame navigation.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

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

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-30 17:01:35]

Description was changed from

==========
Don't use pending NavigationEntries for navigation transfers.

Cross-process transfers for navigations that are about to commit
had been using NavigationControllerImpl::LoadURLWithParams (as
an artifact of previously going through OpenURL).  Most of that
code is unnecessary, in particular the fact that it creates a
new pending NavigationEntry.  That's problematic for subframe
transfers, where we should not affect any existing pending
NavigationEntry (e.g., for a slow main frame navigation).

This CL shortcuts the transfer by creating a NavigationEntry
without making it the pending one.  A future CL can eliminate
the entry entirely by validating and passing the parameters
to the right RenderFrameHost directly.

This also makes it possible to do cross-process navigations
in subframes of the initial blank page, fixing a crash.

BUG=495161, 584739
TEST=Create OOPIF on initial blank page of a tab.
TEST=Cross-process subframe navigation during slow main frame navigation.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't use pending NavigationEntries for navigation transfers.

Cross-process transfers for navigations that are about to commit
had been using NavigationControllerImpl::LoadURLWithParams (as
an artifact of previously going through OpenURL).  Most of that
code is unnecessary, in particular the fact that it creates a
new pending NavigationEntry.  That's problematic for subframe
transfers, where we should not affect any existing pending
NavigationEntry (e.g., for a slow main frame navigation).

This CL shortcuts the transfer by creating a NavigationEntry
without making it the pending one.  A future CL can eliminate
the entry entirely by validating and passing the parameters
to the right RenderFrameHost directly.

This also makes it possible to do cross-process navigations
in subframes of the initial blank page, fixing a crash.

BUG=495161, 584739
TEST=Create OOPIF on initial blank page of a tab.
TEST=Cross-process subframe navigation during slow main frame navigation.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/9bc97a7b5405403cd9878556162a6234ef4381bf
Cr-Commit-Position: refs/heads/master@{#384009}
==========

[commit-bot: I haz the power, 2016-03-30 17:01:36]

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

[Charlie Reis, 2016-04-05 17:28:58]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1861773002/ by creis@chromium.org.

The reason for reverting is: This is causing crashes in
https://crbug.com/600743, as well as a problem with interstitial pages in
https://crbug.com/600046.  I'll look into the problems and then reland this..