Fix the RenderFrameProxyHost mop-up logic in RenderFrameHostManager

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 5114a2b03dbf0700ede3beb254e4a93500c54ac6..d86b7abc19e2c58dd357db15f5707502d802a310 100644
--- a/content/browser/frame_host/render_frame_host_manager_unittest.cc
+++ b/content/browser/frame_host/render_frame_host_manager_unittest.cc
@@ -401,6 +401,24 @@ class RenderFrameHostManagerTest : public RenderViewHostImplTestHarness {
     return manager->pending_frame_host();
   }
 
+  void CrashFrame(TestRenderFrameHost* frame) {
+    // TODO(nick): Standardize how we simulate crashes in content unittests. The
+    // test method |set_render_view_created| should probably be eliminated
+    // altogether, and maybe there should be a CrashProcess() method on
+    // MockRenderProcessHost.
+    ASSERT_TRUE(frame->IsRenderFrameLive());
+    frame->OnMessageReceived(FrameHostMsg_RenderProcessGone(
+        0, base::TERMINATION_STATUS_PROCESS_CRASHED, -1));
+    RenderProcessHost::RendererClosedDetails details(
+        base::TERMINATION_STATUS_PROCESS_CRASHED, 0);
+    NotificationService::current()->Notify(
+        NOTIFICATION_RENDERER_PROCESS_CLOSED,
+        Source<RenderProcessHost>(frame->GetProcess()),
+        Details<RenderProcessHost::RendererClosedDetails>(&details));
+    frame->GetRenderViewHost()->set_render_view_created(false);
+    ASSERT_FALSE(frame->IsRenderFrameLive());
+  }
+
  private:
   RenderFrameHostManagerTestWebUIControllerFactory factory_;
 };
@@ -1393,19 +1411,19 @@ TEST_F(RenderFrameHostManagerTest, DisownOpenerAfterNavigation) {
   EXPECT_FALSE(contents()->HasOpener());
 }
 
-// Test that we clean up swapped out RenderViewHosts when a process hosting
-// those associated RenderViews crashes. http://crbug.com/258993
-TEST_F(RenderFrameHostManagerTest, CleanUpSwappedOutRVHOnProcessCrash) {
+// Test that we clean up swapped out RenderFrameHosts when a process hosting

[Charlie Reis, 2015/04/03 19:55:09]

The test itself still seems to be about swapped out RenderViewHosts, rather than
using GetRenderFrameProxyHost.  Does it make more sense to update the test or
leave the comment as it was?

[ncarter (slow), 2015/04/03 22:03:33]

Keen observation. I restored the comments and test name because I don't want to
disturb the test more than I need to now. This test will need to be updated
again when swappedout goes away.

+// those associated RenderFrame crashes. http://crbug.com/258993

[Charlie Reis, 2015/04/03 19:55:09]

nit: RenderFrames

[ncarter (slow), 2015/04/03 22:03:33]

Done.

+TEST_F(RenderFrameHostManagerTest, CleanUpSwappedOutRFHOnProcessCrash) {
   const GURL kUrl1("http://www.google.com/");
   const GURL kUrl2("http://www.chromium.org/");
 
   // Navigate to an initial URL.
   contents()->NavigateAndCommit(kUrl1);
-  TestRenderViewHost* rvh1 = test_rvh();
+  TestRenderFrameHost* rfh1 = contents()->GetMainFrame();
 
   // Create a new tab as an opener for the main tab.
   scoped_ptr<TestWebContents> opener1(
-      TestWebContents::Create(browser_context(), rvh1->GetSiteInstance()));
+      TestWebContents::Create(browser_context(), rfh1->GetSiteInstance()));
   RenderFrameHostManager* opener1_manager =
       opener1->GetRenderManagerForTesting();
   contents()->SetOpener(opener1.get());
@@ -1417,43 +1435,33 @@ TEST_F(RenderFrameHostManagerTest, CleanUpSwappedOutRVHOnProcessCrash) {
   EXPECT_TRUE(opener1_manager->current_frame_host()->IsRenderFrameLive());
 
   // Use a cross-process navigation in the opener to swap out the old RVH.
-  EXPECT_FALSE(opener1_manager->GetSwappedOutRenderViewHost(
-      rvh1->GetSiteInstance()));
+  EXPECT_FALSE(
+      opener1_manager->GetSwappedOutRenderViewHost(rfh1->GetSiteInstance()));
   opener1->NavigateAndCommit(kUrl2);
-  EXPECT_TRUE(opener1_manager->GetSwappedOutRenderViewHost(
-      rvh1->GetSiteInstance()));
+  EXPECT_TRUE(
+      opener1_manager->GetSwappedOutRenderViewHost(rfh1->GetSiteInstance()));
 
   // Fake a process crash.
-  RenderProcessHost::RendererClosedDetails details(
-      base::TERMINATION_STATUS_PROCESS_CRASHED,
-      0);
-  // TODO(nasko): Investigate whether this test can be made more realistic by
-  // not faking the notification and just doing the RenderProcessGone. This
-  // should also get rid of faking |set_render_view_created()| call below.
-  NotificationService::current()->Notify(
-      NOTIFICATION_RENDERER_PROCESS_CLOSED,
-      Source<RenderProcessHost>(rvh1->GetProcess()),
-      Details<RenderProcessHost::RendererClosedDetails>(&details));
-  rvh1->set_render_view_created(false);
+  CrashFrame(rfh1);
 
   // Ensure that the RenderFrameProxyHost stays around and the RenderFrameProxy
   // is deleted.
   RenderFrameProxyHost* render_frame_proxy_host =
-      opener1_manager->GetRenderFrameProxyHost(rvh1->GetSiteInstance());
+      opener1_manager->GetRenderFrameProxyHost(rfh1->GetSiteInstance());
   EXPECT_TRUE(render_frame_proxy_host);
   EXPECT_FALSE(render_frame_proxy_host->is_render_frame_proxy_live());
 
   // Expect the swapped out RVH to exist.
-  EXPECT_TRUE(opener1_manager->GetSwappedOutRenderViewHost(
-                  rvh1->GetSiteInstance()));
+  EXPECT_TRUE(
+      opener1_manager->GetSwappedOutRenderViewHost(rfh1->GetSiteInstance()));
 
   // Reload the initial tab. This should recreate the opener's swapped out RVH
   // in the original SiteInstance.
   contents()->GetController().Reload(true);
   contents()->GetMainFrame()->PrepareForCommit();
   EXPECT_EQ(opener1_manager->GetSwappedOutRenderViewHost(
-                rvh1->GetSiteInstance())->GetRoutingID(),
-            test_rvh()->opener_route_id());
+                                 rfh1->GetSiteInstance())->GetRoutingID(),
+            contents()->GetMainFrame()->GetRenderViewHost()->opener_route_id());
 }
 
 // Test that RenderViewHosts created for WebUI navigations are properly
@@ -1847,6 +1855,8 @@ TEST_F(RenderFrameHostManagerTest, DetachPendingChild) {
   contents()->GetMainFrame()->OnCreateChildFrame(
       contents()->GetMainFrame()->GetProcess()->GetNextRoutingID(),
       std::string("frame_name"), SandboxFlags::NONE);
+  RenderFrameHostManager* root_manager =
+      contents()->GetFrameTree()->root()->render_manager();
   RenderFrameHostManager* manager =
       contents()->GetFrameTree()->root()->child_at(0)->render_manager();
 
@@ -1879,6 +1889,8 @@ TEST_F(RenderFrameHostManagerTest, DetachPendingChild) {
   // A new RenderFrameHost should be created.
   EXPECT_TRUE(GetPendingFrameHost(manager));
   ASSERT_EQ(host, GetPendingFrameHost(manager));
+  ASSERT_TRUE(RenderFrameHostImpl::IsRFHStateActive(
+      GetPendingFrameHost(manager)->rfh_state()));
   ASSERT_NE(manager->current_frame_host(), GetPendingFrameHost(manager));
   EXPECT_FALSE(contents()->cross_navigation_pending())
       << "There should be no top-level pending navigation.";
@@ -1893,8 +1905,8 @@ TEST_F(RenderFrameHostManagerTest, DetachPendingChild) {
   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());
+  EXPECT_NE(nullptr,
+            root_manager->GetRenderFrameProxyHost(site_instance.get()));
 
   // Now detach the child FrameTreeNode. This should kill the pending host.
   manager->current_frame_host()->OnMessageReceived(
@@ -1902,16 +1914,75 @@ TEST_F(RenderFrameHostManagerTest, DetachPendingChild) {
 
   EXPECT_TRUE(delete_watcher.deleted());
 
-  EXPECT_EQ(1U, site_instance->active_frame_count());
-  site_instance->decrement_active_frame_count();
+  EXPECT_EQ(0U, site_instance->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. http://crbug.com/444955.
-  ASSERT_TRUE(site_instance->HasOneRef())
+  EXPECT_TRUE(site_instance->HasOneRef())
       << "This SiteInstance should be destroyable now.";
-#endif
+
+  EXPECT_EQ(nullptr,
+            root_manager->GetRenderFrameProxyHost(site_instance.get()));
+}
+
+// Two tabs in the same process crash. The first tab is reloaded, and the second
+// tab navigates away without reloading. The second tab's navigation shouldn't
+// mess with the first tab's content.

[Charlie Reis, 2015/04/03 19:55:09]

Can you add https://crbug.com/473714 to the comment?

[ncarter (slow), 2015/04/03 22:03:33]

Done.

+TEST_F(RenderFrameHostManagerTest, TwoTabsCrashOneReloadsOneLeaves) {
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kSitePerProcess);
+
+  const GURL kUrl1("http://www.google.com/");
+  const GURL kUrl2("http://webkit.org/");
+  const GURL kUrl3("http://whatwg.org/");
+
+  // |contents1| and |contents2| navigate to the same page and then crash.
+  TestWebContents* contents1 = contents();
+  scoped_ptr<TestWebContents> contents2(
+      TestWebContents::Create(browser_context(), contents1->GetSiteInstance()));
+  contents1->NavigateAndCommit(kUrl1);
+  contents2->NavigateAndCommit(kUrl1);
+  CrashFrame(contents1->GetMainFrame());
+  CrashFrame(contents2->GetMainFrame());
+
+  ASSERT_EQ(contents1->GetSiteInstance(), contents2->GetSiteInstance());
+
+  // Reload |contents1|.
+  contents1->NavigateAndCommit(kUrl1);
+  ASSERT_TRUE(contents1->GetMainFrame()->IsRenderFrameLive());
+  ASSERT_FALSE(contents2->GetMainFrame()->IsRenderFrameLive());
+  ASSERT_EQ(contents1->GetSiteInstance(), contents2->GetSiteInstance());
+
+  // |contents1| creates an out of process iframe.
+  contents1->GetMainFrame()->OnCreateChildFrame(
+      contents1->GetMainFrame()->GetProcess()->GetNextRoutingID(),
+      std::string("frame_name"), SandboxFlags::NONE);
+  RenderFrameHostManager* iframe =
+      contents()->GetFrameTree()->root()->child_at(0)->render_manager();
+  NavigationEntryImpl entry(NULL /* instance */, -1 /* page_id */, kUrl2,
+                            Referrer(kUrl1, blink::WebReferrerPolicyDefault),
+                            base::string16() /* title */,
+                            ui::PAGE_TRANSITION_LINK,
+                            false /* is_renderer_init */);
+  RenderFrameHostImpl* cross_site = GetFrameHostForNavigation(iframe, entry);
+  iframe->DidNavigateFrame(cross_site, true);
+
+  // A proxy to the iframe should now exist in the site instance of the main

[Charlie Reis, 2015/04/03 19:55:09]

s/site instance/SiteInstance/

[ncarter (slow), 2015/04/03 22:03:33]

Done.

+  // frames.
+  ASSERT_NE(cross_site->GetSiteInstance(), contents1->GetSiteInstance());
+  ASSERT_NE(nullptr,
+            iframe->GetRenderFrameProxyHost(contents1->GetSiteInstance()));
+  ASSERT_NE(nullptr,
+            iframe->GetRenderFrameProxyHost(contents2->GetSiteInstance()));
+
+  // Navigate |contents2| away from the sad tab (and thus away from the
+  // SiteInstance of |contents1|). This should not destroy the proxies needed by
+  // |contents1| -- that was http://crbug.com/473714.
+  ASSERT_FALSE(contents2->GetMainFrame()->IsRenderFrameLive());
+  contents2->NavigateAndCommit(kUrl3);
+  ASSERT_TRUE(contents2->GetMainFrame()->IsRenderFrameLive());
+  ASSERT_NE(nullptr,
+            iframe->GetRenderFrameProxyHost(contents1->GetSiteInstance()));
+  ASSERT_EQ(nullptr,
+            iframe->GetRenderFrameProxyHost(contents2->GetSiteInstance()));
 }
 
 }  // namespace content