Prevent modal dialogs when preparing to swap out.

This ensures that no PageGroupLoadDeferrers will be on the stack when
processing ViewMsg_SwapOut, which would lead to an ASSERT.

BUG=312490
TEST=Start a cross-process navigation and show a loop of alerts before commit.
R=cdn@chromium.org, darin@chromium.org

Review URL: https://codereview.chromium.org/50223002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@233371 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/ui/browser_browsertest.cc

@@ -472,13 +472,13 @@ IN_PROC_BROWSER_TEST_F(BrowserTest, CrossProcessNavCancelsDialogs) {
   GURL url(test_server()->GetURL("empty.html"));
   ui_test_utils::NavigateToURL(browser(), url);
 
-  // TODO(creis): Test this with a setInterval loop of alert dialogs to ensure
-  // that we can navigate away even if the renderer tries to synchronously
-  // create more.  See http://crbug.com/312490.
+  // Test this with multiple alert dialogs to ensure that we can navigate away
+  // even if the renderer tries to synchronously create more.
+  // See http://crbug.com/312490.
   WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents();
   contents->GetRenderViewHost()->ExecuteJavascriptInWebFrame(
       string16(),
-      ASCIIToUTF16("alert('Dialog showing!');"));
+      ASCIIToUTF16("alert('one'); alert('two');"));
   AppModalDialog* alert = ui_test_utils::WaitForAppModalDialog();
   EXPECT_TRUE(alert->IsValid());
   AppModalDialogQueue* dialog_queue = AppModalDialogQueue::GetInstance();

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.
   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.

content/browser/renderer_host/render_view_host_impl.cc

@@ -671,6 +671,10 @@ void RenderViewHostImpl::OnCrossSiteResponse(
   }
 }
 
+void RenderViewHostImpl::SuppressDialogsUntilSwapOut() {
+  Send(new ViewMsg_SuppressDialogsUntilSwapOut(GetRoutingID()));
+}
+
 void RenderViewHostImpl::SwapOut() {
   // This will be set back to false in OnSwapOutACK, just before we replace
   // this RVH with the pending RVH.

content/browser/renderer_host/render_view_host_impl.h

@@ -303,6 +303,12 @@ class CONTENT_EXPORT RenderViewHostImpl
       PageTransition page_transition,
       int64 frame_id);
 
+  // Tells the renderer that this RenderView will soon be swapped out, and thus
+  // not to create any new modal dialogs until it happens.  This must be done
+  // separately so that the PageGroupLoadDeferrers of any current dialogs are no
+  // longer on the stack when we attempt to swap it out.
+  void SuppressDialogsUntilSwapOut();
+
   // Tells the renderer that this RenderView is being swapped out for one in a
   // different renderer process.  It should run its unload handler and move to
   // a blank document.  The renderer should preserve the Frame object until it

content/common/view_messages.h

@@ -1158,6 +1158,11 @@ IPC_MESSAGE_ROUTED0(ViewMsg_CantFocus)
 // via ViewHostMsg_ShouldClose.
 IPC_MESSAGE_ROUTED0(ViewMsg_ShouldClose)
 
+// Tells the renderer to suppress any further modal dialogs until it receives a
+// corresponding ViewMsg_SwapOut message.  This ensures that no
+// PageGroupLoadDeferrer is on the stack for SwapOut.
+IPC_MESSAGE_ROUTED0(ViewMsg_SuppressDialogsUntilSwapOut)
+
 // Instructs the renderer to swap out for a cross-site transition, including
 // running the unload event handler. Expects a SwapOut_ACK message when
 // finished.

content/renderer/render_view_impl.cc

@@ -832,6 +832,7 @@ RenderViewImpl::RenderViewImpl(RenderViewImplParams* params)
       navigation_gesture_(NavigationGestureUnknown),
       opened_by_user_gesture_(true),
       opener_suppressed_(false),
+      suppress_dialogs_until_swap_out_(false),
       page_id_(-1),
       last_page_id_sent_to_browser_(-1),
       next_page_id_(params->next_page_id),
@@ -1425,6 +1426,8 @@ bool RenderViewImpl::OnMessageReceived(const IPC::Message& message) {
                         OnEnumerateDirectoryResponse)
     IPC_MESSAGE_HANDLER(ViewMsg_RunFileChooserResponse, OnFileChooserResponse)
     IPC_MESSAGE_HANDLER(ViewMsg_ShouldClose, OnShouldClose)
+    IPC_MESSAGE_HANDLER(ViewMsg_SuppressDialogsUntilSwapOut,
+                        OnSuppressDialogsUntilSwapOut)
     IPC_MESSAGE_HANDLER(ViewMsg_SwapOut, OnSwapOut)
     IPC_MESSAGE_HANDLER(ViewMsg_ClosePage, OnClosePage)
     IPC_MESSAGE_HANDLER(ViewMsg_ThemeChanged, OnThemeChanged)
@@ -2277,6 +2280,11 @@ bool RenderViewImpl::RunJavaScriptMessage(JavaScriptMessageType type,
                                           const string16& default_value,
                                           const GURL& frame_url,
                                           string16* result) {
+  // Don't allow further dialogs if we are waiting to swap out, since the
+  // PageGroupLoadDeferrer in our stack prevents it.
+  if (suppress_dialogs_until_swap_out_)
+    return false;
+
   bool success = false;
   string16 result_temp;
   if (!result)
@@ -2724,6 +2732,11 @@ bool RenderViewImpl::runModalBeforeUnloadDialog(
   if (is_swapped_out_)
     return true;
 
+  // Don't allow further dialogs if we are waiting to swap out, since the
+  // PageGroupLoadDeferrer in our stack prevents it.
+  if (suppress_dialogs_until_swap_out_)
+    return false;
+
   bool success = false;
   // This is an ignored return value, but is included so we can accept the same
   // response as RunJavaScriptMessage.
@@ -3011,6 +3024,11 @@ void RenderViewImpl::show(WebNavigationPolicy policy) {
 void RenderViewImpl::runModal() {
   DCHECK(did_show_) << "should already have shown the view";
 
+  // Don't allow further dialogs if we are waiting to swap out, since the
+  // PageGroupLoadDeferrer in our stack prevents it.
+  if (suppress_dialogs_until_swap_out_)
+    return;
+
   // We must keep WebKit's shared timer running in this case in order to allow
   // showModalDialog to function properly.
   //
@@ -5336,6 +5354,11 @@ void RenderViewImpl::OnShouldClose() {
                                        before_unload_end_time));
 }
 
+void RenderViewImpl::OnSuppressDialogsUntilSwapOut() {
+  // Don't show any more dialogs until we finish OnSwapOut.
+  suppress_dialogs_until_swap_out_ = true;
+}
+
 void RenderViewImpl::OnSwapOut() {
   // Only run unload if we're not swapped out yet, but send the ack either way.
   if (!is_swapped_out_) {
@@ -5369,6 +5392,9 @@ void RenderViewImpl::OnSwapOut() {
     webview()->setVisibilityState(WebKit::WebPageVisibilityStateHidden, false);
   }
 
+  // It is now safe to show modal dialogs again.
+  suppress_dialogs_until_swap_out_ = false;
+
   Send(new ViewHostMsg_SwapOut_ACK(routing_id_));
 }
 

content/renderer/render_view_impl.h

@@ -1008,6 +1008,7 @@ class CONTENT_EXPORT RenderViewImpl
   void OnShouldClose();
   void OnStop();
   void OnStopFinding(StopFindAction action);
+  void OnSuppressDialogsUntilSwapOut();
   void OnSwapOut();
   void OnThemeChanged();
   void OnUpdateTargetURLAck();
@@ -1250,6 +1251,11 @@ class CONTENT_EXPORT RenderViewImpl
   // decidePolicyForNavigation for details.
   bool opener_suppressed_;
 
+  // Whether we must stop creating nested message loops for modal dialogs until
+  // OnSwapOut is called.  This is necessary because modal dialogs have a
+  // PageGroupLoadDeferrer on the stack that interferes with swapping out.
+  bool suppress_dialogs_until_swap_out_;
+
   // Holds state pertaining to a navigation that we initiated.  This is held by
   // the WebDataSource::ExtraData attribute.  We use pending_navigation_state_
   // as a temporary holder for the state until the WebDataSource corresponding