Fixed race-condition in RenderViewHost(Manager)

See bug report for details.

BUG=104600
TEST=no


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

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

content/browser/tab_contents/render_view_host_manager.cc

@@ -701,30 +701,42 @@ RenderViewHost* RenderViewHostManager::UpdateRendererStateForNavigate(
     }
     // Otherwise, it's safe to treat this as a pending cross-site transition.
 
-    // 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);
+    // 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.
+    }
 
     // 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())

content/browser/tab_contents/render_view_host_manager_unittest.cc

@@ -272,6 +272,141 @@ TEST_F(RenderViewHostManagerTest, Navigate) {
       content::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(RenderViewHostManagerTest, NavigateWithEarlyReNavigation) {
+  TestNotificationTracker notifications;
+
+  SiteInstance* instance = SiteInstance::CreateSiteInstance(profile());
+
+  TestTabContents tab_contents(profile(), instance);
+  notifications.ListenFor(
+      content::NOTIFICATION_RENDER_VIEW_HOST_CHANGED,
+      content::Source<NavigationController>(&tab_contents.controller()));
+
+  // Create.
+  RenderViewHostManager manager(&tab_contents, &tab_contents);
+
+  manager.Init(profile(), instance, MSG_ROUTING_NONE);
+
+  // 1) The first navigation. --------------------------
+  const GURL kUrl1("http://www.google.com/");
+  NavigationEntry entry1(NULL /* instance */, -1 /* page_id */, kUrl1,
+                         GURL() /* referrer */, string16() /* title */,
+                         content::PAGE_TRANSITION_TYPED,
+                         false /* is_renderer_init */);
+  RenderViewHost* host = manager.Navigate(entry1);
+
+  // The RenderViewHost created in Init will be reused.
+  EXPECT_TRUE(host == manager.current_host());
+  EXPECT_FALSE(manager.pending_render_view_host());
+
+  // We should observe a notification.
+  EXPECT_TRUE(notifications.Check1AndReset(
+      content::NOTIFICATION_RENDER_VIEW_HOST_CHANGED));
+  notifications.Reset();
+
+  // Commit.
+  manager.DidNavigateMainFrame(host);
+
+  // Commit to SiteInstance should be delayed until RenderView commit.
+  EXPECT_TRUE(host == manager.current_host());
+  ASSERT_TRUE(host);
+  EXPECT_FALSE(host->site_instance()->has_site());
+  host->site_instance()->SetSite(kUrl1);
+
+  // 2) Cross-site navigate to next site. -------------------------
+  const GURL kUrl2("http://www.example.com");
+  NavigationEntry entry2(NULL /* instance */, -1 /* page_id */, kUrl2,
+                         GURL() /* referrer */, string16() /* title */,
+                         content::PAGE_TRANSITION_TYPED,
+                         false /* is_renderer_init */);
+  RenderViewHost* host2 = manager.Navigate(entry2);
+
+  // A new RenderViewHost should be created.
+  EXPECT_TRUE(manager.pending_render_view_host());
+  ASSERT_EQ(host2, manager.pending_render_view_host());
+
+  // Check that the navigation is still suspended because the old RVH
+  // is not swapped out, yet.
+  MockRenderProcessHost* test_process_host2 =
+      static_cast<MockRenderProcessHost*>(host2->process());
+  test_process_host2->sink().ClearMessages();
+  host2->NavigateToURL(kUrl2);
+  EXPECT_FALSE(test_process_host2->sink().GetUniqueMessageMatching(
+      ViewMsg_Navigate::ID));
+
+  // Allow closing the current Render View (precondition for swapping out
+  // the RVH): Simulate response from RenderView for ViewMsg_ShouldClose sent by
+  // FirePageBeforeUnload
+  TestRenderViewHost* test_host = static_cast<TestRenderViewHost*>(host);
+  MockRenderProcessHost* test_process_host =
+      static_cast<MockRenderProcessHost*>(test_host->process());
+  EXPECT_TRUE(test_process_host->sink().GetUniqueMessageMatching(
+      ViewMsg_ShouldClose::ID));
+  test_host->SendShouldCloseACK(true);
+
+  // CrossSiteResourceHandler::StartCrossSiteTransition can trigger a
+  // call of RenderViewHostManager::OnCrossSiteResponse before
+  // RenderViewHostManager::DidNavigateMainFrame is called. In this case the
+  // RVH is swapped out.
+  manager.OnCrossSiteResponse(host2->process()->GetID(),
+                              host2->GetPendingRequestId());
+  EXPECT_TRUE(test_process_host->sink().GetUniqueMessageMatching(
+      ViewMsg_SwapOut::ID));
+  test_host->OnSwapOutACK();
+
+  EXPECT_EQ(host, manager.current_host());
+  EXPECT_TRUE(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(
+      ViewMsg_Navigate::ID));
+
+  // 3) Cross-site navigate to next site before 2) has committed. --------------
+  const GURL kUrl3("http://webkit.org/");
+  NavigationEntry entry3(NULL /* instance */, -1 /* page_id */, kUrl3,
+                         GURL() /* referrer */, string16() /* title */,
+                         content::PAGE_TRANSITION_TYPED,
+                         false /* is_renderer_init */);
+  RenderViewHost* host3 = manager.Navigate(entry3);
+
+  // A new RenderViewHost should be created.
+  EXPECT_TRUE(manager.pending_render_view_host());
+  ASSERT_EQ(host3, manager.pending_render_view_host());
+
+  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));
+
+  // Commit.
+  manager.DidNavigateMainFrame(host3);
+  EXPECT_TRUE(host3 == manager.current_host());
+  ASSERT_TRUE(host3);
+  EXPECT_TRUE(host3->site_instance()->has_site());
+  // Check the pending RenderViewHost has been committed.
+  EXPECT_FALSE(manager.pending_render_view_host());
+
+  // We should observe a notification.
+  EXPECT_TRUE(notifications.Check1AndReset(
+      content::NOTIFICATION_RENDER_VIEW_HOST_CHANGED));
+}
+
 // Tests WebUI creation.
 TEST_F(RenderViewHostManagerTest, WebUI) {
   BrowserThreadImpl ui_thread(BrowserThread::UI, MessageLoop::current());

content/browser/tab_contents/tab_contents.cc

@@ -99,6 +99,12 @@
 // - 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.
 
 namespace {