Fixed race-condition in RenderViewHost(Manager)

content/browser/tab_contents/render_view_host_manager.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/tab_contents/render_view_host_manager.h"
 
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "content/browser/debugger/devtools_manager.h"
 #include "content/browser/renderer_host/render_view_host.h"
@@ skipping 682 matching lines @@
         // handlers if the current RVH isn't live.)
         CommitPending();
         return render_view_host_;
       } else {
         NOTREACHED();
         return render_view_host_;
       }
     }
     // 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);
+    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()));
 
-    // 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);
+      // 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);
 
-    // We now have a pending RVH.
-    DCHECK(!cross_navigation_pending_);
-    cross_navigation_pending_ = 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);
+      // 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 {
     if (pending_web_ui_.get() && render_view_host_->IsRenderViewLive())
       pending_web_ui_->RenderViewReused(render_view_host_);
 
     // The renderer can exit view source mode when any error or cancellation
     // happen. We must overwrite to recover the mode.
     if (entry.IsViewSourceMode()) {
       render_view_host_->Send(
@@ skipping 67 matching lines @@
   }
 }
 
 bool RenderViewHostManager::IsSwappedOut(RenderViewHost* rvh) {
   if (!rvh->site_instance())
     return false;
 
   return swapped_out_hosts_.find(rvh->site_instance()->id()) !=
       swapped_out_hosts_.end();
 }