Fix the RenderFrameProxyHost mop-up logic in RenderFrameHostManager

content/browser/frame_host/render_frame_host_manager_unittest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/histogram_tester.h"
 #include "base/time/time.h"
 #include "content/browser/frame_host/cross_site_transferring_request.h"
@@ skipping 383 matching lines @@
   // PlzNavigate: returns the speculative RenderFrameHost.
   RenderFrameHostImpl* GetPendingFrameHost(
       RenderFrameHostManager* manager) {
     if (base::CommandLine::ForCurrentProcess()->HasSwitch(
         switches::kEnableBrowserSideNavigation)) {
       return manager->speculative_render_frame_host_.get();
     }
     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_;
 };
 
 // Tests that when you navigate from a chrome:// url to another page, and
 // then do that same thing in another tab, that the two resulting pages have
 // different SiteInstances, BrowsingInstances, and RenderProcessHosts. This is
 // a regression test for bug 9364.
 TEST_F(RenderFrameHostManagerTest, NewTabPageProcesses) {
   set_should_create_webui(true);
@@ skipping 542 matching lines @@
   RenderFrameHostImpl* host = GetFrameHostForNavigation(manager, entry);
 
   // We commit the pending RenderFrameHost immediately because the previous
   // RenderFrameHost was not live.  We test a case where it is live in
   // WebUIInNewTab.
   EXPECT_TRUE(host);
   EXPECT_NE(initial_rfh, host);
   EXPECT_EQ(host, manager->current_frame_host());
   EXPECT_FALSE(GetPendingFrameHost(manager));
 
-  // It's important that the site instance get set on the Web UI page as soon
+  // It's important that the SiteInstance get set on the Web UI page as soon
   // as the navigation starts, rather than lazily after it commits, so we don't
   // try to re-use the SiteInstance/process for non Web UI things that may
   // get loaded in between.
   EXPECT_TRUE(host->GetSiteInstance()->HasSite());
   EXPECT_EQ(kUrl, host->GetSiteInstance()->GetSiteURL());
 
   // The Web UI is committed immediately because the RenderViewHost has not been
   // used yet. UpdateStateForNavigate() took the short cut path.
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableBrowserSideNavigation)) {
@@ skipping 417 matching lines @@
 }
 
 // Test that we clean up swapped out RenderViewHosts when a process hosting
 // those associated RenderViews crashes. http://crbug.com/258993
 TEST_F(RenderFrameHostManagerTest, CleanUpSwappedOutRVHOnProcessCrash) {
   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());
 
   // Make sure the new opener RVH is considered live.
   opener1_manager->current_host()->CreateRenderView(
       base::string16(), -1, MSG_ROUTING_NONE, -1, false);
   EXPECT_TRUE(opener1_manager->current_host()->IsRenderViewLive());
   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
 // granted WebUI bindings even if an unprivileged swapped out RenderViewHost
 // is in the same process (http://crbug.com/79918).
 TEST_F(RenderFrameHostManagerTest, EnableWebUIWithSwappedOutOpener) {
   set_should_create_webui(true);
   const GURL kSettingsUrl("chrome://chrome/settings");
   const GURL kPluginUrl("chrome://plugins");
 
@@ skipping 359 matching lines @@
     pending_rfh->GetSiteInstance()->increment_active_frame_count();
 
     contents()->GetMainFrame()->OnMessageReceived(
         FrameHostMsg_BeforeUnload_ACK(0, false, now, now));
     EXPECT_FALSE(contents()->cross_navigation_pending());
     EXPECT_FALSE(rfh_deleted_observer.deleted());
   }
 }
 
 // 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
+// deleted when the node is detached. Motivated by http://crbug.com/441357 and
+// http://crbug.com/444955.
 TEST_F(RenderFrameHostManagerTest, DetachPendingChild) {

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

Note that in the latest patch set I've changed this test significantly. I
realized that in making it test bug 444955 it no longer was a valid test for
441357 -- I solved this by adding two frames in the same site_instance, and
detaching them both. That causes us to exercise both the "is last active frame"
case and the "is not last active frame" case.

   base::CommandLine::ForCurrentProcess()->AppendSwitch(
       switches::kSitePerProcess);
 
+  const GURL kUrlA("http://www.google.com/");
+  const GURL kUrlB("http://webkit.org/");
+
+  // Create a page with two child frames.
+  contents()->NavigateAndCommit(kUrlA);
+  contents()->GetMainFrame()->OnCreateChildFrame(
+      contents()->GetMainFrame()->GetProcess()->GetNextRoutingID(),
+      std::string("frame_name"), SandboxFlags::NONE);
+  contents()->GetMainFrame()->OnCreateChildFrame(
+      contents()->GetMainFrame()->GetProcess()->GetNextRoutingID(),
+      std::string("frame_name"), SandboxFlags::NONE);
+  RenderFrameHostManager* root_manager =
+      contents()->GetFrameTree()->root()->render_manager();
+  RenderFrameHostManager* iframe1 =
+      contents()->GetFrameTree()->root()->child_at(0)->render_manager();
+  RenderFrameHostManager* iframe2 =
+      contents()->GetFrameTree()->root()->child_at(1)->render_manager();
+
+  // 1) The first navigation.
+  NavigationEntryImpl entryA(NULL /* instance */, -1 /* page_id */, kUrlA,
+                             Referrer(), base::string16() /* title */,
+                             ui::PAGE_TRANSITION_TYPED,
+                             false /* is_renderer_init */);
+  RenderFrameHostImpl* host1 = GetFrameHostForNavigation(iframe1, entryA);
+
+  // The RenderFrameHost created in Init will be reused.
+  EXPECT_TRUE(host1 == iframe1->current_frame_host());
+  EXPECT_FALSE(GetPendingFrameHost(iframe1));
+
+  // Commit.
+  iframe1->DidNavigateFrame(host1, true);
+  // Commit to SiteInstance should be delayed until RenderFrame commit.
+  EXPECT_TRUE(host1 == iframe1->current_frame_host());
+  ASSERT_TRUE(host1);
+  EXPECT_TRUE(host1->GetSiteInstance()->HasSite());
+
+  // 2) Cross-site navigate both frames to next site.
+  NavigationEntryImpl entryB(NULL /* instance */, -1 /* page_id */, kUrlB,
+                             Referrer(kUrlA, blink::WebReferrerPolicyDefault),
+                             base::string16() /* title */,
+                             ui::PAGE_TRANSITION_LINK,
+                             false /* is_renderer_init */);
+  host1 = GetFrameHostForNavigation(iframe1, entryB);
+  RenderFrameHostImpl* host2 = GetFrameHostForNavigation(iframe2, entryB);
+
+  // A new, pending RenderFrameHost should be created in each FrameTreeNode.
+  EXPECT_TRUE(GetPendingFrameHost(iframe1));
+  EXPECT_TRUE(GetPendingFrameHost(iframe2));
+  ASSERT_EQ(host1, GetPendingFrameHost(iframe1));
+  ASSERT_EQ(host2, GetPendingFrameHost(iframe2));
+  ASSERT_TRUE(RenderFrameHostImpl::IsRFHStateActive(
+      GetPendingFrameHost(iframe1)->rfh_state()));
+  ASSERT_TRUE(RenderFrameHostImpl::IsRFHStateActive(
+      GetPendingFrameHost(iframe2)->rfh_state()));
+  ASSERT_NE(GetPendingFrameHost(iframe1), GetPendingFrameHost(iframe2));
+  ASSERT_EQ(GetPendingFrameHost(iframe1)->GetSiteInstance(),
+            GetPendingFrameHost(iframe2)->GetSiteInstance());
+  ASSERT_NE(iframe1->current_frame_host(), GetPendingFrameHost(iframe1));
+  ASSERT_NE(iframe2->current_frame_host(), GetPendingFrameHost(iframe2));
+  EXPECT_FALSE(contents()->cross_navigation_pending())
+      << "There should be no top-level pending navigation.";
+
+  RenderFrameHostDeletedObserver delete_watcher1(GetPendingFrameHost(iframe1));
+  RenderFrameHostDeletedObserver delete_watcher2(GetPendingFrameHost(iframe2));
+  EXPECT_FALSE(delete_watcher1.deleted());
+  EXPECT_FALSE(delete_watcher2.deleted());
+
+  // Keep the SiteInstance alive for testing.
+  scoped_refptr<SiteInstanceImpl> site_instance =
+      GetPendingFrameHost(iframe1)->GetSiteInstance();
+  EXPECT_TRUE(site_instance->HasSite());
+  EXPECT_NE(site_instance, contents()->GetSiteInstance());
+  EXPECT_EQ(2U, site_instance->active_frame_count());
+
+  // Proxies should exist.
+  EXPECT_NE(nullptr,
+            root_manager->GetRenderFrameProxyHost(site_instance.get()));
+  EXPECT_NE(nullptr,
+            iframe1->GetRenderFrameProxyHost(site_instance.get()));
+  EXPECT_NE(nullptr,
+            iframe2->GetRenderFrameProxyHost(site_instance.get()));
+
+  // Detach the first child FrameTreeNode. This should kill the pending host but
+  // not yet destroy proxies in |site_instance| since the other child remains.
+  iframe1->current_frame_host()->OnMessageReceived(
+      FrameHostMsg_Detach(iframe1->current_frame_host()->GetRoutingID()));
+  iframe1 = NULL;  // Was just destroyed.
+
+  EXPECT_TRUE(delete_watcher1.deleted());
+  EXPECT_FALSE(delete_watcher2.deleted());
+  EXPECT_EQ(1U, site_instance->active_frame_count());
+
+  // Proxies should still exist.
+  EXPECT_NE(nullptr,
+            root_manager->GetRenderFrameProxyHost(site_instance.get()));
+  EXPECT_NE(nullptr,
+            iframe2->GetRenderFrameProxyHost(site_instance.get()));
+
+  // Detach the second child FrameTreeNode. This should trigger cleanup of
+  // RenderFrameProxyHosts in |site_instance|.
+  iframe2->current_frame_host()->OnMessageReceived(
+      FrameHostMsg_Detach(iframe2->current_frame_host()->GetRoutingID()));
+  iframe2 = NULL;  // Was just destroyed.
+
+  EXPECT_TRUE(delete_watcher1.deleted());
+  EXPECT_TRUE(delete_watcher2.deleted());
+
+  EXPECT_EQ(0U, site_instance->active_frame_count());
+  EXPECT_EQ(nullptr,
+            root_manager->GetRenderFrameProxyHost(site_instance.get()))
+      << "Proxies should have been cleaned up";
+  EXPECT_TRUE(site_instance->HasOneRef())
+      << "This SiteInstance should be destroyable now.";
+}
+
+// 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. Motivated by http://crbug.com/473714.
+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/");
 
-  RenderFrameHostImpl* host = NULL;
+  // |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());
 
-  contents()->NavigateAndCommit(kUrl1);
-  contents()->GetMainFrame()->OnCreateChildFrame(
-      contents()->GetMainFrame()->GetProcess()->GetNextRoutingID(),
+  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* manager =
+  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);
 
-  // 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 = GetFrameHostForNavigation(manager, entry1);
+  // A proxy to the iframe should now exist in the SiteInstance of the main
+  // frames.
+  ASSERT_NE(cross_site->GetSiteInstance(), contents1->GetSiteInstance());
+  ASSERT_NE(nullptr,
+            iframe->GetRenderFrameProxyHost(contents1->GetSiteInstance()));
+  ASSERT_NE(nullptr,
+            iframe->GetRenderFrameProxyHost(contents2->GetSiteInstance()));
 
-  // The RenderFrameHost created in Init will be reused.
-  EXPECT_TRUE(host == manager->current_frame_host());
-  EXPECT_FALSE(GetPendingFrameHost(manager));
-
-  // 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 = GetFrameHostForNavigation(manager, entry2);
-
-  // A new RenderFrameHost should be created.
-  EXPECT_TRUE(GetPendingFrameHost(manager));
-  ASSERT_EQ(host, GetPendingFrameHost(manager));
-  ASSERT_NE(manager->current_frame_host(), GetPendingFrameHost(manager));
-  EXPECT_FALSE(contents()->cross_navigation_pending())
-      << "There should be no top-level pending navigation.";
-
-  RenderFrameHostDeletedObserver delete_watcher(GetPendingFrameHost(manager));
-  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 =
-      GetPendingFrameHost(manager)->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. http://crbug.com/444955.
-  ASSERT_TRUE(site_instance->HasOneRef())
-      << "This SiteInstance should be destroyable now.";
-#endif
+  // 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