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 1651 matching lines @@
   }
 }
 
 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);
+    SendJavaScriptDialogReply(reply_msg, true, base::string16());
     return;
   }
 
+  // Dialogs are not allowed to run during unload.
+  DCHECK(!IsWaitingForUnloadACK());

[Charlie Reis, 2017/02/27 22:26:17]

Can you double check what happens when closing a tab?  I noticed that this can
return true for RVHI::is_waiting_for_close_ack_.  If that includes the
beforeunload event for closing a tab, then my suggestion might not be safe-- a
tab can show a beforeunload dialog while closing.  If that's the case, maybe it
would be less confusing if we kept track of close (beforeunload + unload) and
navigational-unload separately.

(Maybe we treat the beforeunload and unload behavior separately already for
close, but I don't think so.)

[Avi (use Gerrit), 2017/02/27 22:38:58]

But we already keep track of beforeunload and unload separately, right?

RenderFrameHostImpl has is_waiting_for_beforeunload_ack_, which is set in
RenderFrameHostImpl::DispatchBeforeUnload. RenderViewHostImpl has
is_waiting_for_close_ack_, which is set in RenderViewHostImpl::ClosePage. I
haven't fully traced those calls yet, but RenderViewHostImpl::ClosePage sends
ViewMsg_ClosePage which turns into RenderViewImpl::OnClosePage which dispatches
the unload event, much further along than the beforeunload event.

[Charlie Reis, 2017/02/27 22:41:14]

Does the browser process send a separate beforeunload IPC when closing a tab,
though, or does it just send ViewMsg_ClosePage to run both beforeunload and
unload?

I think we might only split the two events into separate IPCs for navigations,
but I'm not sure.

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

Great!  That simplifies things.

+
   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);
   }
 
   // While a JS message dialog is showing, tabs in the same process shouldn't
   // process input events.
   GetProcess()->SetIgnoreInputEvents(true);
@@ skipping 915 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
+                : 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