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

Review URL: https://codereview.chromium.org/799593004

Cr-Commit-Position: refs/heads/master@{#308219}

content/browser/frame_host/render_frame_host_manager.cc

@@ -67,7 +67,7 @@ RenderFrameHostManager::RenderFrameHostManager(
 
 RenderFrameHostManager::~RenderFrameHostManager() {
   if (pending_render_frame_host_)
-    CancelPending();
+    UnsetPendingRenderFrameHost();
 
   // We should always have a current RenderFrameHost except in some tests.
   SetRenderFrameHost(scoped_ptr<RenderFrameHostImpl>());
@@ -1591,6 +1591,11 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
 void RenderFrameHostManager::CancelPending() {
   TRACE_EVENT1("navigation", "RenderFrameHostManager::CancelPending",
                "FrameTreeNode id", frame_tree_node_->frame_tree_node_id());
+  DiscardUnusedFrame(UnsetPendingRenderFrameHost());
+}
+
+scoped_ptr<RenderFrameHostImpl>
+RenderFrameHostManager::UnsetPendingRenderFrameHost() {
   scoped_ptr<RenderFrameHostImpl> pending_render_frame_host =
       pending_render_frame_host_.Pass();
 
@@ -1601,10 +1606,10 @@ void RenderFrameHostManager::CancelPending() {
   // We no longer need to prevent the process from exiting.
   pending_render_frame_host->GetProcess()->RemovePendingView();
 
-  DiscardUnusedFrame(pending_render_frame_host.Pass());
-
   pending_web_ui_.reset();
   pending_and_current_web_ui_.reset();
+
+  return pending_render_frame_host.Pass();
 }
 
 scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::SetRenderFrameHost(

content/browser/frame_host/render_frame_host_manager.h

@@ -507,9 +507,13 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver {
   // swapped out.
   void ShutdownRenderFrameProxyHostsInSiteInstance(int32 site_instance_id);
 
-  // Helper method to terminate the pending RenderViewHost.
+  // Helper method to terminate the pending RenderFrameHost. The frame may be
+  // deleted immediately, or it may be kept around in hopes of later reuse.
   void CancelPending();
 
+  // Clears pending_render_frame_host_, returning it to the caller for disposal.
+  scoped_ptr<RenderFrameHostImpl> UnsetPendingRenderFrameHost();
+
   // Helper method to set the active RenderFrameHost. Returns the old
   // RenderFrameHost and updates counts.
   scoped_ptr<RenderFrameHostImpl> SetRenderFrameHost(

content/browser/frame_host/render_frame_host_manager_unittest.cc

@@ -902,7 +902,7 @@ TEST_F(RenderFrameHostManagerTest, Navigate) {
 
   // Commit.
   manager->DidNavigateFrame(host, true);
-  // Commit to SiteInstance should be delayed until RenderView commit.
+  // Commit to SiteInstance should be delayed until RenderFrame commit.
   EXPECT_TRUE(host == manager->current_frame_host());
   ASSERT_TRUE(host);
   EXPECT_FALSE(host->GetSiteInstance()->HasSite());
@@ -1524,7 +1524,7 @@ TEST_F(RenderFrameHostManagerTest, NoSwapOnGuestNavigations) {
 
   // Commit.
   manager->DidNavigateFrame(host, true);
-  // Commit to SiteInstance should be delayed until RenderView commit.
+  // Commit to SiteInstance should be delayed until RenderFrame commit.
   EXPECT_EQ(host, manager->current_frame_host());
   ASSERT_TRUE(host);
   EXPECT_TRUE(host->GetSiteInstance()->HasSite());
@@ -1818,7 +1818,7 @@ TEST_F(RenderFrameHostManagerTest,
 
     // Increment the number of active frames in the new SiteInstance, which will
     // cause the pending RFH to be swapped out instead of deleted.
-   pending_rfh->GetSiteInstance()->increment_active_frame_count();
+    pending_rfh->GetSiteInstance()->increment_active_frame_count();
 
     contents()->GetMainFrame()->OnMessageReceived(
         FrameHostMsg_BeforeUnload_ACK(0, false, now, now));
@@ -1827,4 +1827,85 @@ TEST_F(RenderFrameHostManagerTest,
   }
 }
 
+// Test that a pending RenderFrameHost in a non-root frame tree node is properly
+// deleted when the node is detached. Motivated by http://crbug.com/441357
+TEST_F(RenderFrameHostManagerTest, DetachPendingChild) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(switches::kSitePerProcess);
+
+  const GURL kUrl1("http://www.google.com/");
+  const GURL kUrl2("http://webkit.org/");
+
+  RenderFrameHostImpl* host = NULL;
+
+  contents()->NavigateAndCommit(kUrl1);
+  contents()->GetMainFrame()->OnCreateChildFrame(
+      contents()->GetMainFrame()->GetProcess()->GetNextRoutingID(),
+      std::string("frame_name"));
+  RenderFrameHostManager* manager =
+      contents()->GetFrameTree()->root()->child_at(0)->render_manager();
+
+  // 1) The first navigation. --------------------------
+  NavigationEntryImpl entry1(NULL /* instance */, -1 /* page_id */, kUrl1,
+                             Referrer(), base::string16() /* title */,
+                             ui::PAGE_TRANSITION_TYPED,
+                             false /* is_renderer_init */);
+  host = manager->Navigate(entry1);
+
+  // The RenderFrameHost created in Init will be reused.
+  EXPECT_TRUE(host == manager->current_frame_host());
+  EXPECT_FALSE(manager->pending_frame_host());
+
+  // Commit.
+  manager->DidNavigateFrame(host, true);
+  // Commit to SiteInstance should be delayed until RenderFrame commit.
+  EXPECT_TRUE(host == manager->current_frame_host());
+  ASSERT_TRUE(host);
+  EXPECT_TRUE(host->GetSiteInstance()->HasSite());
+
+  // 2) Cross-site navigate to next site. --------------
+  NavigationEntryImpl entry2(NULL /* instance */, -1 /* page_id */, kUrl2,
+                             Referrer(kUrl1, blink::WebReferrerPolicyDefault),
+                             base::string16() /* title */,
+                             ui::PAGE_TRANSITION_LINK,
+                             false /* is_renderer_init */);
+  host = manager->Navigate(entry2);
+
+  // A new RenderFrameHost should be created.
+  EXPECT_TRUE(manager->pending_frame_host());
+  ASSERT_EQ(host, manager->pending_frame_host());
+  ASSERT_NE(manager->current_frame_host(), manager->pending_frame_host());
+  EXPECT_FALSE(contents()->cross_navigation_pending())
+      << "There should be no top-level pending navigation.";
+
+  RenderFrameHostDeletedObserver delete_watcher(manager->pending_frame_host());
+  EXPECT_FALSE(delete_watcher.deleted());
+
+  // Extend the lifetime of the child frame's SiteInstance, pretending
+  // that there is another reference to it.
+  scoped_refptr<SiteInstanceImpl> site_instance =
+      manager->pending_frame_host()->GetSiteInstance();
+  EXPECT_TRUE(site_instance->HasSite());
+  EXPECT_NE(site_instance, contents()->GetSiteInstance());
+  EXPECT_EQ(1U, site_instance->active_frame_count());
+  site_instance->increment_active_frame_count();
+  EXPECT_EQ(2U, site_instance->active_frame_count());
+
+  // Now detach the child FrameTreeNode. This should kill the pending host.
+  manager->current_frame_host()->OnMessageReceived(
+      FrameHostMsg_Detach(manager->current_frame_host()->GetRoutingID()));
+
+  EXPECT_TRUE(delete_watcher.deleted());
+
+  EXPECT_EQ(1U, site_instance->active_frame_count());
+  site_instance->decrement_active_frame_count();
+
+#if 0
+  // TODO(nick): Currently a proxy to the removed frame lingers in the parent.
+  // Enable this assert below once the proxies to the subframe are correctly
+  // cleaned up after detach.
+  ASSERT_TRUE(site_instance->HasOneRef())
+      << "This SiteInstance should be destroyable now.";
+#endif
+}
+
 }  // namespace content