Move beforeunload hang timer duties to its own timer.

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 351 matching lines @@
       enabled_bindings_ = parent_->GetEnabledBindings();
 
     // New child frames should inherit the nav_entry_id of their parent.
     set_nav_entry_id(
         frame_tree_node_->parent()->current_frame_host()->nav_entry_id());
   }
 
   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
     // render frame should probably start owning the RenderWidgetHostImpl,
     // so this logic checking for an already existing RWHI should be removed.
     // https://crbug.com/545684
     render_widget_host_ =
         RenderWidgetHostImpl::FromID(GetProcess()->GetID(), widget_routing_id);
     if (!render_widget_host_) {
       DCHECK(frame_tree_node->parent());
@@ skipping 1080 matching lines @@
         (receive_before_unload_ack_time - send_before_unload_start_time_) -
         (renderer_before_unload_end_time - renderer_before_unload_start_time);
     UMA_HISTOGRAM_TIMES("Navigation.OnBeforeUnloadOverheadTime",
                         on_before_unload_overhead_time);
 
     frame_tree_node_->navigator()->LogBeforeUnloadTime(
         renderer_before_unload_start_time, renderer_before_unload_end_time);
   }
   // 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
   // have the current navigation stop/proceed. Otherwise, send it to the
   // RenderFrameHostManager which handles closing.
   if (IsBrowserSideNavigationEnabled() && unload_ack_is_for_navigation_) {
     // TODO(clamy): see if before_unload_end_time should be transmitted to the
     // Navigator.
     frame_tree_node_->navigator()->OnBeforeUnloadACK(
         frame_tree_node_, proceed);
@@ skipping 201 matching lines @@
 }
 
 void RenderFrameHostImpl::OnRunBeforeUnloadConfirm(
     const GURL& frame_url,
     bool is_reload,
     IPC::Message* reply_msg) {
   // While a JS beforeunload dialog is showing, tabs in the same process
   // shouldn't process input events.
   GetProcess()->SetIgnoreInputEvents(true);
   render_view_host_->GetWidget()->StopHangMonitorTimeout();
+  beforeunload_timeout_->Stop();
   delegate_->RunBeforeUnloadConfirm(this, is_reload, reply_msg);
 }
 
 void RenderFrameHostImpl::OnRunFileChooser(const FileChooserParams& params) {
   // Do not allow messages with absolute paths in them as this can permit a
   // renderer to coerce the browser to perform I/O on a renderer controlled
   // path.
   if (params.default_file_name != params.default_file_name.BaseName()) {
     bad_message::ReceivedBadMessage(GetProcess(),
                                     bad_message::RFH_FILE_CHOOSER_PATH);
@@ skipping 680 matching lines @@
 }
 
 void RenderFrameHostImpl::ResetWaitingState() {
   DCHECK(is_active());
 
   // Whenever we reset the RFH state, we should not be waiting for beforeunload
   // or close acks.  We clear them here to be safe, since they can cause
   // 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;
 }
 
 bool RenderFrameHostImpl::CanCommitOrigin(
     const url::Origin& origin,
     const GURL& url) {
   // If the --disable-web-security flag is specified, all bets are off and the
   // renderer process can send any origin it wishes.
@@ skipping 124 matching lines @@
     // Start the hang monitor in case the renderer hangs in the beforeunload
     // handler.
     is_waiting_for_beforeunload_ack_ = true;
     unload_ack_is_for_navigation_ = for_navigation;
     if (render_view_host_->GetDelegate()->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.
       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));
     }
   }
 }
 
 void RenderFrameHostImpl::SimulateBeforeUnloadAck() {
   DCHECK(is_waiting_for_beforeunload_ack_);
   base::TimeTicks approx_renderer_start_time = send_before_unload_start_time_;
   OnBeforeUnloadACK(true, approx_renderer_start_time, base::TimeTicks::Now());
@@ skipping 48 matching lines @@
 }
 
 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);
 
-  // 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();
   }
 }
 
 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);
@@ skipping 804 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
 }
 
+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