[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 a1ed001fe17b19e9026519dde70ace801855abf0..9eed51881d166956e12a5057e56493e903df49ec 100644
--- a/chrome/browser/profiles/profile_manager.cc
+++ b/chrome/browser/profiles/profile_manager.cc
@@ -1068,6 +1068,9 @@ void ProfileManager::ScheduleProfileForDeletion(
     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() ==
@@ -1079,6 +1082,7 @@ void ProfileManager::ScheduleProfileForDeletion(
       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;
       }
     }
@@ -1102,9 +1106,39 @@ void ProfileManager::ScheduleProfileForDeletion(
                          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);