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 972 matching lines @@
   contents()->GetController().GoBack();
   contents()->GetMainFrame()->PrepareForCommit();
   const NavigationEntry* entry1 = contents()->GetController().GetPendingEntry();
   rfh1->SendNavigate(entry1->GetPageID(), entry1->GetURL());
 
   // Disown the opener from rfh2.
   rfh2->DidDisownOpener();
   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());
 
   // 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 373 matching lines @@
 
   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"), SandboxFlags::NONE);
+  RenderFrameHostManager* root_manager =
+      contents()->GetFrameTree()->root()->render_manager();
   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 = GetFrameHostForNavigation(manager, entry1);
 
@@ skipping 12 matching lines @@
   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_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.";
 
   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());
+  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(
       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();
+  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