Nicely handle OnBeforeUnloads with guest and lock mode.

chrome/browser/ui/browser_list.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/browser_list.h"
 
 #include <algorithm>
 
 #include "base/logging.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browser_shutdown.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_info_cache.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_iterator.h"
 #include "chrome/browser/ui/browser_list_observer.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/host_desktop.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/user_metrics.h"
 
 using base::UserMetricsAction;
 using content::WebContents;
 
 // static
 base::LazyInstance<ObserverList<chrome::BrowserListObserver> >::Leaky
     BrowserList::observers_ = LAZY_INSTANCE_INITIALIZER;
 
 // static
 BrowserList* BrowserList::native_instance_ = NULL;
 BrowserList* BrowserList::ash_instance_ = NULL;
 
+// static
+// The current browser handling its OnBeforeUnload event before closing.
+Browser* current_closing_browser_;

[Peter Kasting, 2014/08/26 20:26:15]

Why do you actually need this?  You only ever use it in
PostBeforeUnloadHandlers(), to check whether it's non-NULL, but the only method
that queues a call to that always sets this.  Is this supposed to cause some
kind of automatic bail-out if we get a new call to CloseAllBrowsersWithProfile()
while a previous one has not finished executing?

[Mike Lerman, 2014/08/27 14:11:38]

This is done in BrowserCloseManager where I got this code from. It does seem to
create an automatic bail-out to make sure we're not trying to close browsers too
many times. I don't know the case that would necessitate its creation (and the
CL where the code was added didn't provide any insight), but I'd rather be safe
than sorry and keep it in.

[Peter Kasting, 2014/08/27 19:31:34]

I'm never OK with landing code the author and reviewer don't fully understand. 
Before you land this, you need to verify for sure that it is necessary and know
why, or else remove it.

[Mike Lerman, 2014/08/28 15:06:52]

Answer: If I have multiple browsers open, and accept all the OnBeforeUnloads of
the first browser but reject on the second browser window, the callback
(PostBeforeUnloadHandlers) will get called for both browsers. We only want to
call PostBeforeUnloadHandlers once.

+
 ////////////////////////////////////////////////////////////////////////////////
 // BrowserList, public:
 
 Browser* BrowserList::GetLastActive() const {
   if (!last_active_browsers_.empty())
     return *(last_active_browsers_.rbegin());
   return NULL;
 }
 
 // static
@@ skipping 64 matching lines @@
 // static
 void BrowserList::AddObserver(chrome::BrowserListObserver* observer) {
   observers_.Get().AddObserver(observer);
 }
 
 // static
 void BrowserList::RemoveObserver(chrome::BrowserListObserver* observer) {
   observers_.Get().RemoveObserver(observer);
 }
 
+// static
 void BrowserList::CloseAllBrowsersWithProfile(Profile* profile) {
   BrowserVector browsers_to_close;
   for (chrome::BrowserIterator it; !it.done(); it.Next()) {
     if (it->profile()->GetOriginalProfile() == profile->GetOriginalProfile())
       browsers_to_close.push_back(*it);
   }
 
   for (BrowserVector::const_iterator it = browsers_to_close.begin();
        it != browsers_to_close.end(); ++it) {
     (*it)->window()->Close();
   }
 }
 
 // static
+void BrowserList::CloseAllBrowsersWithProfile(Profile* profile,

[noms (inactive), 2014/08/26 20:17:32]

I'm a little confused because CodeSearch still shows as this being an existing
method:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

Is CodeSearch just out of sync or Rietveld confused?

[Peter Kasting, 2014/08/26 20:26:15]

I think you're confused; that method is defined just above.

It is, however, confusing that we have these two identically-named methods which
take different args.  Is the top (pre-existing) one no longer needed?  If not,
please remove it.

[Mike Lerman, 2014/08/27 14:11:38]

There is one place (Profile Deletion flow) that calls the above method. My plan
is to land this CL and then change the last caller onto this method, deleting
the method above.

I can either mark is as deprecated in a TODO comment, or change one of the
names.

[Peter Kasting, 2014/08/27 19:31:34]

Can you do that in this CL?  If not, don't worry about doing anything here.

[Mike Lerman, 2014/08/28 15:06:52]

I'm going to see if the TPM's are okay with merging this back to M38 (since
guest lives there), so I'd rather keep this CL small.

+    const base::Callback<void(size_t)>& on_close_success,
+    const base::Callback<void(size_t)>& on_close_aborted) {
+  current_closing_browser_ = NULL;
+  BrowserVector browsers_to_close;
+  for (chrome::BrowserIterator it; !it.done(); it.Next()) {
+    if (it->profile()->GetOriginalProfile() == profile->GetOriginalProfile())
+      browsers_to_close.push_back(*it);
+  }
+
+  ProfileInfoCache& cache =

[Peter Kasting, 2014/08/26 20:26:15]

Nit: Prefer pointers or const refs to non-const refs.  In some cases, simply
avoiding the temp by inlining may be better.

[Mike Lerman, 2014/08/27 14:11:38]

Inlined, since the ProfileInfoCache is only used once here.

+      g_browser_process->profile_manager()->GetProfileInfoCache();
+  size_t profile_index = cache.GetIndexOfProfileWithPath(profile->GetPath());
+
+  TryToCloseBrowserList(browsers_to_close, on_close_success,

[noms (inactive), 2014/08/26 20:17:32]

Hmm, can this and CloseAllBrowsers just get merged? It doesn't seem to be called
anywhere else, and you could just do the if statement from line 165 up after
line 147?

[Mike Lerman, 2014/08/27 14:11:38]

TryToCloseBrowserList is called in PostBeforeUnloadHandlers, below.

+      on_close_aborted, profile_index);

[Peter Kasting, 2014/08/26 20:26:15]

Nit: All lines of args must be aligned

[Mike Lerman, 2014/08/27 14:11:38]

No longer an issue.

+}
+
+// static
+void BrowserList::TryToCloseBrowserList(const BrowserVector& browsers_to_close,
+    const base::Callback<void(size_t)>& on_close_success,
+    const base::Callback<void(size_t)>& on_close_aborted,
+    const size_t profile_index) {
+  for (BrowserVector::const_iterator it = browsers_to_close.begin();
+       it != browsers_to_close.end(); ++it) {
+    if ((*it)->CallBeforeUnloadHandlers(
+            base::Bind(&BrowserList::PostBeforeUnloadHandlers,
+                       browsers_to_close,
+                       on_close_success,
+                       on_close_aborted,
+                       profile_index))) {
+      current_closing_browser_ = *it;
+      return;
+    }
+  }
+
+  on_close_success.Run(profile_index);
+
+  // Close() can invalidate the std::vector::iterator, so use array indexing.
+  Browser*const * browsers_ready_to_close = &browsers_to_close[0];
+  for (size_t i = 0; i < browsers_to_close.size(); i++)
+    browsers_ready_to_close[i]->window()->Close();

[noms (inactive), 2014/08/26 20:17:32]

The original function in CodeSearch shows that an iterator used to work (see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...).
I think it's important to understand why this no longer works.

[Peter Kasting, 2014/08/26 20:26:15]

Yeah, the difference between this and the above scares me.

[Mike Lerman, 2014/08/27 14:11:38]

Okay, I shall dig further into why the iterator gets invalidated. I'll come back
on another patch/comment.

[Mike Lerman, 2014/08/28 15:06:53]

The reason there's a difference between here and above is because here I'm
interacting directly with the OnBeforeUnload handlers, and above I'm not.
Close() calls ShouldClose() which determines if the UnBeforeUnload handlers are
processing (which they are), and thus prevents this from executing correctly.

I've been able to change this block of code to look like the block above by
changing the UnloadController slightly. Added it in for review.

Also added @sammc, since he's very familiar with all this code :)

+}
+
+// static
+void BrowserList::PostBeforeUnloadHandlers(
+    const BrowserVector& browsers_to_close,
+    const base::Callback<void(size_t)>& on_close_success,
+    const base::Callback<void(size_t)>& on_close_aborted,
+    const size_t profile_index,
+    bool proceed) {
+  if (!current_closing_browser_)
+    return;
+
+  current_closing_browser_ = NULL;
+
+  if (proceed) {
+    TryToCloseBrowserList(browsers_to_close, on_close_success, on_close_aborted,
+                          profile_index);
+  } else {
+    for (BrowserVector::const_iterator it = browsers_to_close.begin();
+        it != browsers_to_close.end(); ++it) {
+      (*it)->ResetBeforeUnloadHandlers();
+    }
+    on_close_aborted.Run(profile_index);
+  }
+}
+
+// static
 void BrowserList::SetLastActive(Browser* browser) {
   content::RecordAction(UserMetricsAction("ActiveBrowserChanged"));
   BrowserList* browser_list = GetInstance(browser->host_desktop_type());
 
   RemoveBrowserFrom(browser, &browser_list->last_active_browsers_);
   browser_list->last_active_browsers_.push_back(browser);
 
   FOR_EACH_OBSERVER(chrome::BrowserListObserver, observers_.Get(),
                     OnBrowserSetLastActive(browser));
 }
@@ skipping 30 matching lines @@
 }
 
 // static
 void BrowserList::RemoveBrowserFrom(Browser* browser,
                                     BrowserVector* browser_list) {
   BrowserVector::iterator remove_browser =
       std::find(browser_list->begin(), browser_list->end(), browser);
   if (remove_browser != browser_list->end())
     browser_list->erase(remove_browser);
 }