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 944d444d2d9d16492223aa0b817f738e1876673d..91cf8994d4116fc2013ecfd0102fa21358a2d261 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
@@ -1470,8 +1473,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
@@ -1693,6 +1695,14 @@ void RenderFrameHostImpl::OnRunBeforeUnloadConfirm(
   // 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);
 }
 
@@ -2393,8 +2403,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;
@@ -2539,12 +2548,8 @@ 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);
+      beforeunload_timeout_->Start(
+          TimeDelta::FromMilliseconds(RenderViewHostImpl::kUnloadTimeoutMS));
       send_before_unload_start_time_ = base::TimeTicks::Now();
       Send(new FrameMsg_BeforeUnload(routing_id_, is_reload));
     }
@@ -2609,33 +2614,27 @@ 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));
+      }
+    }
   }
 }
 
@@ -3462,4 +3461,11 @@ RenderFrameHostImpl::TakeNavigationHandleForCommit(
       entry_id_for_data_nav, false);  // started_from_context_menu
 }
 
+void RenderFrameHostImpl::BeforeUnloadTimeout() {
+  if (render_view_host_->GetDelegate()->ShouldIgnoreUnresponsiveRenderer())
+    return;
+
+  SimulateBeforeUnloadAck();
+}
+
 }  // namespace content