[Mac] AppController needs to update its "last profile" pointer when the active profile is deleted

chrome/browser/profiles/profile_manager.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/profiles/profile_manager.h"
 
 #include <set>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 1050 matching lines @@
   return go_off_the_record;
 }
 
 // TODO(robertshield): ProfileManager should not be opening windows and should
 // not have to care about HostDesktopType. See http://crbug.com/153864
 void ProfileManager::ScheduleProfileForDeletion(
     const base::FilePath& profile_dir,
     chrome::HostDesktopType desktop_type) {
   DCHECK(IsMultipleProfilesEnabled());
 
+  bool new_last_profile_being_loaded = false;

[Alexei Svitkine (slow), 2013/05/14 15:34:12]

Instead of keeping track of this here, just do an early return in the
mac-specific code.

[noms (inactive), 2013/06/05 18:16:38]

Done.

+  size_t last_non_managed_profile_index;
+
   PrefService* local_state = g_browser_process->local_state();
   ProfileInfoCache& cache = GetProfileInfoCache();
   if (profile_dir.BaseName().MaybeAsASCII() ==
       local_state->GetString(prefs::kProfileLastUsed)) {
     // Update the last used profile pref before closing browser windows. This
     // way the correct last used profile is set for any notification observers.
     std::string last_non_managed_profile;
     for (size_t i = 0; i < cache.GetNumberOfProfiles(); ++i) {
       base::FilePath cur_path = cache.GetPathOfProfileAtIndex(i);
       if (cur_path != profile_dir && !cache.ProfileIsManagedAtIndex(i)) {
         last_non_managed_profile = cur_path.BaseName().MaybeAsASCII();
+        last_non_managed_profile_index = i;
         break;
       }
     }
     // If we're deleting the last (non-managed) profile, then create a new
     // profile in its place.
     if (last_non_managed_profile.empty()) {
       base::FilePath new_path = GenerateNextProfileDirectoryPath();
       // Make sure the last used profile path is pointing at it. This way the
       // correct last used profile is set for any notification observers.
       local_state->SetString(prefs::kProfileLastUsed,
                              new_path.BaseName().MaybeAsASCII());
       // TODO(robertshield): This desktop type needs to come from the invoker,
       // currently that involves plumbing this through web UI.
       chrome::HostDesktopType desktop_type = chrome::HOST_DESKTOP_TYPE_NATIVE;
       CreateProfileAsync(new_path,
                          base::Bind(&OnOpenWindowForNewProfile,
                                     desktop_type,
                                     CreateCallback()),
                          string16(),
                          string16(),
                          false);
     } else {
       local_state->SetString(prefs::kProfileLastUsed, last_non_managed_profile);
+
+      // On the Mac, the browser process is not killed when all browser windows
+      // are closed, so just in case we are deleting the active profile, and no
+      // other profile has been loaded, we must pre-load a next one.
+#if defined(OS_MACOSX)
+      new_last_profile_being_loaded = true;
+      base::FilePath path = profile_info_cache_->GetPathOfProfileAtIndex(
+          last_non_managed_profile_index);

[Alexei Svitkine (slow), 2013/05/14 15:34:12]

If you're just going to query the path of that profile again, why not save off
the path rather than the index of the profile in the loop? (Since the loop code
is already querying the path.)

You can then just keep the path and compute |last_non_managed_profile| from the
path in the if above.

[noms (inactive), 2013/06/05 18:16:38]

Done.

+      CreateProfileAsync(path,
+                         base::Bind(&ProfileManager::OnNewActiveProfileLoaded,

[Alexei Svitkine (slow), 2013/05/14 15:34:12]

Hmm, this may be tricky. I think you should loop in a profiles owner who touched
this code on the review.

Essentially, you're now leaving the UI thread to perform some stuff on the IO
thread while this profile is in the middle of being deleted. So there may be
some inconsistent state now while this is happening.

There's a potential race where something else may happen on the UI thread that
may interact with profile manager while the async task is in progress, before
this comes back to the UI thread.

The good news is that it looks like this codepath currently doesn't change
anything except setting prefs::kProfileLastUsed, so there's not much
inconsistent state to cause problems.

On the other, setting the pref before the profile gets loaded still causes a
problem. So you should do it in the callback (after that profile has loaded),
else the race is someone using last_non_managed_profile before the callback
runs.

The other case you need to worry about is if there's multiple deletions
happening  in sequence, so while you went off to load last_non_managed_profile
on another thread, what happens when a request to delete that profile comes in
on the UI thread? (It may already be handled well, but please make sure to
reason about this to ensure you understand what happens and why it's okay, and
add it to a comment.)

Also, this could use some tests.

[noms (inactive), 2013/06/05 18:16:38]

Done most of the comments. Working on the tests. 

On 2013/05/14 15:34:12, Alexei Svitkine wrote:

+                                    base::Unretained(this),
+                                    profile_dir),
+                         string16(),
+                         string16(),
+                         false);
+#endif
     }
   }
 
+  if (!new_last_profile_being_loaded)
+    FinishDeletingProfile(profile_dir);
+}
+
+void ProfileManager::OnNewActiveProfileLoaded(const base::FilePath& profile_dir,
+                                              Profile* profile,
+                                              Profile::CreateStatus status) {
+  // Ensure we only call the callback once per profile creation.
+  if (status == Profile::CREATE_STATUS_INITIALIZED)
+    FinishDeletingProfile(profile_dir);
+}
+
+void ProfileManager::FinishDeletingProfile(const base::FilePath& profile_dir) {
+  ProfileInfoCache& cache = GetProfileInfoCache();
   // TODO(sail): Due to bug 88586 we don't delete the profile instance. Once we
   // start deleting the profile instance we need to close background apps too.
   Profile* profile = GetProfileByPath(profile_dir);
   if (profile) {
     BrowserList::CloseAllBrowsersWithProfile(profile);
 
     // Disable sync for doomed profile.
     if (ProfileSyncServiceFactory::GetInstance()->HasProfileSyncService(
         profile)) {
       ProfileSyncServiceFactory::GetInstance()->GetForProfile(
@@ skipping 67 matching lines @@
 ProfileManager::ProfileInfo::ProfileInfo(
     Profile* profile,
     bool created)
     : profile(profile),
       created(created) {
 }
 
 ProfileManager::ProfileInfo::~ProfileInfo() {
   ProfileDestroyer::DestroyProfileWhenAppropriate(profile.release());
 }