Commit 3bbe0ce7 authored by mgiuca's avatar mgiuca Committed by Commit bot

Fix Launcher crash in ChromeOS guest mode.

The CHECK-fail was introduced in r315272 (64121b7f), which switched the
app list to run with the "original profile" rather than the incognito
profile. Partially reverted that CL so that the app list runs with the
incognito profile, and fixed StartPageService to correctly use the
incognito profile (rather than being null when the app list profile is
incognito), which addresses the original issue
(http://crbug.com/440484).

Had to work around DCHECK on shutdown in StartPageService due to
crbug.com/463419.

Added a DCHECK to catch use of non-incognito profiles in guest mode
early (without having to select a search result to trigger the crash).
Added AppListControllerGuestModeBrowserTest to test opening the app list
in guest mode.

BUG=460437,440484

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

Cr-Commit-Position: refs/heads/master@{#319036}
parent d6edee7b
...@@ -15,7 +15,9 @@ ...@@ -15,7 +15,9 @@
#include "chrome/browser/ui/browser_finder.h" #include "chrome/browser/ui/browser_finder.h"
#include "chrome/browser/ui/host_desktop.h" #include "chrome/browser/ui/host_desktop.h"
#include "chrome/common/chrome_paths.h" #include "chrome/common/chrome_paths.h"
#include "chrome/common/chrome_switches.h"
#include "chrome/test/base/in_process_browser_test.h" #include "chrome/test/base/in_process_browser_test.h"
#include "chrome/test/base/testing_profile.h"
#include "ui/app_list/app_list_model.h" #include "ui/app_list/app_list_model.h"
#include "ui/app_list/app_list_switches.h" #include "ui/app_list/app_list_switches.h"
#include "ui/app_list/search_box_model.h" #include "ui/app_list/search_box_model.h"
...@@ -23,6 +25,11 @@ ...@@ -23,6 +25,11 @@
#include "ui/app_list/search_result_observer.h" #include "ui/app_list/search_result_observer.h"
#include "ui/base/models/list_model_observer.h" #include "ui/base/models/list_model_observer.h"
#if defined(OS_CHROMEOS)
#include "chromeos/chromeos_switches.h"
#include "chromeos/login/user_names.h"
#endif // defined(OS_CHROMEOS)
// Browser Test for AppListController that runs on all platforms supporting // Browser Test for AppListController that runs on all platforms supporting
// app_list. // app_list.
typedef InProcessBrowserTest AppListControllerBrowserTest; typedef InProcessBrowserTest AppListControllerBrowserTest;
...@@ -212,3 +219,37 @@ IN_PROC_BROWSER_TEST_F(AppListControllerSearchResultsBrowserTest, ...@@ -212,3 +219,37 @@ IN_PROC_BROWSER_TEST_F(AppListControllerSearchResultsBrowserTest,
StopWatchingResults(); StopWatchingResults();
service->DismissAppList(); service->DismissAppList();
} }
#if defined(OS_CHROMEOS)
class AppListControllerGuestModeBrowserTest : public InProcessBrowserTest {
public:
AppListControllerGuestModeBrowserTest() {}
protected:
void SetUpCommandLine(base::CommandLine* command_line) override;
private:
DISALLOW_COPY_AND_ASSIGN(AppListControllerGuestModeBrowserTest);
};
void AppListControllerGuestModeBrowserTest::SetUpCommandLine(
base::CommandLine* command_line) {
command_line->AppendSwitch(chromeos::switches::kGuestSession);
command_line->AppendSwitchASCII(chromeos::switches::kLoginUser,
chromeos::login::kGuestUserName);
command_line->AppendSwitchASCII(chromeos::switches::kLoginProfile,
TestingProfile::kTestUserProfileDir);
command_line->AppendSwitch(switches::kIncognito);
}
// Test creating the initial app list in guest mode.
IN_PROC_BROWSER_TEST_F(AppListControllerGuestModeBrowserTest, Incognito) {
AppListService* service = test::GetAppListService();
EXPECT_TRUE(service->GetCurrentAppListProfile());
service->ShowForProfile(browser()->profile());
EXPECT_EQ(browser()->profile(), service->GetCurrentAppListProfile());
}
#endif // defined(OS_CHROMEOS)
...@@ -261,7 +261,7 @@ AppListServiceImpl::~AppListServiceImpl() {} ...@@ -261,7 +261,7 @@ AppListServiceImpl::~AppListServiceImpl() {}
AppListViewDelegate* AppListServiceImpl::GetViewDelegate(Profile* profile) { AppListViewDelegate* AppListServiceImpl::GetViewDelegate(Profile* profile) {
if (!view_delegate_) if (!view_delegate_)
view_delegate_.reset(new AppListViewDelegate(GetControllerDelegate())); view_delegate_.reset(new AppListViewDelegate(GetControllerDelegate()));
view_delegate_->SetProfile(profile->GetOriginalProfile()); view_delegate_->SetProfile(profile);
return view_delegate_.get(); return view_delegate_.get();
} }
......
...@@ -285,6 +285,13 @@ void AppListViewDelegate::SetProfile(Profile* new_profile) { ...@@ -285,6 +285,13 @@ void AppListViewDelegate::SetProfile(Profile* new_profile) {
return; return;
} }
// If we are in guest mode, the new profile should be an incognito profile.
// Otherwise, this may later hit a check (same condition as this one) in
// Browser::Browser when opening links in a browser window (see
// http://crbug.com/460437).
DCHECK(!profile_->IsGuestSession() || profile_->IsOffTheRecord())
<< "Guest mode must use incognito profile";
// TODO(vadimt): Remove ScopedTracker below once crbug.com/431326 is fixed. // TODO(vadimt): Remove ScopedTracker below once crbug.com/431326 is fixed.
tracked_objects::ScopedTracker tracking_profile3( tracked_objects::ScopedTracker tracking_profile3(
FROM_HERE_WITH_EXPLICIT_FUNCTION( FROM_HERE_WITH_EXPLICIT_FUNCTION(
......
...@@ -86,6 +86,15 @@ class StartPageService::ProfileDestroyObserver ...@@ -86,6 +86,15 @@ class StartPageService::ProfileDestroyObserver
public: public:
explicit ProfileDestroyObserver(StartPageService* service) explicit ProfileDestroyObserver(StartPageService* service)
: service_(service) { : service_(service) {
if (service_->profile()->IsOffTheRecord()) {
// We need to be notified when the original profile gets destroyed as well
// as the OTR profile, because the original profile will be destroyed
// first, and a DCHECK at that time ensures that the OTR profile has 0
// hosts. See http://crbug.com/463419.
registrar_.Add(
this, chrome::NOTIFICATION_PROFILE_DESTROYED,
content::Source<Profile>(service_->profile()->GetOriginalProfile()));
}
registrar_.Add(this, registrar_.Add(this,
chrome::NOTIFICATION_PROFILE_DESTROYED, chrome::NOTIFICATION_PROFILE_DESTROYED,
content::Source<Profile>(service_->profile())); content::Source<Profile>(service_->profile()));
...@@ -98,7 +107,9 @@ class StartPageService::ProfileDestroyObserver ...@@ -98,7 +107,9 @@ class StartPageService::ProfileDestroyObserver
const content::NotificationSource& source, const content::NotificationSource& source,
const content::NotificationDetails& details) override { const content::NotificationDetails& details) override {
DCHECK_EQ(chrome::NOTIFICATION_PROFILE_DESTROYED, type); DCHECK_EQ(chrome::NOTIFICATION_PROFILE_DESTROYED, type);
DCHECK_EQ(service_->profile(), content::Source<Profile>(source).ptr()); DCHECK(service_->profile()->IsSameProfile(
content::Source<Profile>(source).ptr()));
registrar_.RemoveAll();
service_->Shutdown(); service_->Shutdown();
} }
......
...@@ -55,7 +55,7 @@ KeyedService* StartPageServiceFactory::BuildServiceInstanceFor( ...@@ -55,7 +55,7 @@ KeyedService* StartPageServiceFactory::BuildServiceInstanceFor(
content::BrowserContext* StartPageServiceFactory::GetBrowserContextToUse( content::BrowserContext* StartPageServiceFactory::GetBrowserContextToUse(
content::BrowserContext* context) const { content::BrowserContext* context) const {
// The start page service needs an instance in ChromeOS guest mode. // The start page service needs an instance in ChromeOS guest mode.
return chrome::GetBrowserContextRedirectedInIncognito(context); return chrome::GetBrowserContextOwnInstanceInIncognito(context);
} }
} // namespace app_list } // namespace app_list
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