Reland r318288: Wait until a new profile has been created before deleting the active profile.

Previous review URL:
https://codereview.chromium.org/953453002

BUG=460859
TBR=rsesek@chromium.org

Review URL: https://codereview.chromium.org/962143002

Cr-Commit-Position: refs/heads/master@{#318775}

chrome/browser/app_controller_mac.mm

@@ -897,7 +897,9 @@ class AppControllerProfileObserver : public ProfileInfoCacheObserver {
   // already loaded a new one, so the pointer needs to be updated;
   // otherwise we will try to start up a browser window with a pointer
   // to the old profile.
-  if (lastProfile_ && profilePath == lastProfile_->GetPath())
+  // In a browser test, the application is not brought to the front, so
+  // |lastProfile_| might be null.
+  if (!lastProfile_ || profilePath == lastProfile_->GetPath())
     lastProfile_ = g_browser_process->profile_manager()->GetLastUsedProfile();
 
   auto it = profileBookmarkMenuBridgeMap_.find(profilePath);

chrome/browser/profiles/profile_manager.cc

@@ -558,7 +558,7 @@ Profile* ProfileManager::GetProfileByPathInternal(
 Profile* ProfileManager::GetProfileByPath(const base::FilePath& path) const {
   TRACE_EVENT0("browser", "ProfileManager::GetProfileByPath");
   ProfileInfo* profile_info = GetProfileInfoByPath(path);
-  return profile_info && profile_info->created ? profile_info->profile.get()
+  return (profile_info && profile_info->created) ? profile_info->profile.get()
                                                  : nullptr;
 }
 
@@ -650,7 +650,6 @@ void ProfileManager::ScheduleProfileForDeletion(
     service->CancelDownloads();
   }
 
-  PrefService* local_state = g_browser_process->local_state();
   ProfileInfoCache& cache = GetProfileInfoCache();
 
   // If we're deleting the last (non-legacy-supervised) profile, then create a
@@ -682,32 +681,28 @@ void ProfileManager::ScheduleProfileForDeletion(
 
     new_path = GenerateNextProfileDirectoryPath();
     CreateProfileAsync(new_path,
-                       callback,
+                       base::Bind(&ProfileManager::OnNewActiveProfileLoaded,
+                                  base::Unretained(this),
+                                  profile_dir,
+                                  new_path,
+                                  callback),
                        new_profile_name,
                        new_avatar_url,
                        std::string());
 
     ProfileMetrics::LogProfileAddNewUser(
         ProfileMetrics::ADD_NEW_USER_LAST_DELETED);
+    return;
   }
 
-  // Update the last used profile pref before closing browser windows. This
-  // way the correct last used profile is set for any notification observers.
+#if defined(OS_MACOSX)
+  // 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.
   const std::string last_used_profile =
-      local_state->GetString(prefs::kProfileLastUsed);
+      g_browser_process->local_state()->GetString(prefs::kProfileLastUsed);
   if (last_used_profile == profile_dir.BaseName().MaybeAsASCII() ||
       last_used_profile == GetGuestProfilePath().BaseName().MaybeAsASCII()) {
-    const std::string last_non_supervised_profile =
-        last_non_supervised_profile_path.BaseName().MaybeAsASCII();
-    if (last_non_supervised_profile.empty()) {
-      DCHECK(!new_path.empty());
-      local_state->SetString(prefs::kProfileLastUsed,
-                             new_path.BaseName().MaybeAsASCII());
-    } 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_supervised_profile_path,
                        base::Bind(&ProfileManager::OnNewActiveProfileLoaded,
                                   base::Unretained(this),
@@ -718,15 +713,10 @@ void ProfileManager::ScheduleProfileForDeletion(
                        base::string16(),
                        std::string());
     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_supervised_profile);
-#endif
-    }
   }
-  FinishDeletingProfile(profile_dir);
+#endif  // defined(OS_MACOSX)
+
+  FinishDeletingProfile(profile_dir, last_non_supervised_profile_path);
 }
 
 // static
@@ -1160,7 +1150,15 @@ Profile* ProfileManager::CreateAndInitializeProfile(
   return profile;
 }
 
-void ProfileManager::FinishDeletingProfile(const base::FilePath& profile_dir) {
+void ProfileManager::FinishDeletingProfile(
+    const base::FilePath& profile_dir,
+    const base::FilePath& new_active_profile_dir) {
+  // Update the last used profile pref before closing browser windows. This
+  // way the correct last used profile is set for any notification observers.
+  g_browser_process->local_state()->SetString(
+      prefs::kProfileLastUsed,
+      new_active_profile_dir.BaseName().MaybeAsASCII());
+
   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.
@@ -1372,10 +1370,9 @@ void ProfileManager::BrowserListObserver::OnBrowserSetLastActive(
 }
 #endif  // !defined(OS_ANDROID) && !defined(OS_IOS)
 
-#if defined(OS_MACOSX)
 void ProfileManager::OnNewActiveProfileLoaded(
     const base::FilePath& profile_to_delete_path,
-    const base::FilePath& last_non_supervised_profile_path,
+    const base::FilePath& new_active_profile_path,
     const CreateCallback& original_callback,
     Profile* loaded_profile,
     Profile::CreateStatus status) {
@@ -1383,22 +1380,21 @@ void ProfileManager::OnNewActiveProfileLoaded(
          status != Profile::CREATE_STATUS_REMOTE_FAIL);
 
   // Only run the code if the profile initialization has finished completely.
-  if (status == Profile::CREATE_STATUS_INITIALIZED) {
-    if (IsProfileMarkedForDeletion(last_non_supervised_profile_path)) {
+  if (status != Profile::CREATE_STATUS_INITIALIZED)
+    return;
+
+  if (IsProfileMarkedForDeletion(new_active_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 to load
     // the next available profile.
     ScheduleProfileForDeletion(profile_to_delete_path, original_callback);
-    } else {
-      // Update the local state as promised in the ScheduleProfileForDeletion.
-      g_browser_process->local_state()->SetString(
-          prefs::kProfileLastUsed,
-          last_non_supervised_profile_path.BaseName().MaybeAsASCII());
-      FinishDeletingProfile(profile_to_delete_path);
-    }
+    return;
   }
+
+  FinishDeletingProfile(profile_to_delete_path, new_active_profile_path);
+  if (!original_callback.is_null())
+    original_callback.Run(loaded_profile, status);
 }
-#endif
 
 ProfileManagerWithoutInit::ProfileManagerWithoutInit(
     const base::FilePath& user_data_dir) : ProfileManager(user_data_dir) {

chrome/browser/profiles/profile_manager.h

@@ -265,8 +265,10 @@ class ProfileManager : public base::NonThreadSafe,
   // creation and adds it to the set managed by this ProfileManager.
   Profile* CreateAndInitializeProfile(const base::FilePath& profile_dir);
 
-  // Schedules the profile at the given path to be deleted on shutdown.
-  void FinishDeletingProfile(const base::FilePath& profile_dir);
+  // Schedules the profile at the given path to be deleted on shutdown,
+  // and marks the new profile as active.
+  void FinishDeletingProfile(const base::FilePath& profile_dir,
+                             const base::FilePath& new_active_profile_dir);
 
   // Registers profile with given info. Returns pointer to created ProfileInfo
   // entry.
@@ -317,7 +319,6 @@ class ProfileManager : public base::NonThreadSafe,
   };
 #endif  // !defined(OS_ANDROID) && !defined(OS_IOS)
 
-#if defined(OS_MACOSX)
   // If the |loaded_profile| has been loaded successfully (according to
   // |status|) and isn't already scheduled for deletion, then finishes adding
   // |profile_to_delete_dir| to the queue of profiles to be deleted, and updates
@@ -329,7 +330,6 @@ class ProfileManager : public base::NonThreadSafe,
       const CreateCallback& original_callback,
       Profile* loaded_profile,
       Profile::CreateStatus status);
-#endif
 
   content::NotificationRegistrar registrar_;
 

chrome/browser/profiles/profile_manager_browsertest.cc

@@ -40,10 +40,11 @@ const ProfileManager::CreateCallback kOnProfileSwitchDoNothing;
 
 // An observer that returns back to test code after a new profile is
 // initialized.
-void OnUnblockOnProfileCreation(Profile* profile,
+void OnUnblockOnProfileCreation(base::RunLoop* run_loop,
+                                Profile* profile,
                                 Profile::CreateStatus status) {
   if (status == Profile::CREATE_STATUS_INITIALIZED)
-    base::MessageLoop::current()->Quit();
+    run_loop->Quit();
 }
 
 void ProfileCreationComplete(Profile* profile, Profile::CreateStatus status) {
@@ -91,26 +92,26 @@ class ProfileRemovalObserver : public ProfileInfoCacheObserver {
 
 // The class serves to retrieve passwords from PasswordStore asynchronously. It
 // used by ProfileManagerBrowserTest.DeletePasswords on some platforms.
-class PasswordStoreConsumerVerifier :
-    public password_manager::PasswordStoreConsumer {
+class PasswordStoreConsumerVerifier
+    : public password_manager::PasswordStoreConsumer {
  public:
-  PasswordStoreConsumerVerifier() : called_(false) {}
-
   void OnGetPasswordStoreResults(
       ScopedVector<autofill::PasswordForm> results) override {
-    EXPECT_FALSE(called_);
-    called_ = true;
     password_entries_.swap(results);
+    run_loop_.Quit();
   }
 
-  bool IsCalled() const { return called_; }
+  void Wait() {
+    run_loop_.Run();
+  }
 
   const std::vector<autofill::PasswordForm*>& GetPasswords() const {
     return password_entries_.get();
   }
+
  private:
+  base::RunLoop run_loop_;
   ScopedVector<autofill::PasswordForm> password_entries_;
-  bool called_;
 };
 
 static base::FilePath GetFirstNonSigninProfile(const ProfileInfoCache& cache) {
@@ -160,20 +161,23 @@ IN_PROC_BROWSER_TEST_F(ProfileManagerBrowserTest, DeleteSingletonProfile) {
   // Delete singleton profile.
   base::FilePath singleton_profile_path = cache.GetPathOfProfileAtIndex(0);
   EXPECT_FALSE(singleton_profile_path.empty());
-  profile_manager->ScheduleProfileForDeletion(singleton_profile_path,
-                                              ProfileManager::CreateCallback());
+  base::RunLoop run_loop;
+  profile_manager->ScheduleProfileForDeletion(
+      singleton_profile_path,
+      base::Bind(&OnUnblockOnProfileCreation, &run_loop));
 
-  // Spin things till profile is actually deleted.
-  content::RunAllPendingInMessageLoop();
+  // Run the message loop until the profile is actually deleted (as indicated
+  // by the callback above being called).
+  run_loop.Run();
 
   // Make sure a new profile was created automatically.
   EXPECT_EQ(cache.GetNumberOfProfiles(), 1U);
   base::FilePath new_profile_path = cache.GetPathOfProfileAtIndex(0);
-  EXPECT_NE(new_profile_path, singleton_profile_path);
+  EXPECT_NE(new_profile_path.value(), singleton_profile_path.value());
 
   // Make sure that last used profile preference is set correctly.
   Profile* last_used = ProfileManager::GetLastUsedProfile();
-  EXPECT_EQ(new_profile_path, last_used->GetPath());
+  EXPECT_EQ(new_profile_path.value(), last_used->GetPath().value());
 
   // Make sure the last used profile was set correctly before the notification
   // was sent.
@@ -191,14 +195,14 @@ IN_PROC_BROWSER_TEST_F(ProfileManagerBrowserTest, DISABLED_DeleteAllProfiles) {
 
   // Create an additional profile.
   base::FilePath new_path = profile_manager->GenerateNextProfileDirectoryPath();
-  profile_manager->CreateProfileAsync(new_path,
-                                      base::Bind(&OnUnblockOnProfileCreation),
-                                      base::string16(), base::string16(),
-                                      std::string());
+  base::RunLoop run_loop;
+  profile_manager->CreateProfileAsync(
+      new_path, base::Bind(&OnUnblockOnProfileCreation, &run_loop),
+      base::string16(), base::string16(), std::string());
 
-  // Spin to allow profile creation to take place, loop is terminated
-  // by OnUnblockOnProfileCreation when the profile is created.
-  content::RunMessageLoop();
+  // Run the message loop to allow profile creation to take place; the loop is
+  // terminated by OnUnblockOnProfileCreation when the profile is created.
+  run_loop.Run();
 
   ASSERT_EQ(cache.GetNumberOfProfiles(), 2U);
 
@@ -306,14 +310,14 @@ IN_PROC_BROWSER_TEST_F(ProfileManagerBrowserTest,
   // Create an additional profile.
   base::FilePath path_profile2 =
       profile_manager->GenerateNextProfileDirectoryPath();
-  profile_manager->CreateProfileAsync(path_profile2,
-                                      base::Bind(&OnUnblockOnProfileCreation),
-                                      base::string16(), base::string16(),
-                                      std::string());
+  base::RunLoop run_loop;
+  profile_manager->CreateProfileAsync(
+      path_profile2, base::Bind(&OnUnblockOnProfileCreation, &run_loop),
+      base::string16(), base::string16(), std::string());
 
-  // Spin to allow profile creation to take place, loop is terminated
-  // by OnUnblockOnProfileCreation when the profile is created.
-  content::RunMessageLoop();
+  // Run the message loop to allow profile creation to take place; the loop is
+  // terminated by OnUnblockOnProfileCreation when the profile is created.
+  run_loop.Run();
 
   chrome::HostDesktopType desktop_type = chrome::GetActiveDesktop();
   BrowserList* browser_list = BrowserList::GetInstance(desktop_type);
@@ -445,27 +449,18 @@ IN_PROC_BROWSER_TEST_F(ProfileManagerBrowserTest, DeletePasswords) {
   password_store->AddLogin(form);
   PasswordStoreConsumerVerifier verify_add;
   password_store->GetAutofillableLogins(&verify_add);
+  verify_add.Wait();
+  EXPECT_EQ(1u, verify_add.GetPasswords().size());
 
   ProfileManager* profile_manager = g_browser_process->profile_manager();
-  profile_manager->ScheduleProfileForDeletion(profile->GetPath(),
-                                              ProfileManager::CreateCallback());
-  content::RunAllPendingInMessageLoop();
-  PasswordStoreConsumerVerifier verify_delete;
-  password_store->GetAutofillableLogins(&verify_delete);
-
-  // Run the password background thread.
   base::RunLoop run_loop;
-  base::Closure task = base::Bind(
-      base::IgnoreResult(&content::BrowserThread::PostTask),
-      content::BrowserThread::UI,
-      FROM_HERE,
-      run_loop.QuitClosure());
-  EXPECT_TRUE(password_store->ScheduleTask(task));
+  profile_manager->ScheduleProfileForDeletion(
+      profile->GetPath(), base::Bind(&OnUnblockOnProfileCreation, &run_loop));
   run_loop.Run();
 
-  EXPECT_TRUE(verify_add.IsCalled());
-  EXPECT_EQ(1u, verify_add.GetPasswords().size());
-  EXPECT_TRUE(verify_delete.IsCalled());
+  PasswordStoreConsumerVerifier verify_delete;
+  password_store->GetAutofillableLogins(&verify_delete);
+  verify_delete.Wait();
   EXPECT_EQ(0u, verify_delete.GetPasswords().size());
 }
 #endif  // !defined(OS_WIN) && !defined(OS_ANDROID) && !defined(OS_CHROMEOS)