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

chrome/browser/ui/unload_controller.cc

Index: chrome/browser/ui/unload_controller.cc
diff --git a/chrome/browser/ui/unload_controller.cc b/chrome/browser/ui/unload_controller.cc
index 7a30244ff82a5b5d2a596bc747714e12bc18e913..bc426e79d0a521258c8fe7d3a208e3e0ee18ad36 100644
--- a/chrome/browser/ui/unload_controller.cc
+++ b/chrome/browser/ui/unload_controller.cc
@@ -12,6 +12,7 @@
 #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"
@@ -120,7 +121,7 @@ bool UnloadController::BeforeUnloadFired(content::WebContents* 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.
@@ -163,11 +164,12 @@ bool UnloadController::ShouldCloseWindow() {
 
   // 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
@@ -179,11 +181,11 @@ bool UnloadController::CallBeforeUnloadHandlers(
   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();
@@ -290,7 +292,7 @@ void UnloadController::TabDetachedImpl(content::WebContents* contents) {
                     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();
 
@@ -308,6 +310,18 @@ void UnloadController::ProcessPendingTabs() {
     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()) {
@@ -332,7 +346,8 @@ void UnloadController::ProcessPendingTabs() {
     // 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.
@@ -380,14 +395,15 @@ void UnloadController::ClearUnloadState(content::WebContents* web_contents,
     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));
     }
   }
 }