Move beforeunload hang timer duties to its own timer.

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 419beed88c1937019e7c92813296f66cb8269458..387dd13fd7a17b406a1fccb8651421f82402a76f 100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -369,6 +369,9 @@ RenderFrameHostImpl::RenderFrameHostImpl(SiteInstance* site_instance,
   SetUpMojoIfNeeded();
   swapout_event_monitor_timeout_.reset(new TimeoutMonitor(base::Bind(
       &RenderFrameHostImpl::OnSwappedOut, weak_ptr_factory_.GetWeakPtr())));
+  beforeunload_timeout_.reset(
+      new TimeoutMonitor(base::Bind(&RenderFrameHostImpl::BeforeUnloadTimeout,
+                                    weak_ptr_factory_.GetWeakPtr())));
 
   if (widget_routing_id != MSG_ROUTING_NONE) {
     // TODO(avi): Once RenderViewHostImpl has-a RenderWidgetHostImpl, the main
@@ -1469,8 +1472,7 @@ void RenderFrameHostImpl::OnBeforeUnloadACK(
   }
   // Resets beforeunload waiting state.
   is_waiting_for_beforeunload_ack_ = false;
-  render_view_host_->GetWidget()->decrement_in_flight_event_count();
-  render_view_host_->GetWidget()->StopHangMonitorTimeout();
+  beforeunload_timeout_->Stop();
   send_before_unload_start_time_ = base::TimeTicks();
 
   // PlzNavigate: if the ACK is for a navigation, send it to the Navigator to
@@ -1692,6 +1694,7 @@ void RenderFrameHostImpl::OnRunBeforeUnloadConfirm(
   // shouldn't process input events.
   GetProcess()->SetIgnoreInputEvents(true);
   render_view_host_->GetWidget()->StopHangMonitorTimeout();
+  beforeunload_timeout_->Stop();
   delegate_->RunBeforeUnloadConfirm(this, is_reload, reply_msg);
 }
 
@@ -2392,8 +2395,7 @@ void RenderFrameHostImpl::ResetWaitingState() {
   // navigations to be ignored in OnDidCommitProvisionalLoad.
   if (is_waiting_for_beforeunload_ack_) {
     is_waiting_for_beforeunload_ack_ = false;
-    render_view_host_->GetWidget()->decrement_in_flight_event_count();
-    render_view_host_->GetWidget()->StopHangMonitorTimeout();
+    beforeunload_timeout_->Stop();
   }
   send_before_unload_start_time_ = base::TimeTicks();
   render_view_host_->is_waiting_for_close_ack_ = false;
@@ -2538,13 +2540,10 @@ void RenderFrameHostImpl::DispatchBeforeUnload(bool for_navigation,
       // reply from the dialog.
       SimulateBeforeUnloadAck();
     } 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),
-          blink::WebInputEvent::Undefined,
-          RendererUnresponsiveType::RENDERER_UNRESPONSIVE_BEFORE_UNLOAD);
+      beforeunload_timeout_type_ =
+          RendererUnresponsiveType::RENDERER_UNRESPONSIVE_BEFORE_UNLOAD;
+      beforeunload_timeout_->Start(
+          TimeDelta::FromMilliseconds(RenderViewHostImpl::kUnloadTimeoutMS));
       send_before_unload_start_time_ = base::TimeTicks::Now();
       Send(new FrameMsg_BeforeUnload(routing_id_, is_reload));
     }
@@ -2613,33 +2612,36 @@ void RenderFrameHostImpl::JavaScriptDialogClosed(
     bool dialog_was_suppressed) {
   GetProcess()->SetIgnoreInputEvents(false);
 
-  // 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 beforeunload handling.
+  // If executing as part of beforeunload event handling, resume the timeout
+  // that was suspended when the dialog was shown in OnRunBeforeUnloadConfirm().
+  //
+  // If the user said to leave the current page, they're waiting, so use the
+  // smaller onbeforeunload timeout. If the user said to stay, use the usual
+  // hung page timeout, which is longer.
   if (is_before_unload_dialog) {
-    RendererUnresponsiveType type =
+    beforeunload_timeout_type_ =
         success ? RendererUnresponsiveType::RENDERER_UNRESPONSIVE_BEFORE_UNLOAD
                 : RendererUnresponsiveType::RENDERER_UNRESPONSIVE_DIALOG_CLOSED;
-    render_view_host_->GetWidget()->StartHangMonitorTimeout(
+    beforeunload_timeout_->Start(
         success
             ? TimeDelta::FromMilliseconds(RenderViewHostImpl::kUnloadTimeoutMS)
-            : render_view_host_->GetWidget()->hung_renderer_delay(),
-        blink::WebInputEvent::Undefined, type);
+            : render_view_host_->GetWidget()->hung_renderer_delay());

[Charlie Reis, 2017/03/06 21:10:44]

In the new approach where the beforeunload timer is split out, what are we
waiting for when |success| is false?  Do we get a beforeunload ACK in that case
as well?

I'm just trying to understand why we use the longer delay (or use the timer at
all) in that case.  It'd be nice if this can be simplified, since the comment
above doesn't seem to provide a strong reason for it.  If there is a reason,
though, I'm curious to understand it.

[Avi (use Gerrit), 2017/03/06 22:11:17]

We always get a beforeunload ACK, even when the user decides to stay on the
page. (In fact, the beforeunload ack is how the render process tells us whether
we need to stay or go.)
In that case? Or ever?

For "in that case", there's no reason to treat "stay" vs "leave" differently. In
both cases, we run the beforeunload handler, get told to run a dialog or not,
send the result back, and get an ack.

For "ever", that's actually a very interesting question. The fact that we get
told to run a dialog means that the page succeeded in running the beforeunload
handler to completion. We totally could say, "hey, we've seen a dialog, so we
know we didn't hang." That would be a change in what we do from today, and I
would want to put that in a different CL so that it's didn't get caught up in
this refactor. In addition, today we pass the result of the stay/go back to the
render process, and get returned it via the FrameHostMsg_BeforeUnload_ACK, so
there's some plumbing work that would need to be done.

It's totally doable but I want to focus on one refactor at a time.

[Charlie Reis, 2017/03/06 23:47:56]

Great-- maybe we can use the shorter timeout for both just to simplify things a
bit?  No reason to use the long one now that we're distinct from input events.
Cool.  Let's consider that for later.  (Agreed there's no reason to do it in
this CL.)

[Avi (use Gerrit), 2017/03/06 23:59:15]

Sure!

   }
 
   SendJavaScriptDialogReply(reply_msg, success, user_input);
 
   // 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.
+  // messages, kill the tab immediately. A page that's spamming 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);
+    UMA_HISTOGRAM_ENUMERATION(
+        "ChildProcess.HangRendererType",
+        RendererUnresponsiveType::RENDERER_UNRESPONSIVE_DIALOG_SUPPRESSED,
+        RendererUnresponsiveType::RENDERER_UNRESPONSIVE_MAX);
+
+    SimulateBeforeUnloadAck();
   }
 }
 
@@ -3464,4 +3466,15 @@ RenderFrameHostImpl::TakeNavigationHandleForCommit(
       entry_id_for_data_nav, false);  // started_from_context_menu
 }
 
+void RenderFrameHostImpl::BeforeUnloadTimeout() {
+  if (render_view_host_->GetDelegate()->ShouldIgnoreUnresponsiveRenderer())
+    return;
+
+  UMA_HISTOGRAM_ENUMERATION(
+      "ChildProcess.HangRendererType", beforeunload_timeout_type_,

[dtapuska, 2017/03/06 18:23:46]

Is this safe? Declaring the histogram twice? The macro is going to load the
value into two different static fields. I imagine you probably want to make this
code common.

[Avi (use Gerrit), 2017/03/06 18:27:26]

Wait.. you can't do this? I already did this in
https://codereview.chromium.org/2725993002/... dammit.

Is this a useful enumeration?

[Avi (use Gerrit), 2017/03/06 18:43:09]

Ilya says in chat: "I see.  That's fine, in terms of things working correctly.
We do generally recommend refactoring the code so that there's a shared
codepath, both to reduce on the amount of generated code (the macro expands to
several lines) and to reduce the likelihood of different paths diverging from
one another (basically the same motivation as the DRY principle in general)."

[Avi (use Gerrit), 2017/03/06 23:59:15]

WebContentsImpl::RendererUnresponsive is a bunch of unrelated code, and
ChildProcess.HangRendererType registered the value of which of the unrelated
bits of code ran. This bug is about ripping apart RendererUnresponsive and
moving its random bits of code to be with the other code they're related to.

I want to entirely remove this enum as it doesn't make sense, but until then, I
don't want to merge this code into somewhere common. The whole point is that
there _is_ no common place to put it that makes sense.

Given that Ilya says it's safe, I'm assuming you're ok proceeding here?

+      RendererUnresponsiveType::RENDERER_UNRESPONSIVE_MAX);
+
+  SimulateBeforeUnloadAck();
+}
+
 }  // namespace content