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

content/browser/web_contents/web_contents_impl.cc

 // Copyright (c) 2012 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/web_contents/web_contents_impl.h"
 
 #include <stddef.h>
 
 #include <cmath>
 #include <utility>
@@ skipping 4534 matching lines @@
     return;
 
   RenderFrameHostImpl* rfhi =
       static_cast<RenderFrameHostImpl*>(GetRenderViewHost()->GetMainFrame());
 
   // Ignore renderer unresponsive event if debugger is attached to the tab
   // since the event may be a result of the renderer sitting on a breakpoint.
   // 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.
     // Pretend the handler fired so tab closing continues as if it had.
     GetRenderViewHost()->set_sudden_termination_allowed(true);
 
     if (!GetRenderManager()->ShouldCloseTabOnUnresponsiveRenderer())
       return;
 
     // If the tab hangs in the beforeunload/unload handler there's really
     // nothing we can do to recover. If the hang is in the beforeunload handler,
     // pretend the beforeunload listeners have all fired and allow the delegate
     // to continue closing; the user will not have the option of cancelling the
     // close. Otherwise, pretend the unload listeners have all fired and close
     // the tab.
     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();
     return;
   }
 
   if (!GetRenderViewHost() || !GetRenderViewHost()->IsRenderViewLive())
     return;
 
   if (delegate_)
@@ skipping 449 matching lines @@
   for (RenderViewHost* render_view_host : render_view_host_set)
     render_view_host->OnWebkitPreferencesChanged();
 }
 
 void WebContentsImpl::SetJavaScriptDialogManagerForTesting(
     JavaScriptDialogManager* dialog_manager) {
   dialog_manager_ = dialog_manager;
 }
 
 }  // namespace content