Don't crash while detaching a pending child frame under --site-per-process.

Don't crash while detaching a pending child frame under --site-per-process.

Fixes a null pointer dereference in RenderFrameProxyHost::RenderFrameProxyHost,
when a RenderFrameProxyHost is created as a side effect of destroying the
FrameTreeNode and RenderFrameHostManager. The crash occurs because of the
operation below:

  if (!frame_tree_node_->IsMainFrame() &&
      frame_tree_node_->parent()
              ->render_manager()
              ->current_frame_host()
              ->GetSiteInstance() == site_instance) {

When a FrameTreeNode is being detached from the tree, it is not the main
frame (because it is not equal to the root), but it also does not have a
parent (because, for reasons explained in FrameTreeNode::RemoveChild, it
is trimmed from the tree before destruction). So the parent() call above
returns NULL, resulting in great misfortune.

Add a test that failed without the fix.

BUG=441357
Committed: https://crrev.com/807e4955fa017715970d456d17bcc3a0730c72a0
Cr-Commit-Position: refs/heads/master@{#308219}

[ncarter (slow), 2014-12-11 23:24:26]

nick@chromium.org changed reviewers:
+ nasko@chromium.org

[ncarter (slow), 2014-12-11 23:24:27]

Hi Nasko, please review.

[nasko, 2014-12-11 23:49:54]

Looking good, a few nits here and there.

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

https://codereview.chromium.org/799593004/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:70:
pending_render_frame_host_->GetProcess()->RemovePendingView();
Don't we need the RenderViewDevToolsAgentHost::OnCancelPendingNavigation call
from CancelPending?

https://codereview.chromium.org/799593004/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:71:
pending_render_frame_host_.reset();
Does this need to be explicitly reset? If yes, it will be good to comment why.

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

https://codereview.chromium.org/799593004/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1807: //
deleted when the node is detached.
nit: Add a link to the bug it is testing for.

https://codereview.chromium.org/799593004/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1811: const
GURL kUrl1("http://www.google.com/");
nit: Google?! Why not chromium.org : )))

https://codereview.chromium.org/799593004/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1836: // Commit
to SiteInstance should be delayed until RenderView commit.
RenderFrame instead of RenderView?

https://codereview.chromium.org/799593004/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1870:
manager->current_frame_host()->OnMessageReceived(FrameHostMsg_Detach(0));
Let's be more correct - s/0/current_frame_host()->GetRoutingID/

[ncarter (slow), 2014-12-12 20:21:06]

PTAL. I significantly changed the approach for the fix, and had to rebase to fix
a conflict. So beware if you look at the incremental diffs, you'll see lots of
lines from 746993003

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

https://codereview.chromium.org/799593004/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:70:
pending_render_frame_host_->GetProcess()->RemovePendingView();
On 2014/12/11 23:49:53, nasko wrote:
> Don't we need the RenderViewDevToolsAgentHost::OnCancelPendingNavigation call
> from CancelPending?

I thought about this. OnCancelPendingNavigation would switch the DevTool's
tracking back to the current view, which should then be deleted a couple lines
below. So it seemed like an unnecessary step.

https://codereview.chromium.org/799593004/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:71:
pending_render_frame_host_.reset();
On 2014/12/11 23:49:54, nasko wrote:
> Does this need to be explicitly reset? If yes, it will be good to comment why.

I just wanted to preserve the existing relative destruction order, since it's
observer-visible.

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

https://codereview.chromium.org/799593004/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1807: //
deleted when the node is detached.
On 2014/12/11 23:49:54, nasko wrote:
> nit: Add a link to the bug it is testing for.

Done.

https://codereview.chromium.org/799593004/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1811: const
GURL kUrl1("http://www.google.com/");
On 2014/12/11 23:49:54, nasko wrote:
> nit: Google?! Why not http://chromium.org : )))

These URLs are just copied from the test above.

https://codereview.chromium.org/799593004/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1836: // Commit
to SiteInstance should be delayed until RenderView commit.
On 2014/12/11 23:49:54, nasko wrote:
> RenderFrame instead of RenderView?

Fixed here (and in the other places in this file, from which this line was
copied)

https://codereview.chromium.org/799593004/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1870:
manager->current_frame_host()->OnMessageReceived(FrameHostMsg_Detach(0));
On 2014/12/11 23:49:54, nasko wrote:
> Let's be more correct - s/0/current_frame_host()->GetRoutingID/

Done.

[ncarter (slow), 2014-12-12 20:22:41]

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

https://codereview.chromium.org/799593004/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:70:
pending_render_frame_host_->GetProcess()->RemovePendingView();
On 2014/12/12 20:21:06, ncarter wrote:
> On 2014/12/11 23:49:53, nasko wrote:
> > Don't we need the RenderViewDevToolsAgentHost::OnCancelPendingNavigation
call
> > from CancelPending?
> 
> I thought about this. OnCancelPendingNavigation would switch the DevTool's
> tracking back to the current view, which should then be deleted a couple lines
> below. So it seemed like an unnecessary step.

Whoops -- I published the wrong comment. Meant to say I put it back in (out of
conservatism), which entailed extracting a helper function out of CancelPending.

[nasko, 2014-12-12 21:54:50]

Just one comment, to avoid extraneous moving of code around.

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

https://codereview.chromium.org/799593004/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1836: // Commit
to SiteInstance should be delayed until RenderView commit.
On 2014/12/12 20:21:06, ncarter wrote:
> On 2014/12/11 23:49:54, nasko wrote:
> > RenderFrame instead of RenderView?
> 
> Fixed here (and in the other places in this file, from which this line was
> copied)

Acknowledged.

https://codereview.chromium.org/799593004/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (left):

https://codereview.chromium.org/799593004/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:567: void
RenderFrameHostManager::DiscardUnusedFrame(
In a parallel CL (https://codereview.chromium.org/701953006/) carlosk@ is
working on, he is using this method in another place. It will be nice to keep
this method around instead of inlining it back into CancelPending (from which it
was factored out).

[ncarter (slow), 2014-12-12 22:33:24]

https://codereview.chromium.org/799593004/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (left):

https://codereview.chromium.org/799593004/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:567: void
RenderFrameHostManager::DiscardUnusedFrame(
On 2014/12/12 21:54:49, nasko wrote:
> In a parallel CL (https://codereview.chromium.org/701953006/) carlosk@ is
> working on, he is using this method in another place. It will be nice to keep
> this method around instead of inlining it back into CancelPending (from which
it
> was factored out).

Done. I inlined it in an attempt to control the proliferation of "delete frame"
private methods. Thanks for catching this.

Incidentally, the CL you linked to would introduce a new instance of this bug
(it adds a new call to DiscardUnusedFrame in the dtor). I'll add a comment
there.

[nasko, 2014-12-12 22:36:17]

LGTM

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

https://codereview.chromium.org/799593004/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1594:
DiscardUnusedFrame(UnsetPendingRenderFrameHost());
Looks actually quite nice! Thanks!

[ncarter (slow), 2014-12-13 00:49:59]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2014-12-13 00:52:16]

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

[commit-bot: I haz the power, 2014-12-13 01:53:57]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-12-13 01:55:18]

Patchset 7 (id:??) landed as
https://crrev.com/807e4955fa017715970d456d17bcc3a0730c72a0
Cr-Commit-Position: refs/heads/master@{#308219}