Don't swap out the old RenderFrameHost until the new one commits.

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 eac088a9e7d94491d6f055654b1240125556fde4..c026ab82643c7fd4fc5d3d1498a6a5d471d7cab0 100644
--- a/content/browser/frame_host/render_frame_host_manager_unittest.cc
+++ b/content/browser/frame_host/render_frame_host_manager_unittest.cc
@@ -269,20 +269,8 @@ class RenderFrameHostManagerTest
     factory_.set_should_create_webui(should_create_webui);
   }
 
-  void StartCrossSiteTransition(TestWebContents* contents) {
-    std::vector<GURL> url_chain;
-    contents->GetRenderManagerForTesting()->OnCrossSiteResponse(
-        contents->GetRenderManagerForTesting()->pending_frame_host(),
-        GlobalRequestID(0, 0), scoped_ptr<CrossSiteTransferringRequest>(),
-        url_chain, Referrer(), PAGE_TRANSITION_TYPED, false);
-    EXPECT_TRUE(contents->cross_navigation_pending());
-    RenderViewHostImpl* rvh = contents->GetRenderViewHost();
-    EXPECT_EQ(RenderViewHostImpl::STATE_WAITING_FOR_UNLOAD_ACK,
-              rvh->rvh_state());
-  }
-
   void NavigateActiveAndCommit(const GURL& url) {
-    // Note: we navigate the active RenderViewHost because previous navigations
+    // Note: we navigate the active RenderFrameHost because previous navigations
     // won't have committed yet, so NavigateAndCommit does the wrong thing
     // for us.
     controller().LoadURL(url, Referrer(), PAGE_TRANSITION_LINK, std::string());
@@ -299,25 +287,36 @@ class RenderFrameHostManagerTest
     int32 max_page_id = contents()->GetMaxPageIDForSiteInstance(
         active_rvh()->GetSiteInstance());
 
-    // Simulate the response coming from the pending renderer.
-    if (old_rvh != active_rvh())
-      StartCrossSiteTransition(contents());
-
-    // Simulate the SwapOut_ACK that fires if you commit a cross-site
-    // navigation.
-    if (old_rvh != active_rvh()) {
-      old_rvh->OnSwappedOut(false);
-      EXPECT_EQ(RenderViewHostImpl::STATE_WAITING_FOR_COMMIT,
-                old_rvh->rvh_state());
-    }
-
     // Use an observer to avoid accessing a deleted renderer later on when the
     // state is being checked.
     RenderViewHostDeletedObserver rvh_observer(old_rvh);
     active_test_rvh()->SendNavigate(max_page_id + 1, url);
 
-    if (old_rvh != active_rvh() && !rvh_observer.deleted())
-      EXPECT_TRUE(old_rvh->IsSwappedOut());
+    // Make sure that we start to run the unload handler at the time of commit.
+    bool expecting_rvh_shutdown = false;
+    if (old_rvh != active_rvh() && !rvh_observer.deleted()) {
+      if (!static_cast<SiteInstanceImpl*>(
+              old_rvh->GetSiteInstance())->active_view_count()) {
+        expecting_rvh_shutdown = true;
+        EXPECT_EQ(RenderViewHostImpl::STATE_PENDING_SHUTDOWN,
+                  old_rvh->rvh_state());
+      } else {
+        EXPECT_EQ(RenderViewHostImpl::STATE_PENDING_SWAP_OUT,
+                  old_rvh->rvh_state());
+      }
+    }
+
+    // Simulate the swap out ACK coming from the pending renderer.  This should
+    // either shut down the old RVH or leave it in a swapped out state.
+    if (old_rvh != active_rvh()) {
+      old_rvh->OnSwappedOut(false);
+      if (expecting_rvh_shutdown) {
+        EXPECT_TRUE(rvh_observer.deleted());
+      } else {
+        EXPECT_EQ(RenderViewHostImpl::STATE_SWAPPED_OUT,
+                  old_rvh->rvh_state());
+      }
+    }
   }
 
   bool ShouldSwapProcesses(RenderFrameHostManager* manager,
@@ -423,7 +422,6 @@ TEST_F(RenderFrameHostManagerTest, NewTabPageProcesses) {
   ASSERT_TRUE(dest_rfh2);
 
   ntp_rfh2->GetRenderViewHost()->SendBeforeUnloadACK(true);
-  StartCrossSiteTransition(contents2.get());
   dest_rfh2->SendNavigate(101, kDestUrl);
 
   // The two RFH's should be different in every way.
@@ -441,7 +439,6 @@ TEST_F(RenderFrameHostManagerTest, NewTabPageProcesses) {
   contents2->GetController().LoadURL(
       kChromeUrl, Referrer(), PAGE_TRANSITION_LINK, std::string());
   dest_rfh2->GetRenderViewHost()->SendBeforeUnloadACK(true);
-  StartCrossSiteTransition(contents2.get());
   contents2->GetPendingMainFrame()->SendNavigate(102, kChromeUrl);
 
   EXPECT_NE(contents()->GetMainFrame()->GetSiteInstance(),
@@ -877,193 +874,6 @@ TEST_F(RenderFrameHostManagerTest, Navigate) {
       notifications.Check1AndReset(NOTIFICATION_RENDER_VIEW_HOST_CHANGED));
 }
 
-// Tests the Navigate function. In this unit test we verify that the Navigate
-// function can handle a new navigation event before the previous navigation
-// has been committed. This is also a regression test for
-// http://crbug.com/104600.
-TEST_F(RenderFrameHostManagerTest, NavigateWithEarlyReNavigation) {

[nasko, 2014/08/19 16:45:22]

Just to make sure I understand it right, we don't need this test as there is no
longer state changing before we hit the commit time in the new process and any
new incoming navigation will just cancel the old one. Is that right?

[Charlie Reis, 2014/08/19 21:20:31]

Correct.  This was exercising a bug that happened if we navigated between
SwapOut and Commit, and that state no longer exists.

-  TestNotificationTracker notifications;
-
-  SiteInstance* instance = SiteInstance::Create(browser_context());
-
-  scoped_ptr<TestWebContents> web_contents(
-      TestWebContents::Create(browser_context(), instance));
-  notifications.ListenFor(NOTIFICATION_RENDER_VIEW_HOST_CHANGED,
-                          Source<WebContents>(web_contents.get()));
-
-  RenderFrameHostManager* manager = web_contents->GetRenderManagerForTesting();
-
-  // 1) The first navigation. --------------------------
-  const GURL kUrl1("http://www.google.com/");
-  NavigationEntryImpl entry1(NULL /* instance */, -1 /* page_id */, kUrl1,
-                             Referrer(), base::string16() /* title */,
-                             PAGE_TRANSITION_TYPED,
-                             false /* is_renderer_init */);
-  RenderFrameHostImpl* 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());
-
-  // We should observe a notification.
-  EXPECT_TRUE(
-      notifications.Check1AndReset(NOTIFICATION_RENDER_VIEW_HOST_CHANGED));
-  notifications.Reset();
-
-  // Commit.
-  manager->DidNavigateFrame(host);
-
-  // Commit to SiteInstance should be delayed until RenderView commit.
-  EXPECT_TRUE(host == manager->current_frame_host());
-  ASSERT_TRUE(host);
-  EXPECT_FALSE(static_cast<SiteInstanceImpl*>(host->GetSiteInstance())->
-      HasSite());
-  static_cast<SiteInstanceImpl*>(host->GetSiteInstance())->SetSite(kUrl1);
-
-  // 2) Cross-site navigate to next site. -------------------------
-  const GURL kUrl2("http://www.example.com");
-  NavigationEntryImpl entry2(
-      NULL /* instance */, -1 /* page_id */, kUrl2, Referrer(),
-      base::string16() /* title */, PAGE_TRANSITION_TYPED,
-      false /* is_renderer_init */);
-  RenderFrameHostImpl* host2 = manager->Navigate(entry2);
-  int host2_process_id = host2->GetProcess()->GetID();
-
-  // A new RenderFrameHost should be created.
-  EXPECT_TRUE(manager->pending_frame_host());
-  ASSERT_EQ(host2, manager->pending_frame_host());
-  EXPECT_NE(host2, host);
-
-  // Check that the navigation is still suspended because the old RVH
-  // is not swapped out, yet.
-  EXPECT_TRUE(host2->are_navigations_suspended());
-  MockRenderProcessHost* test_process_host2 =
-      static_cast<MockRenderProcessHost*>(host2->GetProcess());
-  test_process_host2->sink().ClearMessages();
-  host2->render_view_host()->NavigateToURL(kUrl2);
-  EXPECT_FALSE(test_process_host2->sink().GetUniqueMessageMatching(
-      FrameMsg_Navigate::ID));
-
-  // Allow closing the current Render View (precondition for swapping out
-  // the RVH): Simulate response from RenderFrame for FrameMsg_BeforeUnload sent
-  // by DispatchBeforeUnload.
-  TestRenderViewHost* test_host =
-      static_cast<TestRenderViewHost*>(host->render_view_host());
-  MockRenderProcessHost* test_process_host =
-      static_cast<MockRenderProcessHost*>(test_host->GetProcess());
-  EXPECT_TRUE(test_process_host->sink().GetUniqueMessageMatching(
-      FrameMsg_BeforeUnload::ID));
-  test_host->SendBeforeUnloadACK(true);
-
-  // CrossSiteResourceHandler::StartCrossSiteTransition triggers a
-  // call of RenderFrameHostManager::SwapOutOldPage before
-  // RenderFrameHostManager::DidNavigateFrame is called.
-  // The RVH is swapped out after receiving the unload ack.
-  manager->SwapOutOldPage();
-  EXPECT_TRUE(test_process_host->sink().GetUniqueMessageMatching(
-      FrameMsg_SwapOut::ID));
-  test_host->OnSwappedOut(false);
-
-  EXPECT_EQ(host, manager->current_frame_host());
-  EXPECT_FALSE(manager->current_frame_host()->is_swapped_out());
-  EXPECT_EQ(host2, manager->pending_frame_host());
-  // There should be still no navigation messages being sent.
-  EXPECT_FALSE(test_process_host2->sink().GetUniqueMessageMatching(
-      FrameMsg_Navigate::ID));
-
-  // 3) Cross-site navigate to next site before 2) has committed. --------------
-  const GURL kUrl3("http://webkit.org/");
-  NavigationEntryImpl entry3(NULL /* instance */, -1 /* page_id */, kUrl3,
-                             Referrer(), base::string16() /* title */,
-                             PAGE_TRANSITION_TYPED,
-                             false /* is_renderer_init */);
-  test_process_host->sink().ClearMessages();
-  RenderFrameHostImpl* host3 = manager->Navigate(entry3);
-
-  // A new RenderFrameHost should be created. host2 is now deleted.
-  EXPECT_TRUE(manager->pending_frame_host());
-  ASSERT_EQ(host3, manager->pending_frame_host());
-  EXPECT_NE(host3, host);
-  EXPECT_NE(host3->GetProcess()->GetID(), host2_process_id);
-
-  // Navigations in the new RFH should be suspended.
-  EXPECT_TRUE(host3->are_navigations_suspended());
-  EXPECT_EQ(host, manager->current_frame_host());
-  EXPECT_FALSE(manager->current_frame_host()->is_swapped_out());
-
-  // Simulate a response to the second beforeunload request.
-  EXPECT_TRUE(test_process_host->sink().GetUniqueMessageMatching(
-      FrameMsg_BeforeUnload::ID));
-  test_host->SendBeforeUnloadACK(true);
-
-  // CrossSiteResourceHandler::StartCrossSiteTransition triggers a
-  // call of RenderFrameHostManager::SwapOutOldPage before
-  // RenderFrameHostManager::DidNavigateFrame is called. Since the previous
-  // navigation has already caused the renderer to start swapping out, there
-  // will be no more SwapOut messages being sent.
-  manager->SwapOutOldPage();
-  EXPECT_FALSE(test_process_host->sink().GetUniqueMessageMatching(
-      FrameMsg_SwapOut::ID));
-  test_host->OnSwappedOut(false);
-
-  // Commit.
-  manager->DidNavigateFrame(host3);
-  EXPECT_TRUE(host3 == manager->current_frame_host());
-  ASSERT_TRUE(host3);
-  EXPECT_TRUE(static_cast<SiteInstanceImpl*>(host3->GetSiteInstance())->
-      HasSite());
-  // Check the pending RenderFrameHost has been committed.
-  EXPECT_FALSE(manager->pending_frame_host());
-
-  // We should observe a notification.
-  EXPECT_TRUE(
-      notifications.Check1AndReset(NOTIFICATION_RENDER_VIEW_HOST_CHANGED));
-}
-
-// Test that navigation is not blocked when we make new navigation before
-// previous one has been committed. This is also a regression test for
-// http://crbug.com/104600.
-TEST_F(RenderFrameHostManagerTest, NewCrossNavigationBetweenSwapOutAndCommit) {
-  const GURL kUrl1("http://www.google.com/");
-  const GURL kUrl2("http://www.chromium.org/");
-  const GURL kUrl3("http://www.youtube.com/");
-
-  contents()->NavigateAndCommit(kUrl1);
-  TestRenderViewHost* rvh1 = test_rvh();
-
-  // Keep active_view_count nonzero so that no swapped out views in
-  // this SiteInstance get forcefully deleted.
-  static_cast<SiteInstanceImpl*>(rvh1->GetSiteInstance())->
-      increment_active_view_count();
-
-  // Navigate but don't commit.
-  contents()->GetController().LoadURL(
-      kUrl2, Referrer(), PAGE_TRANSITION_LINK, std::string());
-  EXPECT_TRUE(rvh1->is_waiting_for_beforeunload_ack());
-  contents()->ProceedWithCrossSiteNavigation();
-  EXPECT_FALSE(rvh1->is_waiting_for_beforeunload_ack());
-  StartCrossSiteTransition(contents());
-  EXPECT_TRUE(rvh1->IsWaitingForUnloadACK());
-
-  rvh1->OnSwappedOut(false);
-  EXPECT_EQ(RenderViewHostImpl::STATE_WAITING_FOR_COMMIT, rvh1->rvh_state());
-
-  TestRenderViewHost* rvh2 = pending_test_rvh();
-  EXPECT_TRUE(rvh2);
-  static_cast<SiteInstanceImpl*>(rvh2->GetSiteInstance())->
-      increment_active_view_count();
-
-  contents()->GetController().LoadURL(
-      kUrl3, Referrer(), PAGE_TRANSITION_LINK, std::string());
-  // Pending rvh2 is already deleted.
-  contents()->ProceedWithCrossSiteNavigation();
-
-  TestRenderFrameHost* rfh3 = contents()->GetPendingMainFrame();
-  EXPECT_TRUE(rfh3);
-  // Navigation should be already unblocked by rvh1.
-  EXPECT_FALSE(rfh3->are_navigations_suspended());
-}
-
 // Tests WebUI creation.
 TEST_F(RenderFrameHostManagerTest, WebUI) {
   set_should_create_webui(true);
@@ -1253,18 +1063,16 @@ TEST_F(RenderFrameHostManagerTest, NavigateAfterMissingSwapOutACK) {
   EXPECT_TRUE(rvh2->is_waiting_for_beforeunload_ack());
   contents()->ProceedWithCrossSiteNavigation();
   EXPECT_FALSE(rvh2->is_waiting_for_beforeunload_ack());
-  StartCrossSiteTransition(contents());
-  EXPECT_TRUE(rvh2->IsWaitingForUnloadACK());
 
   // The back navigation commits.
   const NavigationEntry* entry1 = contents()->GetController().GetPendingEntry();
   rvh1->SendNavigate(entry1->GetPageID(), entry1->GetURL());
+  EXPECT_TRUE(rvh2->IsWaitingForUnloadACK());
   EXPECT_EQ(RenderViewHostImpl::STATE_PENDING_SWAP_OUT, rvh2->rvh_state());
 
   // We should be able to navigate forward.
   contents()->GetController().GoForward();
   contents()->ProceedWithCrossSiteNavigation();
-  StartCrossSiteTransition(contents());
   const NavigationEntry* entry2 = contents()->GetController().GetPendingEntry();
   rvh2->SendNavigate(entry2->GetPageID(), entry2->GetURL());
   EXPECT_EQ(rvh2, rvh());
@@ -1272,6 +1080,7 @@ TEST_F(RenderFrameHostManagerTest, NavigateAfterMissingSwapOutACK) {
   EXPECT_EQ(RenderViewHostImpl::STATE_PENDING_SWAP_OUT, rvh1->rvh_state());
   rvh1->OnSwappedOut(false);
   EXPECT_TRUE(rvh1->IsSwappedOut());
+  EXPECT_EQ(RenderViewHostImpl::STATE_SWAPPED_OUT, rvh1->rvh_state());
 }
 
 // Test that we create swapped out RVHs for the opener chain when navigating an
@@ -1559,118 +1368,9 @@ TEST_F(RenderFrameHostManagerTest, NavigateWithEarlyClose) {
   EXPECT_EQ(host, manager->current_frame_host());
 }
 
-// Tests that the RenderViewHost is properly deleted when the SwapOutACK is
-// received before the new page commits.
-TEST_F(RenderFrameHostManagerTest,
-       SwapOutACKBeforeNewPageCommitsLeadsToDeletion) {
-  const GURL kUrl1("http://www.google.com/");
-  const GURL kUrl2("http://www.chromium.org/");
-
-  // Navigate to the first page.
-  contents()->NavigateAndCommit(kUrl1);
-  TestRenderFrameHost* rfh1 = contents()->GetMainFrame();
-  RenderViewHostDeletedObserver rvh_deleted_observer(rfh1->GetRenderViewHost());
-  EXPECT_EQ(RenderViewHostImpl::STATE_DEFAULT,
-            rfh1->GetRenderViewHost()->rvh_state());
-
-  // Navigate to new site, simulating onbeforeunload approval.
-  controller().LoadURL(kUrl2, Referrer(), PAGE_TRANSITION_LINK, std::string());
-  base::TimeTicks now = base::TimeTicks::Now();
-  rfh1->OnMessageReceived(FrameHostMsg_BeforeUnload_ACK(0, true, now, now));
-  EXPECT_TRUE(contents()->cross_navigation_pending());
-  TestRenderFrameHost* rfh2 = contents()->GetPendingMainFrame();
-
-  // Simulate rfh2's response, which leads to an unload request being sent to
-  // rfh1.
-  std::vector<GURL> url_chain;
-  url_chain.push_back(GURL());
-  contents()->GetRenderManagerForTesting()->OnCrossSiteResponse(
-      rfh2,
-      GlobalRequestID(0, 0), scoped_ptr<CrossSiteTransferringRequest>(),
-      url_chain, Referrer(), PAGE_TRANSITION_TYPED, false);
-  EXPECT_TRUE(contents()->cross_navigation_pending());
-  EXPECT_EQ(RenderViewHostImpl::STATE_WAITING_FOR_UNLOAD_ACK,
-            rfh1->GetRenderViewHost()->rvh_state());
-
-  // Simulate the swap out ack.
-  rfh1->OnSwappedOut(false);
-  EXPECT_EQ(RenderViewHostImpl::STATE_WAITING_FOR_COMMIT,
-            rfh1->GetRenderViewHost()->rvh_state());
-
-  // The new page commits.
-  contents()->TestDidNavigate(rfh2, 1, kUrl2, PAGE_TRANSITION_TYPED);
-  EXPECT_FALSE(contents()->cross_navigation_pending());
-  EXPECT_EQ(rfh2, contents()->GetMainFrame());
-  EXPECT_TRUE(contents()->GetPendingMainFrame() == NULL);
-  EXPECT_EQ(RenderViewHostImpl::STATE_DEFAULT,
-            rfh2->GetRenderViewHost()->rvh_state());
-
-  // rfh1's rvh should have been deleted.
-  EXPECT_TRUE(rvh_deleted_observer.deleted());
-  rfh1 = NULL;
-}
-
-// Tests that the RenderViewHost is properly swapped out when the SwapOutACK is
-// received before the new page commits.
-TEST_F(RenderFrameHostManagerTest,
-       SwapOutACKBeforeNewPageCommitsLeadsToSwapOut) {
-  const GURL kUrl1("http://www.google.com/");
-  const GURL kUrl2("http://www.chromium.org/");
-
-  // Navigate to the first page.
-  contents()->NavigateAndCommit(kUrl1);
-  TestRenderFrameHost* rfh1 = contents()->GetMainFrame();
-  RenderViewHostDeletedObserver rvh_deleted_observer(rfh1->GetRenderViewHost());
-  EXPECT_EQ(RenderViewHostImpl::STATE_DEFAULT,
-            rfh1->GetRenderViewHost()->rvh_state());
-
-  // Increment the number of active views in SiteInstanceImpl so that rfh2 is
-  // not deleted on swap out.
-  static_cast<SiteInstanceImpl*>(
-      rfh1->GetSiteInstance())->increment_active_view_count();
-
-  // Navigate to new site, simulating onbeforeunload approval.
-  controller().LoadURL(kUrl2, Referrer(), PAGE_TRANSITION_LINK, std::string());
-  base::TimeTicks now = base::TimeTicks::Now();
-  contents()->GetMainFrame()->OnMessageReceived(
-      FrameHostMsg_BeforeUnload_ACK(0, true, now, now));
-  EXPECT_TRUE(contents()->cross_navigation_pending());
-  TestRenderFrameHost* rfh2 = contents()->GetPendingMainFrame();
-
-  // Simulate rfh2's response, which leads to an unload request being sent to
-  // rfh1.
-  std::vector<GURL> url_chain;
-  url_chain.push_back(GURL());
-  contents()->GetRenderManagerForTesting()->OnCrossSiteResponse(
-      rfh2,
-      GlobalRequestID(0, 0), scoped_ptr<CrossSiteTransferringRequest>(),
-      url_chain, Referrer(), PAGE_TRANSITION_TYPED, false);
-  EXPECT_TRUE(contents()->cross_navigation_pending());
-  EXPECT_EQ(RenderViewHostImpl::STATE_WAITING_FOR_UNLOAD_ACK,
-            rfh1->GetRenderViewHost()->rvh_state());
-
-  // Simulate the swap out ack.
-  rfh1->OnSwappedOut(false);
-  EXPECT_EQ(RenderViewHostImpl::STATE_WAITING_FOR_COMMIT,
-            rfh1->GetRenderViewHost()->rvh_state());
-
-  // The new page commits.
-  contents()->TestDidNavigate(rfh2, 1, kUrl2, PAGE_TRANSITION_TYPED);
-  EXPECT_FALSE(contents()->cross_navigation_pending());
-  EXPECT_EQ(rfh2, contents()->GetMainFrame());
-  EXPECT_TRUE(contents()->GetPendingMainFrame() == NULL);
-  EXPECT_EQ(RenderViewHostImpl::STATE_DEFAULT,
-            rfh2->GetRenderViewHost()->rvh_state());
-
-  // rfh1 should be swapped out.
-  EXPECT_FALSE(rvh_deleted_observer.deleted());
-  EXPECT_TRUE(rfh1->GetRenderViewHost()->IsSwappedOut());
-}
-
-// Tests that the RenderViewHost is properly deleted when the new
-// page commits before the swap out ack is received.
-TEST_F(RenderFrameHostManagerTest,
-       NewPageCommitsBeforeSwapOutACKLeadsToDeletion) {
+// Tests that the RenderFrameHost is properly deleted when the swap out ack is
+// received (after commit).

[nasko, 2014/08/19 16:45:23]

nit: I read "(after commit)" as a test case variation and expecting to see a
test case for "before commit". In reality, the ack will always come after the
commit. I'd word this part a bit more strongly.

[Charlie Reis, 2014/08/19 21:20:31]

Done.

+TEST_F(RenderFrameHostManagerTest, DeleteFrameAfterSwapOutACK) {
   const GURL kUrl1("http://www.google.com/");
   const GURL kUrl2("http://www.chromium.org/");
 
@@ -1689,18 +1389,6 @@ TEST_F(RenderFrameHostManagerTest,
   EXPECT_TRUE(contents()->cross_navigation_pending());
   TestRenderFrameHost* rfh2 = contents()->GetPendingMainFrame();
 
-  // Simulate rfh2's response, which leads to an unload request being sent to
-  // rfh1.
-  std::vector<GURL> url_chain;
-  url_chain.push_back(GURL());
-  contents()->GetRenderManagerForTesting()->OnCrossSiteResponse(
-      rfh2,
-      GlobalRequestID(0, 0), scoped_ptr<CrossSiteTransferringRequest>(),
-      url_chain, Referrer(), PAGE_TRANSITION_TYPED, false);
-  EXPECT_TRUE(contents()->cross_navigation_pending());
-  EXPECT_EQ(RenderViewHostImpl::STATE_WAITING_FOR_UNLOAD_ACK,

[nasko, 2014/08/19 16:45:22]

I'd keep the state expectation, just change the value, so we ensure we don't
introduce a bug somewhere that moves us to a different state.

[Charlie Reis, 2014/08/19 21:20:31]

Done.

-            rfh1->GetRenderViewHost()->rvh_state());
-
   // The new page commits.
   contents()->TestDidNavigate(rfh2, 1, kUrl2, PAGE_TRANSITION_TYPED);
   EXPECT_FALSE(contents()->cross_navigation_pending());
@@ -1719,10 +1407,9 @@ TEST_F(RenderFrameHostManagerTest,
   rfh1 = NULL;
 }
 
-// Tests that the RenderViewHost is properly swapped out when the new page
-// commits before the swap out ack is received.
-TEST_F(RenderFrameHostManagerTest,
-       NewPageCommitsBeforeSwapOutACKLeadsToSwapOut) {
+// Tests that the RenderFrameHost is properly swapped out when the swap out ack
+// is received (after commit).
+TEST_F(RenderFrameHostManagerTest, SwapOutFrameAfterSwapOutACK) {
   const GURL kUrl1("http://www.google.com/");
   const GURL kUrl2("http://www.chromium.org/");
 
@@ -1746,18 +1433,6 @@ TEST_F(RenderFrameHostManagerTest,
   EXPECT_TRUE(contents()->cross_navigation_pending());
   TestRenderFrameHost* rfh2 = contents()->GetPendingMainFrame();
 
-  // Simulate rfh2's response, which leads to an unload request being sent to
-  // rfh1.
-  std::vector<GURL> url_chain;
-  url_chain.push_back(GURL());
-  contents()->GetRenderManagerForTesting()->OnCrossSiteResponse(
-      rfh2,
-      GlobalRequestID(0, 0), scoped_ptr<CrossSiteTransferringRequest>(),
-      url_chain, Referrer(), PAGE_TRANSITION_TYPED, false);
-  EXPECT_TRUE(contents()->cross_navigation_pending());
-  EXPECT_EQ(RenderViewHostImpl::STATE_WAITING_FOR_UNLOAD_ACK,
-            rfh1->GetRenderViewHost()->rvh_state());
-
   // The new page commits.
   contents()->TestDidNavigate(rfh2, 1, kUrl2, PAGE_TRANSITION_TYPED);
   EXPECT_FALSE(contents()->cross_navigation_pending());