Fixed race-condition in RenderViewHost(Manager)

content/browser/tab_contents/render_view_host_manager.cc

Index: content/browser/tab_contents/render_view_host_manager.cc
diff --git a/content/browser/tab_contents/render_view_host_manager.cc b/content/browser/tab_contents/render_view_host_manager.cc
index 5fccef96637528c1ed6be16ff9e70c8ebd0b3712..7c3de2b60e869ac859d74e05665a8d97e84ce7b4 100644
--- a/content/browser/tab_contents/render_view_host_manager.cc
+++ b/content/browser/tab_contents/render_view_host_manager.cc
@@ -701,29 +701,44 @@ 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);
-
-    // We now have a pending RVH.
-    DCHECK(!cross_navigation_pending_);
-    cross_navigation_pending_ = true;
+    // 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);
+
+      // We now have a pending RVH.
+      DCHECK(!cross_navigation_pending_);
+      cross_navigation_pending_ = true;

[Charlie Reis, 2011/11/21 19:29:02]

It looks like it might be safe to move this block after the if/else so that we
don't have to repeat it.  Is there a reason to keep it this way?

[battre, 2011/11/21 20:59:45]

I thought that the delegates in FirePageBeforeUnload might rely on
chross_navigation_pending_ being set. I did not see anything though, so I have
moved it.

Done.

+
+      // 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 unload. Therefore, we also do not need to suspend
+      // outgoing navigation messages

[Charlie Reis, 2011/11/21 19:29:02]

Nit:
// 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.

(If you delete this else clause, please move this comment above the if.)

[battre, 2011/11/21 20:59:45]

I have put this comment into an otherwise empty else block.

 
-    // 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);
+      // We now have a pending RVH.
+      DCHECK(!cross_navigation_pending_);
+      cross_navigation_pending_ = true;
+    }
 
     return pending_render_view_host_;
   } else {