A page can't fire dialogs in unload handlers; remove code that handles that case.

content/browser/frame_host/render_frame_host_impl.cc

 // Copyright 2013 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/frame_host/render_frame_host_impl.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 1650 matching lines @@
     NOTREACHED() << "Received script response for unknown request";
   }
 }
 
 void RenderFrameHostImpl::OnRunJavaScriptDialog(
     const base::string16& message,
     const base::string16& default_prompt,
     const GURL& frame_url,
     JavaScriptDialogType dialog_type,
     IPC::Message* reply_msg) {
-  if (!is_active()) {
-    JavaScriptDialogClosed(reply_msg, true, base::string16(),
-                           /*is_before_unload_dialog=*/false,
-                           /*dialog_was_suppressed=*/true);
+  if (IsWaitingForUnloadACK()) {

[Charlie Reis, 2017/02/28 04:59:21]

Interesting.  Yes, I think this is safer, since it can handle races where the
renderer process sent up a dialog IPC just as the browser was starting to close
the tab.  We should cancel the dialog as you're doing here.

[Avi (use Gerrit), 2017/02/28 05:12:42]

Yep. I realized that we already were handling the passing-IPCs-in-the-night case
for swapping out, so it would be appropriate to do so for closing, as well.

+    SendJavaScriptDialogReply(reply_msg, true, base::string16());
     return;
   }
 
   int32_t message_length = static_cast<int32_t>(message.length());
   if (GetParent()) {
     UMA_HISTOGRAM_COUNTS("JSDialogs.CharacterCount.Subframe", message_length);
   } else {
     UMA_HISTOGRAM_COUNTS("JSDialogs.CharacterCount.MainFrame", message_length);
   }
 
@@ skipping 918 matching lines @@
   Send(new InputMsg_DeleteSurroundingText(routing_id_, before, after));
 }
 
 void RenderFrameHostImpl::JavaScriptDialogClosed(
     IPC::Message* reply_msg,
     bool success,
     const base::string16& user_input,
     bool is_before_unload_dialog,
     bool dialog_was_suppressed) {
   GetProcess()->SetIgnoreInputEvents(false);
-  bool is_waiting = is_before_unload_dialog || IsWaitingForUnloadACK();
 
-  // If we are executing as part of (before)unload event handling, we don't
+  // If we are executing as part of beforeunload event handling, we don't
   // want to use the regular hung_renderer_delay_ms_ if the user has agreed to
   // leave the current page. In this case, use the regular timeout value used
-  // during the (before)unload handling.
-  if (is_waiting) {
+  // during the beforeunload handling.
+  if (is_before_unload_dialog) {
     RendererUnresponsiveType type =
-        RendererUnresponsiveType::RENDERER_UNRESPONSIVE_DIALOG_CLOSED;
-    if (success) {
-      type = is_before_unload_dialog
-                 ? RendererUnresponsiveType::RENDERER_UNRESPONSIVE_BEFORE_UNLOAD
-                 : RendererUnresponsiveType::RENDERER_UNRESPONSIVE_UNLOAD;
-    }
+        success ? RendererUnresponsiveType::RENDERER_UNRESPONSIVE_BEFORE_UNLOAD

[clamy, 2017/03/01 16:11:00]

If we are in the before unload dialog, shouldn't the RendererUnresponsiveType
always be RendererUnresponsiveType::RENDERER_UNRESPONSIVE_BEFORE_UNLOAD?

[Avi (use Gerrit), 2017/03/01 16:17:38]

This metric is yours, so I'm trying to preserve existing behavior. I hadn't
thought a ton about the semantics of those types but the more that I think about
them the more confused I get.

From what I can see in the old code, we had three types:

RENDERER_UNRESPONSIVE_DIALOG_CLOSED: If this was a "beforeunload" dialog, this
meant that the hang was after the user clicked the "stay on page" button. If
this was a dialog that ran inside of "unload" (which shouldn't happen but
whatever) this meant that the hang was after the user clicked "cancel" in an
"ok/cancel" dialog. This conflation is weird.
RENDERER_UNRESPONSIVE_BEFORE_UNLOAD: This meant that the hang was after the user
clicked the "leave page" button.
RENDERER_UNRESPONSIVE_UNLOAD: This meant that the hang was after the user
clicked "ok" in an "ok/cancel" dialog.

This CL is meant to preserve the behavior of the code. Your comment would be a
change in the case where the user clicked "stay on page" in a "beforeunload"
dialog.

[clamy, 2017/03/01 16:25:56]

Well I don't quite remember the choices behind the metric, so let's keep the
behavior as is.

+                : RendererUnresponsiveType::RENDERER_UNRESPONSIVE_DIALOG_CLOSED;
     render_view_host_->GetWidget()->StartHangMonitorTimeout(
         success
             ? TimeDelta::FromMilliseconds(RenderViewHostImpl::kUnloadTimeoutMS)
             : render_view_host_->GetWidget()->hung_renderer_delay(),
         blink::WebInputEvent::Undefined, type);
   }
 
-  FrameHostMsg_RunJavaScriptDialog::WriteReplyParams(reply_msg, success,
-                                                     user_input);
-  Send(reply_msg);
+  SendJavaScriptDialogReply(reply_msg, success, user_input);
 
-  // If we are waiting for an unload or beforeunload ack and the user has
-  // suppressed messages, kill the tab immediately; a page that's spamming
-  // alerts in onbeforeunload is presumably malicious, so there's no point in
-  // continuing to run its script and dragging out the process.
-  // This must be done after sending the reply since RenderView can't close
-  // correctly while waiting for a response.
-  if (is_waiting && dialog_was_suppressed) {
+  // If we are waiting for a beforeunload ack and the user has suppressed
+  // messages, kill the tab immediately; a page that's spamming alerts in
+  // onbeforeunload is presumably malicious, so there's no point in continuing
+  // to run its script and dragging out the process. This must be done after
+  // sending the reply since RenderView can't close correctly while waiting for
+  // a response.
+  if (is_before_unload_dialog && dialog_was_suppressed) {
     render_view_host_->GetWidget()->delegate()->RendererUnresponsive(
         render_view_host_->GetWidget(),
         RendererUnresponsiveType::RENDERER_UNRESPONSIVE_DIALOG_SUPPRESSED);
   }
 }
 
+void RenderFrameHostImpl::SendJavaScriptDialogReply(
+    IPC::Message* reply_msg,
+    bool success,
+    const base::string16& user_input) {
+  FrameHostMsg_RunJavaScriptDialog::WriteReplyParams(reply_msg, success,
+                                                     user_input);
+  Send(reply_msg);
+}
+
 // PlzNavigate
 void RenderFrameHostImpl::CommitNavigation(
     ResourceResponse* response,
     std::unique_ptr<StreamHandle> body,
     const CommonNavigationParams& common_params,
     const RequestNavigationParams& request_params,
     bool is_view_source) {
   DCHECK(
       (response && body.get()) ||
       common_params.url.SchemeIs(url::kDataScheme) ||
@@ skipping 789 matching lines @@
   // There is no pending NavigationEntry in these cases, so pass 0 as the
   // pending_nav_entry_id. If the previous handle was a prematurely aborted
   // navigation loaded via LoadDataWithBaseURL, propagate the entry id.
   return NavigationHandleImpl::Create(
       params.url, params.redirects, frame_tree_node_, is_renderer_initiated,
       params.was_within_same_page, base::TimeTicks::Now(),
       entry_id_for_data_nav, false);  // started_from_context_menu
 }
 
 }  // namespace content