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 1081 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();
+
+  // The beforeunload dialog for this frame may have been triggered by a
+  // browser-side request to this frame or a frame up in the frame hierarchy.
+  // Stop any timers that are waiting.

[Charlie Reis, 2017/03/14 17:19:29]

Yeah, that sounds right.

[Avi (use Gerrit), 2017/03/14 20:52:03]

Acknowledged.

+  for (RenderFrameHostImpl* frame = this; frame; frame = frame->GetParent())

[Charlie Reis, 2017/03/14 17:19:29]

nit: This would need braces for a multi-line body, unless we remove the
conditional below (see my comment there).

[Avi (use Gerrit), 2017/03/14 20:52:03]

Acknowledged.

+    if (frame->is_waiting_for_beforeunload_ack_)

[Charlie Reis, 2017/03/14 17:19:29]

Is this check needed?  I'm guessing that is_waiting_for_beforeunload_ack_
roughly corresponds to beforeunload_timeout_->IsRunning() now, and would even
love to see us remove is_waiting_for_beforeunload_ack_ in favor of that if we
can (perhaps as a followup).

Whether we do that or not, though, I'm curious if there's a reason why we would
potentially leave a frame's timer running if is_waiting_for_beforeunload_ack_ is
false (in case they ever do differ).  If not, calling Stop unconditionally seems
simpler, since it's a no-op if the timer isn't running.

[Avi (use Gerrit), 2017/03/14 20:52:03]

If we don't have a dialog up, yes, is_waiting_for_beforeunload_ack_ ==
beforeunload_timeout_->IsRunning().

When we put up the dialog, we stop the timer and leave
is_waiting_for_beforeunload_ack_ set, so we know that we have to restart the
timer.
1. There shouldn't be a case where the timer is running with
is_waiting_for_beforeunload_ack_ false.

2. An unconditional Stop is indeed simpler.

[Avi (use Gerrit), 2017/03/14 20:58:58]

FYI, before OldSpice, alerts were serialized in a FIFO queue, so this wouldn't
be possible. With auto-dismissing dialogs, this is no longer guaranteed and that
is what causes the problem here.

+      frame->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);
+      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 44 matching lines @@
 void RenderFrameHostImpl::DeleteSurroundingTextInCodePoints(int before,
                                                             int after) {
   Send(new InputMsg_DeleteSurroundingTextInCodePoints(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);
 
-  // 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 (is_before_unload_dialog) {
-    render_view_host_->GetWidget()->StartHangMonitorTimeout(
-        success
-            ? TimeDelta::FromMilliseconds(RenderViewHostImpl::kUnloadTimeoutMS)
-            : render_view_host_->GetWidget()->hung_renderer_delay(),
-        blink::WebInputEvent::Undefined);
-  }
-
   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.
-  if (is_before_unload_dialog && dialog_was_suppressed) {
-    render_view_host_->GetWidget()->delegate()->RendererUnresponsive(
-        render_view_host_->GetWidget());
+  // If executing as part of beforeunload event handling, there may have been
+  // timers stopped in this frame or a frame up in the frame hierarchy. Restart
+  // any timers that were stopped in OnRunBeforeUnloadConfirm().
+  for (RenderFrameHostImpl* frame = this; frame; frame = frame->GetParent()) {
+    if (frame->is_waiting_for_beforeunload_ack_) {

[Charlie Reis, 2017/03/14 17:19:29]

Oh, I guess we can't remove this after all, since we're using it here to tell if
a parent needs to restart its timer?

Are we sure this is correct?  It assumes that no JavaScriptDialogClosed calls
will be made for normal dialogs whenever is_waiting_for_beforeunload_ack_ is
true on a a frame or any of its ancestors.  I could imagine a race where an
OOPIF shows an alert dialog and a parent frame starts its beforeunload timer,
with JavaScriptDialogClosed coming in for the subframe after that and before the
real beforeunload dialog appears.  Or is that not possible due to some
invariant?

[Avi (use Gerrit), 2017/03/14 20:52:03]

Yes.
No. This logic comes from pre-oopif days. It's correct in the easy case, but you
tend to see the edge cases :)
That's probably possible. Lemme think hard about this.

[Avi (use Gerrit), 2017/03/14 21:03:14]

** this belongs to this thread **

FYI, before OldSpice, alerts were serialized in a FIFO queue, so this wouldn't
be possible. With auto-dismissing dialogs, this is no longer guaranteed and that
is what causes the problem here.

[Charlie Reis, 2017/03/15 17:22:02]

Is that true with OOPIFs, though?  In my example above, an OOPIF shows an alert
dialog.  While that's showing, the parent frame (in a different renderer,
unblocked) triggers a cross-process navigation via OpenURL (e.g., to an
extension URL).  Would that start the beforeunload on the parent frame before
the alert is dismissed?  If so, closing the alert might unexpectedly restart the
timer.

In retrospect, though, I think this probably can't happen, and may not even
matter if it did.  I don't think the parent frame renderer process can trigger
is_waiting_for_beforeunload_ack_ even if it goes through OpenURL, right?  And if
the user navigates the tab (e.g., by editing the URL while the alert is visible,
which is possible now), the subframe's dialog appears to be dismissed before we
start beforeunload.  (Is that right?  I'm only checking on Canary, not testing
in a debugger.)

Even if the alert were dismissed after is_waiting_for_beforeunload_ack_ gets set
on the parent, we're pretty much just restarting a 1 second timer, which
shouldn't be a problem.  (I suppose there's the dialog_was_suppressed case, but
that's probably doing something sensible too.)

Sorry for being paranoid here, but these are the kinds of things that lead to
big hang monitor investigations down the road.  :)
Great, thanks for looking into it.

[Avi (use Gerrit), 2017/03/15 19:57:38]

Yes.
There is *no timer* in this case.

Suppose the javascript alert dialog is up. The parent frame triggers a
cross-process navigation. That navigation causes
RenderFrameHostImpl::DispatchBeforeUnload to be called.
RenderFrameHostImpl::DispatchBeforeUnload sets is_waiting_for_beforeunload_ack_
to be true, and *before starting the timer* checks to see if a dialog is being
shown on the page. Yes there is, so *no timer* is started, and it calls
SimulateBeforeUnloadAck, which proceeds with the navigation.
I believe it can set is_waiting_for_beforeunload_ack_, but there will be no
timer and is_waiting_for_beforeunload_ack_ will be immediately reset, as I trace
above.
Navigating dismisses dialogs. I don't know offhand if this is before or after
beforeunload.
No worries.

[Charlie Reis, 2017/03/15 21:34:20]

Thanks-- that does explain it.  And in particular, it checks with the
*WebContents* whether a dialog is showing, so the parent frame would be able to
notice the dialog even in the case an OOPIF put it up.  Great.

+      // If we are waiting for a beforeunload ack and the user has suppressed
+      // 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.
+      if (dialog_was_suppressed) {
+        frame->SimulateBeforeUnloadAck();
+      } else {
+        frame->beforeunload_timeout_->Start(
+            TimeDelta::FromMilliseconds(RenderViewHostImpl::kUnloadTimeoutMS));
+      }
+    }
   }
 }
 
 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 806 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;
+
+  SimulateBeforeUnloadAck();
+}
+
 }  // namespace content