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 683 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()));
+    // 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);
+      // 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 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;
+      // 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);
+      // 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.

+
+      // 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();
 }