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 593933a9bed3c634268d3473c331cfc247504c57..02be74253d850ebbf0e74f8b216d333785c608a9 100644
--- a/content/browser/tab_contents/render_view_host_manager.cc
+++ b/content/browser/tab_contents/render_view_host_manager.cc
@@ -700,29 +700,37 @@ 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;
 
-    // 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);
+    if (!render_view_host_->is_swapped_out()) {

[Charlie Reis, 2011/11/18 18:00:13]

I'm torn between this approach and having RenderViewHost short-circuit calls to
FirePageBeforeUnload and SwapOut if it is already swapped out.  That seems like
it's easier to follow in the code, if perhaps not quite as efficient.

I've thought about this approach some and I think it does work, but there's some
non-obvious reasons why.  Yes, we skip the FirePageBeforeUnload call, but it's
not clear to others that skipping SetNavigationsSuspended and
SetHasPendingCrossSiteRequest means that we'll also skip telling the old RVH to
run its unload handler (in SwapOut).

Whether we stick with this or modify RVH, I think it's important to add comments
saying what's going on.  You may also see if it makes sense to update the
comment at the top of tab_contents.cc, but I'm not sure it's worth complicating
that comment with edge case behavior.

I do think this requires a test, since it's subtle and could easily regress. 
Thankfully, render_view_host_manager_unittest.cc should make that pretty easy,
since you can specify exactly when certain events happen.  (There's also a
browsertest version, but I think a unittest would be easier.)

[battre, 2011/11/21 17:11:57]

I did not quite understand whether you prefer short-circuiting
SetNavigationsSuspended and FirePageBeforeUnload. I think that short-circuiting
these methods would make the code more complicated because the functions do not
execute the side-effects as their names claim.
I have added more comments. Please have a look and maybe make counter proposals
in case they seem unclear too you.
I have added a unittest test.

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