Prevent modal dialogs when preparing to swap out.

content/browser/frame_host/render_view_host_manager.cc

Index: content/browser/frame_host/render_view_host_manager.cc
diff --git a/content/browser/frame_host/render_view_host_manager.cc b/content/browser/frame_host/render_view_host_manager.cc
index 34e2a00c10427fb9d6d29ee521b5b52ec81aabb9..a8473cd19969eb4c3abaf0180bae9767008847bd 100644
--- a/content/browser/frame_host/render_view_host_manager.cc
+++ b/content/browser/frame_host/render_view_host_manager.cc
@@ -437,12 +437,14 @@ void RenderViewHostManager::SwapOutOldPage() {
   // Should only see this while we have a pending renderer or transfer.
   CHECK(cross_navigation_pending_ || pending_nav_params_.get());
 
-  // First close any modal dialogs that would prevent us from swapping out.
-  // TODO(creis): This is not a guarantee.  The renderer could immediately
-  // create another dialog in a loop, potentially causing a renderer crash when
-  // we tell it to swap out with a nested message loop and PageGroupLoadDeferrer
-  // on the stack.  We should prevent the renderer from showing more dialogs
-  // until the SwapOut.  See http://crbug.com/312490.
+  // Tell the renderer to suppress any further modal dialogs so that we can swap
+  // it out.  This must be done before canceling any current dialog, in case
+  // there is a loop creating additional dialogs.
+  render_view_host_->SuppressDialogsUntilSwapOut();
+
+  // Now close any modal dialogs that would prevent us from swapping out.  This
+  // must be done separately from SwapOut, so that the PageGroupLoadDeferrer is
+  // no longer on the stack when we send the SwapOut message.

[darin (slow to review), 2013/11/01 21:01:42]

But isn't there a race condition here? What if a PageGroupLoadDeferrer is active
before the call to SuppressDialogsUntilSwapOut?

[Charlie Reis, 2013/11/04 19:32:49]

This should work in that case, as long as there's only one PageGroupLoadDeferrer
on the stack.  Here's what should happen:
1) Renderer creates a PageGroupLoadDeferrer for some kind of modal dialog.
2) Browser sends a SuppressDialogsUntilSwapOut message.  This means no further
dialogs can be created, though one is still showing.
3) Browser calls CancelModalDialogsForRenderManager, closing any current
dialogs.  No further dialogs can be created, and none are showing.
4) Browser sends a SwapOut message, with no PageGroupLoadDeferrer on the stack.

Sound right?

   delegate_->CancelModalDialogsForRenderManager();
 
   // Tell the old renderer it is being swapped out.  This will fire the unload
@@ -773,14 +775,6 @@ void RenderViewHostManager::CommitPending() {
   // this triggers won't be able to figure out what's going on.
   bool will_focus_location_bar = delegate_->FocusLocationBarByDefault();
 
-  // We currently can't guarantee that the renderer isn't showing a new modal
-  // dialog, even though we canceled them in SwapOutOldPage.  (It may have
-  // created another in the meantime.)  Make sure we run and reset the callback
-  // now before we delete its RVH below.
-  // TODO(creis): Remove this if we can guarantee that no new dialogs will be
-  // shown after SwapOutOldPage.  See http://crbug.com/312490.
-  delegate_->CancelModalDialogsForRenderManager();
-
   // Next commit the Web UI, if any. Either replace |web_ui_| with
   // |pending_web_ui_|, or clear |web_ui_| if there is no pending WebUI, or
   // leave |web_ui_| as is if reusing it.