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

chrome/browser/profiles/profile_manager.cc

Index: chrome/browser/profiles/profile_manager.cc
diff --git a/chrome/browser/profiles/profile_manager.cc b/chrome/browser/profiles/profile_manager.cc
index dc5a0d1d38d0e79d9ea41e8510af4af1ffd4a782..d1ad9ad2a13927e296a825377d6cb22661406dc9 100644
--- a/chrome/browser/profiles/profile_manager.cc
+++ b/chrome/browser/profiles/profile_manager.cc
@@ -495,8 +495,7 @@ void ProfileManager::CreateProfileAsync(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   // Make sure that this profile is not pending deletion.
-  if (std::find(ProfilesToDelete().begin(), ProfilesToDelete().end(),
-      profile_path) != ProfilesToDelete().end()) {
+  if (IsProfileMarkedForDeletion(profile_path)) {
     if (!callback.is_null())
       callback.Run(NULL, Profile::CREATE_STATUS_LOCAL_FAIL);
     return;
@@ -1043,23 +1042,28 @@ void ProfileManager::ScheduleProfileForDeletion(
     const base::FilePath& profile_dir,
     const CreateCallback& callback) {
   DCHECK(IsMultipleProfilesEnabled());
-
   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;
+    base::FilePath last_non_managed_profile_path;
     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();
+      // Make sure that this profile is not pending deletion.
+      if (cur_path != profile_dir && !cache.ProfileIsManagedAtIndex(i) &&
+          !IsProfileMarkedForDeletion(cur_path)) {
+        last_non_managed_profile_path = cur_path;
         break;
       }
     }
+
     // If we're deleting the last (non-managed) profile, then create a new
     // profile in its place.
+    const std::string last_non_managed_profile =
+        last_non_managed_profile_path.BaseName().MaybeAsASCII();
     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
@@ -1072,13 +1076,62 @@ void ProfileManager::ScheduleProfileForDeletion(
                          string16(),
                          false);
     } else {
+      // 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)
+      CreateProfileAsync(last_non_managed_profile_path,
+                         base::Bind(&ProfileManager::OnNewActiveProfileLoaded,
+                                    base::Unretained(this),
+                                    profile_dir,
+                                    last_non_managed_profile_path,
+                                    callback),
+                         string16(),
+                         string16(),
+                         false);
+      return;
+#else
+      // For OS_MACOSX the pref is updated in the callback to make sure that
+      // it isn't used before the profile is actually loaded.
       local_state->SetString(prefs::kProfileLastUsed, last_non_managed_profile);
+#endif
     }
   }
+  FinishDeletingProfile(profile_dir);
+}
 
+void ProfileManager::OnNewActiveProfileLoaded(
+    const base::FilePath& profile_to_delete_dir,
+    const base::FilePath& last_non_managed_profile_path,
+    const CreateCallback& original_callback,
+    Profile* loaded_profile,
+    Profile::CreateStatus status) {
+  // Only run the code if the profile initialization has finished completely
+  // (either completed successfully, or has failed).
+  if (status == Profile::CREATE_STATUS_INITIALIZED ||
+      status == Profile::CREATE_STATUS_LOCAL_FAIL ||

[Alexei Svitkine (slow), 2013/06/19 14:58:31]

Should we actually be doing the recursion in the fail status case?

From what I remember, we couldn't get it to happen in the tests, but it seems
wrong to set prefs::kProfileLastUsed to last_non_managed_profile_path if it
failed to load.

[noms (inactive), 2013/06/21 23:11:47]

I've added a DCHECK. 
On 2013/06/19 14:58:31, Alexei Svitkine wrote:

+      status == Profile::CREATE_STATUS_REMOTE_FAIL) {
+    if (IsProfileMarkedForDeletion(last_non_managed_profile_path)) {
+      // If the profile we tried to load as the next active profile has been
+      // deleted, then retry deleting this profile to redo the logic and load

[Alexei Svitkine (slow), 2013/06/19 14:58:31]

nit: "and load" -> "to load"

[noms (inactive), 2013/06/21 23:11:47]

Done.

+      // the next available profile.
+      ScheduleProfileForDeletion(profile_to_delete_dir, original_callback);
+    } else {
+      // Update the local state as promised in the ScheduleProfileForDeletion.
+      g_browser_process->local_state()->SetString(
+          prefs::kProfileLastUsed,
+          last_non_managed_profile_path.BaseName().MaybeAsASCII());
+      FinishDeletingProfile(profile_to_delete_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);
 
@@ -1094,6 +1147,12 @@ void ProfileManager::ScheduleProfileForDeletion(
   cache.DeleteProfileFromCache(profile_dir);
 }
 
+bool ProfileManager::IsProfileMarkedForDeletion(

[Alexei Svitkine (slow), 2013/06/19 14:58:31]

Move this to the anon namespace at the top of the file instead.

[noms (inactive), 2013/06/21 23:11:47]

Done.

+    const base::FilePath& profile_dir) {
+  return std::find(ProfilesToDelete().begin(), ProfilesToDelete().end(),
+      profile_dir) != ProfilesToDelete().end();
+}
+
 // static
 bool ProfileManager::IsMultipleProfilesEnabled() {
 #if defined(OS_ANDROID)