Skip to content
This repository has been archived by the owner on Apr 3, 2020. It is now read-only.

Commit

Permalink
Wait until a new profile has been created before deleting the active …
Browse files Browse the repository at this point in the history 
…profile.

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

BUG=460859
[email protected]

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

Cr-Commit-Position: refs/heads/master@{#318775}
(cherry picked from commit 447f643)

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

Cr-Commit-Position: refs/branch-heads/2311@{#162}
Cr-Branched-From: 09b7de5-refs/heads/master@{#317474}
  • Loading branch information
@sheepmaster
sheepmaster committed Mar 6, 2015
1 parent a066654 commit 5ffd605
Show file tree
Hide file tree
Showing 4 changed files with 97 additions and 104 deletions.
4 changes: 3 additions & 1 deletion chrome/browser/app_controller_mac.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -897,7 +897,9 @@ - (void)profileWasRemoved:(const base::FilePath&)profilePath {
// 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);
Expand Down
102 changes: 49 additions & 53 deletions chrome/browser/profiles/profile_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -562,8 +562,8 @@ 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()
: nullptr;
return (profile_info && profile_info->created) ? profile_info->profile.get()
: nullptr;
}

// static
Expand Down Expand Up @@ -654,7 +654,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
Expand Down Expand Up @@ -686,51 +685,42 @@ 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),
profile_dir,
last_non_supervised_profile_path,
callback),
base::string16(),
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
}
CreateProfileAsync(last_non_supervised_profile_path,
base::Bind(&ProfileManager::OnNewActiveProfileLoaded,
base::Unretained(this),
profile_dir,
last_non_supervised_profile_path,
callback),
base::string16(),
base::string16(),
std::string());
return;
}
FinishDeletingProfile(profile_dir);
#endif // defined(OS_MACOSX)

FinishDeletingProfile(profile_dir, last_non_supervised_profile_path);
}

// static
Expand Down Expand Up @@ -1163,7 +1153,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.
Expand Down Expand Up @@ -1375,33 +1373,31 @@ 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) {
DCHECK(status != Profile::CREATE_STATUS_LOCAL_FAIL &&
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 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);
}
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);
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) {
Expand Down
8 changes: 4 additions & 4 deletions chrome/browser/profiles/profile_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand All @@ -329,7 +330,6 @@ class ProfileManager : public base::NonThreadSafe,
const CreateCallback& original_callback,
Profile* loaded_profile,
Profile::CreateStatus status);
#endif

content::NotificationRegistrar registrar_;

Expand Down
87 changes: 41 additions & 46 deletions chrome/browser/profiles/profile_manager_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,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) {
Expand Down Expand Up @@ -90,26 +91,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_;
};

} // namespace
Expand Down Expand Up @@ -143,20 +144,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.
Expand All @@ -174,14 +178,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);

Expand Down Expand Up @@ -288,14 +292,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);
Expand Down Expand Up @@ -426,27 +430,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)

0 comments on commit 5ffd605

Please sign in to comment.