Commit f5966937 authored by mgiuca's avatar mgiuca Committed by Commit bot

Fixed app launcher crashing trying to load guest profile.

Previously, if the user has never used the app launcher, and it loads
while in a guest mode browser window, the browser would crash. This was
particularly bad because it could happen during startup on Windows, even
without explicitly opening the launcher (due to app list warmup).

Now if the last used profile is the guest profile, just uses the
default.

Added DCHECKs on app list profile loads to ensure they are not guest
profiles.

BUG=468812

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

Cr-Commit-Position: refs/heads/master@{#330712}
parent c8088647
...@@ -210,6 +210,16 @@ void RecordAppListDiscoverability(PrefService* local_state, ...@@ -210,6 +210,16 @@ void RecordAppListDiscoverability(PrefService* local_state,
AppListService::ENABLE_NUM_ENABLE_SOURCES); AppListService::ENABLE_NUM_ENABLE_SOURCES);
} }
// Checks whether a profile name is valid for the app list. Returns false if the
// name is empty, or represents a guest profile.
bool IsValidProfileName(const std::string& profile_name) {
if (profile_name.empty())
return false;
return profile_name !=
base::FilePath(chrome::kGuestProfileDir).AsUTF8Unsafe();
}
} // namespace } // namespace
void AppListServiceImpl::RecordAppListLaunch() { void AppListServiceImpl::RecordAppListLaunch() {
...@@ -301,14 +311,20 @@ void AppListServiceImpl::SetProfilePath(const base::FilePath& profile_path) { ...@@ -301,14 +311,20 @@ void AppListServiceImpl::SetProfilePath(const base::FilePath& profile_path) {
void AppListServiceImpl::CreateShortcut() {} void AppListServiceImpl::CreateShortcut() {}
std::string AppListServiceImpl::GetProfileName() { std::string AppListServiceImpl::GetProfileName() {
const std::string app_list_profile = std::string app_list_profile =
local_state_->GetString(prefs::kAppListProfile); local_state_->GetString(prefs::kAppListProfile);
if (!app_list_profile.empty()) if (IsValidProfileName(app_list_profile))
return app_list_profile; return app_list_profile;
// If the user has no profile preference for the app launcher, default to the // If the user has no profile preference for the app launcher, default to the
// last browser profile used. // last browser profile used.
return profile_store_->GetLastUsedProfileName(); app_list_profile = profile_store_->GetLastUsedProfileName();
if (IsValidProfileName(app_list_profile))
return app_list_profile;
// If the last profile used was invalid (ie, guest profile), use the initial
// profile.
return chrome::kInitialProfile;
} }
void AppListServiceImpl::OnProfileWillBeRemoved( void AppListServiceImpl::OnProfileWillBeRemoved(
......
...@@ -346,6 +346,12 @@ void AppListServiceMac::Init(Profile* initial_profile) { ...@@ -346,6 +346,12 @@ void AppListServiceMac::Init(Profile* initial_profile) {
void AppListServiceMac::InitWithProfilePath( void AppListServiceMac::InitWithProfilePath(
Profile* initial_profile, Profile* initial_profile,
const base::FilePath& profile_path) { const base::FilePath& profile_path) {
// App list profiles should not be off-the-record. It is currently possible to
// get here in an off-the-record profile via the Web Store
// (http://crbug.com/416380).
// TODO(mgiuca): DCHECK that requested_profile->IsOffTheRecord() and
// requested_profile->IsGuestSession() are false, once that is resolved.
// On Mac, Init() is called multiple times for a process: any time there is no // On Mac, Init() is called multiple times for a process: any time there is no
// browser window open and a new window is opened, and during process startup // browser window open and a new window is opened, and during process startup
// to handle the silent launch case (e.g. for app shims). In the startup case, // to handle the silent launch case (e.g. for app shims). In the startup case,
......
...@@ -169,6 +169,31 @@ TEST_F(AppListServiceUnitTest, ...@@ -169,6 +169,31 @@ TEST_F(AppListServiceUnitTest,
EXPECT_EQ(0, service_->destroy_app_list_call_count()); EXPECT_EQ(0, service_->destroy_app_list_call_count());
} }
TEST_F(AppListServiceUnitTest, RefusesToLoadGuestAppListProfile) {
// Unlikely, but if somehow the user's app_list.profile pref was set to the
// guest profile, make sure we refuse to load it (or it would crash).
local_state_->SetString(
prefs::kAppListProfile,
base::FilePath(chrome::kGuestProfileDir).MaybeAsASCII());
local_state_->SetString(prefs::kProfileLastUsed, "last-used");
base::FilePath last_used_profile_path =
user_data_dir_.AppendASCII("last-used");
EXPECT_EQ(last_used_profile_path,
service_->GetProfilePath(profile_store_->GetUserDataDir()));
}
TEST_F(AppListServiceUnitTest, RefusesToLoadGuestLastUsedProfile) {
// If the user's most recent browser session was a guest session, make sure we
// do not open a guest profile in the launcher (which would crash).
local_state_->SetString(
prefs::kProfileLastUsed,
base::FilePath(chrome::kGuestProfileDir).MaybeAsASCII());
base::FilePath initial_profile_path =
user_data_dir_.AppendASCII(chrome::kInitialProfile);
EXPECT_EQ(initial_profile_path,
service_->GetProfilePath(profile_store_->GetUserDataDir()));
}
TEST_F(AppListServiceUnitTest, SwitchingProfilesPersists) { TEST_F(AppListServiceUnitTest, SwitchingProfilesPersists) {
profile_store_->LoadProfile(profile1_.get()); profile_store_->LoadProfile(profile1_.get());
profile_store_->LoadProfile(profile2_.get()); profile_store_->LoadProfile(profile2_.get());
......
...@@ -30,6 +30,12 @@ void AppListServiceViews::Init(Profile* initial_profile) { ...@@ -30,6 +30,12 @@ void AppListServiceViews::Init(Profile* initial_profile) {
} }
void AppListServiceViews::ShowForProfile(Profile* requested_profile) { void AppListServiceViews::ShowForProfile(Profile* requested_profile) {
// App list profiles should not be off-the-record. It is currently possible to
// get here in an off-the-record profile via the Web Store
// (http://crbug.com/416380).
// TODO(mgiuca): DCHECK that requested_profile->IsOffTheRecord() and
// requested_profile->IsGuestSession() are false, once that is resolved.
ShowForProfileInternal(requested_profile, ShowForProfileInternal(requested_profile,
app_list::AppListModel::INVALID_STATE); app_list::AppListModel::INVALID_STATE);
} }
......
...@@ -271,6 +271,12 @@ void AppListServiceWin::ShowForProfile(Profile* requested_profile) { ...@@ -271,6 +271,12 @@ void AppListServiceWin::ShowForProfile(Profile* requested_profile) {
} }
void AppListServiceWin::OnLoadProfileForWarmup(Profile* initial_profile) { void AppListServiceWin::OnLoadProfileForWarmup(Profile* initial_profile) {
// App list profiles should not be off-the-record. It is currently possible to
// get here in an off-the-record profile via the Web Store
// (http://crbug.com/416380).
// TODO(mgiuca): DCHECK that requested_profile->IsOffTheRecord() and
// requested_profile->IsGuestSession() are false, once that is resolved.
if (!IsWarmupNeeded()) if (!IsWarmupNeeded())
return; return;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment