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

content/browser/frame_host/render_frame_host_manager_unittest.cc

Index: content/browser/frame_host/render_frame_host_manager_unittest.cc
diff --git a/content/browser/frame_host/render_frame_host_manager_unittest.cc b/content/browser/frame_host/render_frame_host_manager_unittest.cc
index 69eb043931967a088a1e8e44b4c4ccf782ba506b..808a9e1cd64e8b0b1e5b16dd0c411e378bca8447 100644
--- a/content/browser/frame_host/render_frame_host_manager_unittest.cc
+++ b/content/browser/frame_host/render_frame_host_manager_unittest.cc
@@ -1794,7 +1794,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));
@@ -1803,4 +1803,84 @@ TEST_F(RenderFrameHostManagerTest,
   }
 }
 
+// Test that a pending RenderFrameHost in a non-root frame tree node is properly
+// deleted when the node is detached.

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

nit: Add a link to the bug it is testing for.

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

Done.

+TEST_F(RenderFrameHostManagerTest, DetachPendingChild) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(switches::kSitePerProcess);
+
+  const GURL kUrl1("http://www.google.com/");

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

nit: Google?! Why not chromium.org : )))

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

These URLs are just copied from the test above.

+  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);
+  // Commit to SiteInstance should be delayed until RenderView commit.

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

RenderFrame instead of RenderView?

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

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

[nasko, 2014/12/12 21:54:49]

Acknowledged.

+  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(1, site_instance->active_frame_count());
+  site_instance->increment_active_frame_count();
+  EXPECT_EQ(2, site_instance->active_frame_count());
+
+  // Now detach the child FrameTreeNode. This should kill the pending host.
+  manager->current_frame_host()->OnMessageReceived(FrameHostMsg_Detach(0));

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

Let's be more correct - s/0/current_frame_host()->GetRoutingID/

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

Done.

+
+  EXPECT_TRUE(delete_watcher.deleted());
+
+  EXPECT_EQ(1, 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