Drop CreateChildFrame messages when swapping out.

Drop CreateChildFrame messages when swapping out.

There is a race condition in the current state of the code where in cross-process navigation we swap the existing RenderFrameHost with a new RenderFrameHost. If the existing host sends an IPC message to create a new child frame, it arrives on the IO thread, allocates a routing id based of the existing process (p1) and does a PostTask to the UI thread. If there is a CommitPending event either executing on the UI thread or in the task queue before the task posted from the IO thread, it will end up putting the existing RenderFrameHost in swapped out state (or waiting for swapped out). When the task to create a child frame is executed after that, it creates a new RenderFrameHost, but it uses the "current" process (p2), which is different than the process that sent the message (p1). This manifests sometimes as adding duplicate routing ids to RenderProcessHost and is in general really bad bug.

BUG=415059, 423691, 381990
Committed: https://crrev.com/dcdb02fab210ec5f7b8b560075ce96d0f48f344c
Cr-Commit-Position: refs/heads/master@{#299939}

[nasko, 2014-10-15 23:19:07]

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

[nasko, 2014-10-15 23:19:08]

Hey Charlie,
Can you review this patch for me? 

Hajime, feel free to patch this in and see if you still hit any of the DCHECKs
you've added. I hope they are not hit anymore : ).

Thanks,
Nasko

[Hajime Morrita, 2014-10-15 23:46:22]

Running tests, no crash so far.

[Charlie Reis, 2014-10-15 23:54:18]

Wow!  Great job connecting the rogue CreateChildFrame message to the duplicate
routing ID issue.

I blame FrameTree::AddFrame for taking in a routing ID and assuming it's for the
current process.

https://codereview.chromium.org/642813007/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/642813007/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:621: RenderFrameHostImpl*
new_frame = frame_tree_->AddFrame(
FrameTree::AddFrame is still a problematic API, in my opinion.  It assumes the
new_routing_id is from the current process, and that turns out to be easy to
violate.  We should make it take in both a process ID and the routing ID, so
that we can verify we're using the right process internally.

It's fine to do that in a follow-up CL if you'd prefer, since I think this is
the only call-site and there shouldn't be a way for the process to be different
while this RFH not in the default state.  Might be nice to get it done all at
once, though.

https://codereview.chromium.org/642813007/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/642813007/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager_unittest.cc:592:
TEST_F(RenderFrameHostManagerTest, DropCreateChildFrameWhileSwappedOut) {
Can you add a comment here mentioning the duplicate routing ID bug number?  That
provides some motivation for why this test is important.

https://codereview.chromium.org/642813007/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager_unittest.cc:596: //
Navigate first to one site and then cross-site.
nit: Drop "and then cross-site" since it's not done in this block.

https://codereview.chromium.org/642813007/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager_unittest.cc:605: // Create
one more frame in the same SiteInstance where initial_rfh
nit: Either add a blank line above this or remove the one on 599, depending on
whether this is all supposed to be one block or three.  (It seems odd to have
this joined with the {...} block.)

https://codereview.chromium.org/642813007/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager_unittest.cc:606: // exists
so that it doesn't get deleted on navigation to another
nit: it -> initial_rfh

(It's a little ambiguous at the moment.)

[nasko, 2014-10-16 17:08:36]

https://codereview.chromium.org/642813007/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/642813007/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:621: RenderFrameHostImpl*
new_frame = frame_tree_->AddFrame(
On 2014/10/15 23:54:18, Charlie Reis wrote:
> FrameTree::AddFrame is still a problematic API, in my opinion.  It assumes the
> new_routing_id is from the current process, and that turns out to be easy to
> violate.  We should make it take in both a process ID and the routing ID, so
> that we can verify we're using the right process internally.
> 
> It's fine to do that in a follow-up CL if you'd prefer, since I think this is
> the only call-site and there shouldn't be a way for the process to be
different
> while this RFH not in the default state.  Might be nice to get it done all at
> once, though.

No need for follow up CL. Let's do it right from the beginning.

https://codereview.chromium.org/642813007/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/642813007/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager_unittest.cc:592:
TEST_F(RenderFrameHostManagerTest, DropCreateChildFrameWhileSwappedOut) {
On 2014/10/15 23:54:18, Charlie Reis wrote:
> Can you add a comment here mentioning the duplicate routing ID bug number? 
That
> provides some motivation for why this test is important.

Done.

https://codereview.chromium.org/642813007/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager_unittest.cc:596: //
Navigate first to one site and then cross-site.
On 2014/10/15 23:54:18, Charlie Reis wrote:
> nit: Drop "and then cross-site" since it's not done in this block.

Done.

https://codereview.chromium.org/642813007/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager_unittest.cc:605: // Create
one more frame in the same SiteInstance where initial_rfh
On 2014/10/15 23:54:18, Charlie Reis wrote:
> nit: Either add a blank line above this or remove the one on 599, depending on
> whether this is all supposed to be one block or three.  (It seems odd to have
> this joined with the {...} block.)

Done.

https://codereview.chromium.org/642813007/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager_unittest.cc:606: // exists
so that it doesn't get deleted on navigation to another
On 2014/10/15 23:54:18, Charlie Reis wrote:
> nit: it -> initial_rfh
> 
> (It's a little ambiguous at the moment.)

Done.

[Charlie Reis, 2014-10-16 17:22:11]

Thanks!  LGTM with one additional CHECK.

https://codereview.chromium.org/642813007/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/642813007/diff/20001/content/browser/frame_ho...
content/browser/frame_host/frame_tree.cc:165: parent->AddChild(node.Pass(),
new_routing_id);
Can we add process_id to this call as well, just to CHECK that it's equal to
render_manager_.current_host()->GetProcess()->GetID()?  I don't want future
callers of that function to make the same mistake.

https://codereview.chromium.org/642813007/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree_unittest.cc (right):

https://codereview.chromium.org/642813007/diff/20001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_unittest.cc:244: // Simulate attaching a
frame from mismatche process id.
nit: mismatched

[nasko, 2014-10-16 17:41:31]

https://codereview.chromium.org/642813007/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/642813007/diff/20001/content/browser/frame_ho...
content/browser/frame_host/frame_tree.cc:165: parent->AddChild(node.Pass(),
new_routing_id);
On 2014/10/16 17:22:11, Charlie Reis wrote:
> Can we add process_id to this call as well, just to CHECK that it's equal to
> render_manager_.current_host()->GetProcess()->GetID()?  I don't want future
> callers of that function to make the same mistake.

Done.

https://codereview.chromium.org/642813007/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree_unittest.cc (right):

https://codereview.chromium.org/642813007/diff/20001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_unittest.cc:244: // Simulate attaching a
frame from mismatche process id.
On 2014/10/16 17:22:11, Charlie Reis wrote:
> nit: mismatched

Done.

[Charlie Reis, 2014-10-16 18:03:31]

Thanks-- still LGTM.

[nasko, 2014-10-16 18:04:37]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-10-16 18:07:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/642813007/40001

[commit-bot: I haz the power, 2014-10-16 18:33:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-10-16 18:35:11]

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