Don't wait to close tabs waiting for JavaScript dialogs.

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 c8b5dcc9fbd8a11090cd2d404fdbcc9cc96cc1bd..3a9303bc76b79f91f78647c8d3779e0a450340c4 100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -2388,14 +2388,26 @@ void RenderFrameHostImpl::DispatchBeforeUnload(bool for_navigation,
     // handler.
     is_waiting_for_beforeunload_ack_ = true;
     unload_ack_is_for_navigation_ = for_navigation;
-    // Increment the in-flight event count, to ensure that input events won't
-    // cancel the timeout timer.
-    render_view_host_->GetWidget()->increment_in_flight_event_count();
-    render_view_host_->GetWidget()->StartHangMonitorTimeout(
-        TimeDelta::FromMilliseconds(RenderViewHostImpl::kUnloadTimeoutMS),
-        RenderWidgetHostDelegate::RENDERER_UNRESPONSIVE_BEFORE_UNLOAD);
-    send_before_unload_start_time_ = base::TimeTicks::Now();
-    Send(new FrameMsg_BeforeUnload(routing_id_, is_reload));
+    if (delegate_->IsJavaScriptDialogShowing()) {
+      // If there is a JavaScript dialog up, don't bother sending the renderer
+      // the unload event because it is known unresponsive, waiting for the
+      // reply from the dialog. Instead, force the page to be in "waiting for
+      // unload ack", and tell the widget's delegate that the renderer is
+      // unresponsive. That will let the page close immediately.
+      render_view_host_->is_waiting_for_close_ack_ = true;
+      render_view_host_->GetWidget()->delegate()->RendererUnresponsive(
+          render_view_host_->GetWidget(),
+          RenderWidgetHostDelegate::RENDERER_UNRESPONSIVE_DIALOG_SHOWING);

[Charlie Reis, 2016/10/07 22:44:23]

Could we call OnBeforeUnloadACK(true, ...) here instead?  

I'm not sure it's correct to call RendererUnresponsive.  I see that you would
end up hitting the Close() block in there (by setting is_waiting_for_close_ack_
to true), but that only seems to make sense in the close case.  We can get here
for navigations as well, right?

(I would also expect a similar check in RVH?)

[Avi (use Gerrit), 2016/10/08 00:13:19]

I was hoping to run past both issues with one call, but you're right that I
should go more slowly. BTW that call is titled
"RenderFrameHostImpl::SimulateBeforeUnloadAck()" and is called by the WC in
RendererUnresponsive.
Yes. D'oh. Sigh.
If we don't skip to always closing and mess up nav, yes.

+    } else {
+      // Increment the in-flight event count, to ensure that input events won't
+      // cancel the timeout timer.
+      render_view_host_->GetWidget()->increment_in_flight_event_count();
+      render_view_host_->GetWidget()->StartHangMonitorTimeout(
+          TimeDelta::FromMilliseconds(RenderViewHostImpl::kUnloadTimeoutMS),
+          RenderWidgetHostDelegate::RENDERER_UNRESPONSIVE_BEFORE_UNLOAD);
+      send_before_unload_start_time_ = base::TimeTicks::Now();
+      Send(new FrameMsg_BeforeUnload(routing_id_, is_reload));
+    }
   }
 }