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

content/browser/frame_host/render_frame_host_impl.cc

Index: content/browser/frame_host/render_frame_host_impl.cc
diff --git a/content/browser/frame_host/render_frame_host_impl.cc b/content/browser/frame_host/render_frame_host_impl.cc
index f541c6b760e24ca65c3ab75b3afba37c456d998c..f64e3d03eb47044372c896080a4635eea3c9b3fe 100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -1668,10 +1668,8 @@ void RenderFrameHostImpl::OnRunJavaScriptDialog(
     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;
   }
 
@@ -2610,20 +2608,15 @@ void RenderFrameHostImpl::JavaScriptDialogClosed(
     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)
@@ -2631,23 +2624,30 @@ void RenderFrameHostImpl::JavaScriptDialogClosed(
         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,