Fix: tabs slow to response to beforeunload are closed prematurely.

content/browser/web_contents/web_contents_impl.cc

Index: content/browser/web_contents/web_contents_impl.cc
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index 3362cc1b63417be749ef6832eabd73c03b175083..9779af39ede349e55b19f490bf760b1c63f1f7ef 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -4552,7 +4552,6 @@ void WebContentsImpl::RendererUnresponsive(
   // See http://crbug.com/65458
   if (DevToolsAgentHost::IsDebuggerAttached(this))
     return;
-
   if (rfhi->is_waiting_for_beforeunload_ack() ||
       rfhi->IsWaitingForUnloadACK()) {
     // Hang occurred while firing the beforeunload/unload handler.
@@ -4571,6 +4570,10 @@ void WebContentsImpl::RendererUnresponsive(
     bool close = true;
     if (rfhi->is_waiting_for_beforeunload_ack() && delegate_) {
       delegate_->BeforeUnloadFired(this, true, &close);
+      // Reset flag so we don't get duplicate of of ACK or run into
+      // situation when we have both |is_waiting_for_beforeunload_ack| and
+      // |IsWaitingForUnloadACK| on.
+      rfhi->DontWaitForBeforeUnloadAck();

[Charlie Reis, 2016/05/13 22:24:01]

I wonder if we should expose and call RFHI::OnBeforeUnloadACK instead.  There's
other state that gets reset in there, and listeners that get notified.  Plus it
would call BeforeUnloadFired (via RFHM::OnBeforeUnloadACK).  Seems like that
keeps with the spirit of pretending the ACK arrived when the timer expires.

clamy@, does that sound reasonable to you?  I notice there are some timing UMA
stats in there, as well as implications for PlzNavigate
(NavigatorImpl::OnBeforeUnloadACK doesn't apparently call
RFHM::OnBeforeUnloadACK, so I'm not sure it will automatically call
BeforeUnloadFired).

On the other hand, it's possible we could do this as cleanup after landing this
simpler change.  The simple change is nice and might be more easily merged to
branch, though I do worry about adding more one-off changes that will make this
harder to refactor in the long run.

[Mikhail Goncharov, 2016/05/16 05:02:01]

I have inspected RenderFrameHostImpl::OnBeforeUnloadACK and it's seems to me
that we indeed should expose this method as right now we will end up with wrong
|in_flight_event_count_| as we don't decrement it in web_contents_impl.

     }
     if (close)
       Close();