Don't swap out the old RenderFrameHost until the new one commits.

Don't swap out the old RenderFrameHost until the new one commits.

This is a major cleanup of cross-process navigation that delays running
the old render frame's unload handler until the new render frame has
committed.  As a result, there is no period of time when swapped out 
RenderFrameHosts are visible.

Most cross-process navigations are no longer paused in the network 
stack (unless they require a transfer), and the WasSwappedOut IPC 
message is removed.  The STATE_WAITING_FOR_COMMIT and 
STATE_WAITING_FOR_UNLOAD_ACK states in RenderViewHostImpl 
can also be removed.

Overview doc with navigation timeline diagrams:
https://docs.google.com/document/d/1nLUxn4nV5g0HVGf-n34us5kQ0-erE-7050lzpMSxa4Q/edit?usp=sharing

BUG=402020
TEST=Unload handlers still run on cross-process navigations.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290828

[Charlie Reis, 2014-08-18 23:27:53]

@nasko, @clamy: This is a big cleanup after making SwapOut asynchronous, which
should simplify things quite a bit.  Please see the linked doc for an overview
and updated timeline diagrams.  Thanks!

https://codereview.chromium.org/464593003/diff/200001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/464593003/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:702: set_swapped_out(true);
Note: This was moved from RFHM::CommitPending, and only applies to the subframe
case.

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

https://codereview.chromium.org/464593003/diff/200001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1412:
loader->CompleteTransfer();
Note: We now only use this for transfers that end up not needing a new process. 
This means we can "complete" the transfer without changing any of the request's
metadata.

[clamy, 2014-08-19 14:59:06]

Thanks! It looks a lot better and easier to understand that way. After a first
pass through the code, I have one question (see below).

Also I am not completely sure of what happens in the case of a transferring
request that happens to come back in the same process.

https://chromiumcodereview.appspot.com/464593003/diff/200001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/464593003/diff/200001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:369:
cross_site_transferring_request_.reset();
Why do you reset cross_site_transferring_request_ here?

[Charlie Reis, 2014-08-19 16:26:04]

On 2014/08/19 14:59:06, clamy wrote:
> Thanks! It looks a lot better and easier to understand that way. After a first
> pass through the code, I have one question (see below).
> 
> Also I am not completely sure of what happens in the case of a transferring
> request that happens to come back in the same process.

Good question-- that's a bit subtle.  We can only detect that a transfer will
come back to the originating process after selecting the SiteInstance and
RenderFrameHost in RenderFrameHostManager::Navigate.  I've added a check in
NavigatorImpl::NavigateToEntry to detect it.

When that occurs, we tell the IO thread to resume the paused response (to its
original RFH) instead of asking the RenderFrameHost to navigate to the same URL
a second time.  This skips the normal transfer logic in
RDH::UpdateRequestForTransfer, since it's a no-op in this case.

Does that help?

https://chromiumcodereview.appspot.com/464593003/diff/200001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/464593003/diff/200001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:369:
cross_site_transferring_request_.reset();
On 2014/08/19 14:59:05, clamy wrote:
> Why do you reset cross_site_transferring_request_ here?

It's only needed during the RequestTransferURL call, after which the transfer
has completed.  I've added a comment to that effect.

I considered finding a way to not store it as state at all, but the lifetime of
that object is a bit subtle and this CL seemed big enough as is.

[nasko, 2014-08-19 16:45:23]

This is an awesome cleanup! Thanks for doing it!
Just a few questions/nits.

https://codereview.chromium.org/464593003/diff/200001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (left):

https://codereview.chromium.org/464593003/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:884:
TEST_F(RenderFrameHostManagerTest, NavigateWithEarlyReNavigation) {
Just to make sure I understand it right, we don't need this test as there is no
longer state changing before we hit the commit time in the new process and any
new incoming navigation will just cancel the old one. Is that right?

https://codereview.chromium.org/464593003/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1701:
EXPECT_EQ(RenderViewHostImpl::STATE_WAITING_FOR_UNLOAD_ACK,
I'd keep the state expectation, just change the value, so we ensure we don't
introduce a bug somewhere that moves us to a different state.

https://codereview.chromium.org/464593003/diff/200001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/464593003/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1372: //
received (after commit).
nit: I read "(after commit)" as a test case variation and expecting to see a
test case for "before commit". In reality, the ack will always come after the
commit. I'd word this part a bit more strongly.

https://codereview.chromium.org/464593003/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1447:
rfh1->OnSwappedOut(false);
nit: So what happens if we get a swap out ack before the commit? Should we have
a negative test case to ensure we don't get into a confused state?

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

https://codereview.chromium.org/464593003/diff/200001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.cc:94:
CHECK(CommandLine::ForCurrentProcess()->HasSwitch(switches::kSitePerProcess));
Thanks for adding that!

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

https://codereview.chromium.org/464593003/diff/200001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1412:
loader->CompleteTransfer();
On 2014/08/18 23:27:53, Charlie Reis wrote:
> Note: We now only use this for transfers that end up not needing a new
process. 
> This means we can "complete" the transfer without changing any of the
request's
> metadata.

Converting to this doesn't remove the cross_site_handler() on the
ResourceRequestInfo object. Is this ok?
Also, if we complete here, does that work fine for the transition navigations?
They rely on the cross-site resource handler to pause the navigation through a
defer/resume.

[Charlie Reis, 2014-08-19 21:20:32]

Thanks!  New patch uploaded.

https://codereview.chromium.org/464593003/diff/200001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (left):

https://codereview.chromium.org/464593003/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:884:
TEST_F(RenderFrameHostManagerTest, NavigateWithEarlyReNavigation) {
On 2014/08/19 16:45:22, nasko wrote:
> Just to make sure I understand it right, we don't need this test as there is
no
> longer state changing before we hit the commit time in the new process and any
> new incoming navigation will just cancel the old one. Is that right?

Correct.  This was exercising a bug that happened if we navigated between
SwapOut and Commit, and that state no longer exists.

https://codereview.chromium.org/464593003/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1701:
EXPECT_EQ(RenderViewHostImpl::STATE_WAITING_FOR_UNLOAD_ACK,
On 2014/08/19 16:45:22, nasko wrote:
> I'd keep the state expectation, just change the value, so we ensure we don't
> introduce a bug somewhere that moves us to a different state.

Done.

https://codereview.chromium.org/464593003/diff/200001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/464593003/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1372: //
received (after commit).
On 2014/08/19 16:45:23, nasko wrote:
> nit: I read "(after commit)" as a test case variation and expecting to see a
> test case for "before commit". In reality, the ack will always come after the
> commit. I'd word this part a bit more strongly.

Done.

https://codereview.chromium.org/464593003/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1447:
rfh1->OnSwappedOut(false);
On 2014/08/19 16:45:22, nasko wrote:
> nit: So what happens if we get a swap out ack before the commit? Should we
have
> a negative test case to ensure we don't get into a confused state?

Good idea.  I added a test line to verify that we ignore the unexpected ack (see
RenderViewHostImpl::OnSwappedOut).

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

https://codereview.chromium.org/464593003/diff/200001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1412:
loader->CompleteTransfer();
On 2014/08/19 16:45:23, nasko wrote:
> On 2014/08/18 23:27:53, Charlie Reis wrote:
> > Note: We now only use this for transfers that end up not needing a new
> process. 
> > This means we can "complete" the transfer without changing any of the
> request's
> > metadata.
> 
> Converting to this doesn't remove the cross_site_handler() on the
> ResourceRequestInfo object. Is this ok?

I think the sole difference is that we set loader->is_transferring_ to false
before calling cross_site_handler()->ResumeResponse().  That should still remove
it from the info object.

> Also, if we complete here, does that work fine for the transition navigations?
> They rely on the cross-site resource handler to pause the navigation through a
> defer/resume.

I *think* their pause and resume logic should continue to work, since they have
a similar set of operations as transfers.  I admit that I don't have full
knowledge of what they're relying on CSRH for, but I didn't see any test
failures from the change.

[nasko, 2014-08-19 21:39:01]

LGTM

[Charlie Reis, 2014-08-19 23:39:29]

Great.  @clamy, I've rebased to pick up your PlzNavigate CL, which didn't
conflict much.  If it looks good, feel free to CQ it.

[clamy, 2014-08-20 11:19:25]

Thanks! LGTM.

[clamy, 2014-08-20 11:19:32]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2014-08-20 11:20:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/creis@chromium.org/464593003/260001

[commit-bot: I haz the power, 2014-08-20 13:18:00]

Committed patchset #13 (260001) as 290828

[dglazkov, 2014-08-20 19:51:18]

On 2014/08/20 at 13:18:00, commit-bot wrote:
> Committed patchset #13 (260001) as 290828

Did this change cause
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...

[Charlie Reis, 2014-08-20 19:53:19]

On 2014/08/20 19:51:18, dglazkov wrote:
> On 2014/08/20 at 13:18:00, commit-bot wrote:
> > Committed patchset #13 (260001) as 290828
> 
> Did this change cause
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...

Yes, but we've already disabled the test in http://crbug.com/405582.