Add Browser::SkipCallBeforeUnload so that the browser windows can be closed regardless of beforeunl…

chrome/browser/ui/unload_controller.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 "chrome/browser/ui/unload_controller.h"
 
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/devtools/devtools_window.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/unload_controller_web_contents_delegate.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "extensions/features/features.h"
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/constants.h"
@@ skipping 88 matching lines @@
   if (!proceed) {
     CancelWindowClose();
     contents->SetClosedByUserGesture(false);
     return false;
   }
 
   if (RemoveFromSet(&tabs_needing_before_unload_fired_, contents)) {
     // Now that beforeunload has fired, put the tab on the queue to fire
     // unload.
     tabs_needing_unload_fired_.insert(contents);
-    ProcessPendingTabs();
+    ProcessPendingTabs(false);
     // We want to handle firing the unload event ourselves since we want to
     // fire all the beforeunload events before attempting to fire the unload
     // events should the user cancel closing the browser.
     return false;
   }
 
   return true;
 }
 
 bool UnloadController::ShouldCloseWindow() {
@@ skipping 22 matching lines @@
   //    return false.
   // 4. Otherwise: return true.
   is_attempting_to_close_browser_ = true;
   // Cases 1 and 4.
   bool need_beforeunload_fired = TabsNeedBeforeUnloadFired();
   if (need_beforeunload_fired == is_calling_before_unload_handlers())
     return !need_beforeunload_fired;
 
   // Cases 2 and 3.
   on_close_confirmed_.Reset();
-  ProcessPendingTabs();
+  ProcessPendingTabs(false);
   return false;
 }
 
-bool UnloadController::CallBeforeUnloadHandlers(
+bool UnloadController::TryToCloseWindow(
+    bool skip_beforeunload,
     const base::Callback<void(bool)>& on_close_confirmed) {
   // The devtools browser gets its beforeunload events as the results of
   // intercepting events from the inspected tab, so don't send them here as
   // well.
   if (browser_->is_devtools() || HasCompletedUnloadProcessing() ||
       !TabsNeedBeforeUnloadFired())
     return false;
 
   is_attempting_to_close_browser_ = true;
   on_close_confirmed_ = on_close_confirmed;
 
-  ProcessPendingTabs();
-  return true;
+  ProcessPendingTabs(skip_beforeunload);
+  return true && !skip_beforeunload;

[sky, 2017/03/14 00:04:05]

return !skip_beforeunload

[zmin, 2017/03/14 17:46:47]

Done.

 }
 
-void UnloadController::ResetBeforeUnloadHandlers() {
+void UnloadController::ResetTryToCloseWindow() {
   if (!is_calling_before_unload_handlers())
     return;
   CancelWindowClose();
 }
 
 bool UnloadController::TabsNeedBeforeUnloadFired() {
   if (tabs_needing_before_unload_fired_.empty()) {
     for (int i = 0; i < browser_->tab_strip_model()->count(); ++i) {
       content::WebContents* contents =
           browser_->tab_strip_model()->GetWebContentsAt(i);
@@ skipping 86 matching lines @@
 }
 
 void UnloadController::TabDetachedImpl(content::WebContents* contents) {
   if (is_attempting_to_close_browser_)
     ClearUnloadState(contents, false);
   registrar_.Remove(this,
                     content::NOTIFICATION_WEB_CONTENTS_DISCONNECTED,
                     content::Source<content::WebContents>(contents));
 }
 
-void UnloadController::ProcessPendingTabs() {
+void UnloadController::ProcessPendingTabs(bool skip_beforeunload) {
   // Cancel posted/queued ProcessPendingTabs task if there is any.
   weak_factory_.InvalidateWeakPtrs();
 
   if (!is_attempting_to_close_browser_) {
     // Because we might invoke this after a delay it's possible for the value of
     // is_attempting_to_close_browser_ to have changed since we scheduled the
     // task.
     return;
   }
 
   if (HasCompletedUnloadProcessing() && !TabsNeedBeforeUnloadFired()) {
     // We've finished all the unload events and can proceed to close the
     // browser.
     browser_->OnWindowClosing();
     return;
   }
 
+  if (skip_beforeunload) {
+    tabs_needing_unload_fired_.insert(tabs_needing_before_unload_fired_.begin(),
+                                      tabs_needing_before_unload_fired_.end());
+    if (!web_contents_delegate_)
+      web_contents_delegate_.reset(new UnloadControllerWebContentsDelegate());

[sky, 2017/03/14 00:04:05]

MakeUnique

[zmin, 2017/03/14 17:46:47]

Done.

+    for (UnloadListenerSet::iterator it = tabs_needing_unload_fired_.begin();
+         it != tabs_needing_unload_fired_.end(); ++it) {
+      (*it)->SetDelegate(web_contents_delegate_.get());

[sky, 2017/03/14 00:04:05]

How do you know it's safe to reset the delegate? Might there already be a
delegate that wants to track the WebContents?

[zmin, 2017/03/14 17:46:47]

This delegate makes sure that after windows are closed, there is no opened
dialog left behind. I think it's safe because FastUnloadController has been
using it.

https://cs.chromium.org/chromium/src/chrome/browser/ui/fast_unload_controller...

[sky, 2017/03/14 20:26:34]

AFAIK FastUnloadController isn't shipping yet, so I'm concerned. Charlie, does
this look problematic to you too?

[Charlie Reis, 2017/03/14 23:21:44]

That's a great question.  It looks like it depends on what
tabs_needing_unload_fired_ can contain, and whether it overlaps with any of the
(many) users of WebContents::SetDelegate.  It might be worth checking with
whomever works on FastUnloadController to see if they've reasoned about why it's
safe.

At first glance, there's a ton of features that create a WebContents and call
SetDelegate on it, many of which are under our control and might be unlikely to
have unload handlers (e.g., ChromeOS first run, login, etc).  Some seem less
obvious, like ChromeAppDelegate-- app windows could presumably have unload
handlers, but I don't know if they end up on this list.

If there is overlap, we'd be concerned if the existing delegate wanted to do
something on cleanup which is now skipped.

To be honest, it seems hard to guarantee this will remain safe as future code
adds more uses of delegates, even if we happen to thread the needle and it is
safe today.  I wonder if it's better to to handle it in a different way?

For example, if all we care about is that dialogs are suppressed and the tab
gets deleted at a particular stage, maybe that's state we could set on the
WebContents itself, rather than changing the delegate?

+    }
+    tabs_needing_before_unload_fired_.clear();
+  }
+
   // Process beforeunload tabs first. When that queue is empty, process
   // unload tabs.
   if (!tabs_needing_before_unload_fired_.empty()) {
     content::WebContents* web_contents =
         *(tabs_needing_before_unload_fired_.begin());
     // Null check render_view_host here as this gets called on a PostTask and
     // the tab's render_view_host may have been nulled out.
     if (web_contents->GetRenderViewHost()) {
       // If there's a devtools window attached to |web_contents|,
       // we would like devtools to call its own beforeunload handlers first,
       // and then call beforeunload handlers for |web_contents|.
       // See DevToolsWindow::InterceptPageBeforeUnload for details.
       if (!DevToolsWindow::InterceptPageBeforeUnload(web_contents))
         web_contents->DispatchBeforeUnload();
     } else {
       ClearUnloadState(web_contents, true);
     }
   } else if (is_calling_before_unload_handlers()) {
     base::Callback<void(bool)> on_close_confirmed = on_close_confirmed_;
     // Reset |on_close_confirmed_| in case the callback tests
     // |is_calling_before_unload_handlers()|, we want to return that calling
     // is complete.
     if (tabs_needing_unload_fired_.empty())
       on_close_confirmed_.Reset();
-    on_close_confirmed.Run(true);
+    if (!skip_beforeunload)
+      on_close_confirmed.Run(true);
   } else if (!tabs_needing_unload_fired_.empty()) {
     // We've finished firing all beforeunload events and can proceed with unload
     // events.
     // TODO(ojan): We should add a call to browser_shutdown::OnShutdownStarting
     // somewhere around here so that we have accurate measurements of shutdown
     // time.
     // TODO(ojan): We can probably fire all the unload events in parallel and
     // get a perf benefit from that in the cases where the tab hangs in it's
     // unload handler or takes a long time to page in.
     content::WebContents* web_contents = *(tabs_needing_unload_fired_.begin());
@@ skipping 27 matching lines @@
   }
   return false;
 }
 
 void UnloadController::ClearUnloadState(content::WebContents* web_contents,
                                         bool process_now) {
   if (is_attempting_to_close_browser_) {
     RemoveFromSet(&tabs_needing_before_unload_fired_, web_contents);
     RemoveFromSet(&tabs_needing_unload_fired_, web_contents);
     if (process_now) {
-      ProcessPendingTabs();
+      ProcessPendingTabs(false);
     } else {
       // Do not post a new task if there is already any.
       if (weak_factory_.HasWeakPtrs())
         return;
       base::ThreadTaskRunnerHandle::Get()->PostTask(
-          FROM_HERE, base::Bind(&UnloadController::ProcessPendingTabs,
-                                weak_factory_.GetWeakPtr()));
+          FROM_HERE,
+          base::Bind(&UnloadController::ProcessPendingTabs,
+                     weak_factory_.GetWeakPtr(), false));
     }
   }
 }
 
 }  // namespace chrome