Do not filter IPC messages until the new RVH commits, to avoid a race.

This is an alternate fix to http://codereview.chromium.org/8587029/.

BUG=93427
BUG=104600
TEST=Go back/forward quickly with cross-process navigations.

Review URL: http://codereview.chromium.org/9288037

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@119335 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 59 insertions, 64 deletions

diff --git a/content/browser/renderer_host/render_view_host.cc b/content/browser/renderer_host/render_view_host.cc
index 5e05554..22d9b89 100644
--- a/content/browser/renderer_host/render_view_host.cc
+++ b/content/browser/renderer_host/render_view_host.cc
@@ -333,12 +333,6 @@ void RenderViewHost::FirePageBeforeUnload(bool for_cross_site_transition) {
 
 void RenderViewHost::SwapOut(int new_render_process_host_id,
                              int new_request_id) {
-  // Start filtering IPC messages to avoid confusing the delegate.  This will
-  // prevent any dialogs from appearing during unload handlers, but we've
-  // already decided to silence them in crbug.com/68780.  We will set it back
-  // to false in SetNavigationsSuspended if we swap back in.
-  is_swapped_out_ = true;
-
   // This will be set back to false in OnSwapOutACK, just before we replace
   // this RVH with the pending RVH.
   is_waiting_for_unload_ack_ = true;
@@ -371,6 +365,14 @@ void RenderViewHost::WasSwappedOut() {
   // Don't bother reporting hung state anymore.
   StopHangMonitorTimeout();
 
+  // Now that we're no longer the active RVH in the tab, start filtering out
+  // most IPC messages.  Usually the renderer will have stopped sending
+  // messages as of OnSwapOutACK.  However, we may have timed out waiting
+  // for that message, and additional IPC messages may keep streaming in.
+  // We filter them out, as long as that won't cause problems (e.g., we
+  // still allow synchronous messages through).
+  is_swapped_out_ = true;
+
   // Inform the renderer that it can exit if no one else is using it.
   Send(new ViewMsg_WasSwappedOut(routing_id()));
 }
diff --git a/content/browser/tab_contents/render_view_host_manager.cc b/content/browser/tab_contents/render_view_host_manager.cc
index caaad5e..f517479 100644
--- a/content/browser/tab_contents/render_view_host_manager.cc
+++ b/content/browser/tab_contents/render_view_host_manager.cc
@@ -703,42 +703,30 @@ RenderViewHost* RenderViewHostManager::UpdateRendererStateForNavigate(
     }
     // Otherwise, it's safe to treat this as a pending cross-site transition.
 
-    // It is possible that a previous cross-site navigation caused
-    // render_view_host_ to be swapped out and we are still waiting for
-    // the old pending_render_view_host_ to inform us about the committed
-    // navigation.
-    if (!render_view_host_->is_swapped_out()) {
-      // Make sure the old render view stops, in case a load is in progress.
-      render_view_host_->Send(
-          new ViewMsg_Stop(render_view_host_->routing_id()));
-
-      // Suspend the new render view (i.e., don't let it send the cross-site
-      // Navigate message) until we hear back from the old renderer's
-      // onbeforeunload handler.  If the handler returns false, we'll have to
-      // cancel the request.
-      DCHECK(!pending_render_view_host_->are_navigations_suspended());
-      pending_render_view_host_->SetNavigationsSuspended(true);
-
-      // Tell the CrossSiteRequestManager that this RVH has a pending cross-site
-      // request, so that ResourceDispatcherHost will know to tell us to run the
-      // old page's onunload handler before it sends the response.
-      pending_render_view_host_->SetHasPendingCrossSiteRequest(true, -1);
-
-      // Tell the old render view to run its onbeforeunload handler, since it
-      // doesn't otherwise know that the cross-site request is happening.  This
-      // will trigger a call to ShouldClosePage with the reply.
-      render_view_host_->FirePageBeforeUnload(true);
-    } else {
-      // As the render_view_host_ is already swapped out, we do not need
-      // to instruct it to run its beforeunload or unload handlers.  Therefore,
-      // we also do not need to suspend outgoing navigation messages and can
-      // just let the new pending_render_view_host_ immediately navigate.
-    }
+    // Make sure the old render view stops, in case a load is in progress.
+    render_view_host_->Send(new ViewMsg_Stop(render_view_host_->routing_id()));
+
+    // Suspend the new render view (i.e., don't let it send the cross-site
+    // Navigate message) until we hear back from the old renderer's
+    // onbeforeunload handler.  If the handler returns false, we'll have to
+    // cancel the request.
+    DCHECK(!pending_render_view_host_->are_navigations_suspended());
+    pending_render_view_host_->SetNavigationsSuspended(true);
+
+    // Tell the CrossSiteRequestManager that this RVH has a pending cross-site
+    // request, so that ResourceDispatcherHost will know to tell us to run the
+    // old page's onunload handler before it sends the response.
+    pending_render_view_host_->SetHasPendingCrossSiteRequest(true, -1);
 
     // We now have a pending RVH.
     DCHECK(!cross_navigation_pending_);
     cross_navigation_pending_ = true;
 
+    // Tell the old render view to run its onbeforeunload handler, since it
+    // doesn't otherwise know that the cross-site request is happening.  This
+    // will trigger a call to ShouldClosePage with the reply.
+    render_view_host_->FirePageBeforeUnload(true);
+
     return pending_render_view_host_;
   } else {
     if (pending_web_ui_.get() && render_view_host_->IsRenderViewLive())
diff --git a/content/browser/tab_contents/render_view_host_manager_unittest.cc b/content/browser/tab_contents/render_view_host_manager_unittest.cc
index 0891743..78e3e5d 100644
--- a/content/browser/tab_contents/render_view_host_manager_unittest.cc
+++ b/content/browser/tab_contents/render_view_host_manager_unittest.cc
@@ -436,13 +436,16 @@ TEST_F(RenderViewHostManagerTest, NavigateWithEarlyReNavigation) {
       string16() /* title */, content::PAGE_TRANSITION_TYPED,
       false /* is_renderer_init */);
   RenderViewHost* host2 = manager.Navigate(entry2);
+  int host2_process_id = host2->process()->GetID();
 
   // A new RenderViewHost should be created.
   EXPECT_TRUE(manager.pending_render_view_host());
   ASSERT_EQ(host2, manager.pending_render_view_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->process());
   test_process_host2->sink().ClearMessages();
@@ -452,7 +455,7 @@ TEST_F(RenderViewHostManagerTest, NavigateWithEarlyReNavigation) {
 
   // Allow closing the current Render View (precondition for swapping out
   // the RVH): Simulate response from RenderView for ViewMsg_ShouldClose sent by
-  // FirePageBeforeUnload
+  // FirePageBeforeUnload.
   TestRenderViewHost* test_host = static_cast<TestRenderViewHost*>(host);
   MockRenderProcessHost* test_process_host =
       static_cast<MockRenderProcessHost*>(test_host->process());
@@ -460,10 +463,10 @@ TEST_F(RenderViewHostManagerTest, NavigateWithEarlyReNavigation) {
       ViewMsg_ShouldClose::ID));
   test_host->SendShouldCloseACK(true);
 
-  // CrossSiteResourceHandler::StartCrossSiteTransition can trigger a
+  // CrossSiteResourceHandler::StartCrossSiteTransition triggers a
   // call of RenderViewHostManager::OnCrossSiteResponse before
-  // RenderViewHostManager::DidNavigateMainFrame is called. In this case the
-  // RVH is swapped out.
+  // RenderViewHostManager::DidNavigateMainFrame is called.
+  // The RVH is not swapped out until the commit.
   manager.OnCrossSiteResponse(host2->process()->GetID(),
                               host2->GetPendingRequestId());
   EXPECT_TRUE(test_process_host->sink().GetUniqueMessageMatching(
@@ -471,7 +474,7 @@ TEST_F(RenderViewHostManagerTest, NavigateWithEarlyReNavigation) {
   test_host->OnSwapOutACK();
 
   EXPECT_EQ(host, manager.current_host());
-  EXPECT_TRUE(manager.current_host()->is_swapped_out());
+  EXPECT_FALSE(manager.current_host()->is_swapped_out());
   EXPECT_EQ(host2, manager.pending_render_view_host());
   // There should be still no navigation messages being sent.
   EXPECT_FALSE(test_process_host2->sink().GetUniqueMessageMatching(
@@ -483,28 +486,36 @@ TEST_F(RenderViewHostManagerTest, NavigateWithEarlyReNavigation) {
                              content::Referrer(), string16() /* title */,
                              content::PAGE_TRANSITION_TYPED,
                              false /* is_renderer_init */);
+  test_process_host->sink().ClearMessages();
   RenderViewHost* host3 = manager.Navigate(entry3);
 
-  // A new RenderViewHost should be created.
+  // A new RenderViewHost should be created. host2 is now deleted.
   EXPECT_TRUE(manager.pending_render_view_host());
   ASSERT_EQ(host3, manager.pending_render_view_host());
+  EXPECT_NE(host3, host);
+  EXPECT_NE(host3->process()->GetID(), host2_process_id);
 
+  // Navigations in the new RVH should be suspended, which is ok because the
+  // old RVH is not yet swapped out and can respond to a second beforeunload
+  // request.
+  EXPECT_TRUE(host3->are_navigations_suspended());
   EXPECT_EQ(host, manager.current_host());
-  EXPECT_TRUE(manager.current_host()->is_swapped_out());
-
-  // The navigation should not be suspended because the RVH |host| has been
-  // swapped out already. Therefore, the RVH should send a navigation event
-  // immediately.
-  MockRenderProcessHost* test_process_host3 =
-      static_cast<MockRenderProcessHost*>(host3->process());
-  test_process_host3->sink().ClearMessages();
-
-  // Usually TabContents::NavigateToEntry would call
-  // RenderViewHostManager::Navigate followed by RenderViewHost::Navigate.
-  // Here we need to call the latter ourselves.
-  host3->NavigateToURL(kUrl3);
-  EXPECT_TRUE(test_process_host3->sink().GetUniqueMessageMatching(
-      ViewMsg_Navigate::ID));
+  EXPECT_FALSE(manager.current_host()->is_swapped_out());
+
+  // Simulate a response to the second beforeunload request.
+  EXPECT_TRUE(test_process_host->sink().GetUniqueMessageMatching(
+      ViewMsg_ShouldClose::ID));
+  test_host->SendShouldCloseACK(true);
+
+  // CrossSiteResourceHandler::StartCrossSiteTransition triggers a
+  // call of RenderViewHostManager::OnCrossSiteResponse before
+  // RenderViewHostManager::DidNavigateMainFrame is called.
+  // The RVH is not swapped out until the commit.
+  manager.OnCrossSiteResponse(host3->process()->GetID(),
+                              host3->GetPendingRequestId());
+  EXPECT_TRUE(test_process_host->sink().GetUniqueMessageMatching(
+      ViewMsg_SwapOut::ID));
+  test_host->OnSwapOutACK();
 
   // Commit.
   manager.DidNavigateMainFrame(host3);
diff --git a/content/browser/tab_contents/tab_contents.cc b/content/browser/tab_contents/tab_contents.cc
index b6c4e85..8edcdf3 100644
--- a/content/browser/tab_contents/tab_contents.cc
+++ b/content/browser/tab_contents/tab_contents.cc
@@ -103,12 +103,6 @@
 // - The previous renderer is kept swapped out in RenderViewHostManager in case
 //   the user goes back.  The process only stays live if another tab is using
 //   it, but if so, the existing frame relationships will be maintained.
-//
-// It is possible that we trigger a new navigation after we have received
-// a SwapOut_ACK message but before the FrameNavigation has been confirmed.
-// In this case the old RVH has been swapped out but the new one has not
-// replaced it, yet. Therefore, we cancel the pending RVH and skip the unloading
-// of the old RVH.
 
 using content::DevToolsAgentHost;
 using content::DevToolsAgentHostRegistry;