Fix Profile* lifetime issues in Chrome's AppListViewDelegate

Currently AppListViewDelegate can hold on to references to a destroyed
Profile*. It's managed to escape crashing in most cases so far because
the LocalState pref is updated for the next time the app launcher is
shown. However, if the profile the app launcher is first created for is
ever deleted in the same session, then a crash usually follows (but
doesn't always create a crash dump, due to a corrupt stack).

This decouples the Profile from the AppListViewDelegate constructor to
make it clear the lifetimes are not in step. Then "SetProfile" correctly
tears down any references to an old profile, before setting a new one.

When the app list's active profile is deleted, the AppListViewDelegate
is destroyed by forcibly closing/destroying the AppList's widget via a
new method AppListServiceImpl::DestroyAppList().

BUG=392763, 403647, 373689, 405827
TEST=(windows) Show the app list, right-click an app and "uninstall".
Leave the uninstall dialog open. Switch to a chrome://settings in a
browser and delete the profile being shown in the app list. App list
should close.

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

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

chrome/browser/ui/app_list/app_list_service_impl.cc

@@ -283,17 +283,31 @@ void AppListServiceImpl::SetProfilePath(const base::FilePath& profile_path) {
 
 void AppListServiceImpl::CreateShortcut() {}
 
-// We need to watch for profile removal to keep kAppListProfile updated.
 void AppListServiceImpl::OnProfileWillBeRemoved(
     const base::FilePath& profile_path) {
-  // If the profile the app list uses just got deleted, reset it to the last
-  // used profile.
+  // We need to watch for profile removal to keep kAppListProfile updated, for
+  // the case that the deleted profile is being used by the app list.
   std::string app_list_last_profile = local_state_->GetString(
       prefs::kAppListProfile);
-  if (profile_path.BaseName().MaybeAsASCII() == app_list_last_profile) {
-    local_state_->SetString(prefs::kAppListProfile,
-        local_state_->GetString(prefs::kProfileLastUsed));
-  }
+  if (profile_path.BaseName().MaybeAsASCII() != app_list_last_profile)
+    return;
+
+  // Switch the app list over to a valid profile.
+  // Before ProfileInfoCache::DeleteProfileFromCache() calls this function,
+  // ProfileManager::ScheduleProfileForDeletion() will have checked to see if
+  // the deleted profile was also "last used", and updated that setting with
+  // something valid.
+  local_state_->SetString(prefs::kAppListProfile,
+                          local_state_->GetString(prefs::kProfileLastUsed));
+
+  // The Chrome AppListViewDelegate now needs to be torn down, because:
+  //  1. it has many references to the profile and can't be profile-keyed, and
+  //  2. the last used profile might not be loaded yet.
+  //    - this loading is sometimes done by the ProfileManager asynchronously,
+  //      so the app list can't just switch to that.
+  // Currently, the AppListViewDelegate is owned by the platform-specific
+  // AppListView, so just force-close the window.
+  DestroyAppList();
 }
 
 void AppListServiceImpl::Show() {

chrome/browser/ui/app_list/app_list_service_impl.h

@@ -53,6 +53,10 @@ class AppListServiceImpl : public AppListService,
  protected:
   AppListServiceImpl();
 
+  // Destroy the app list. Called when the profile that the app list is showing
+  // is being deleted.
+  virtual void DestroyAppList() = 0;
+
   void InvalidatePendingProfileLoads();
   ProfileLoader& profile_loader() { return *profile_loader_; }
   const ProfileLoader& profile_loader() const { return *profile_loader_; }

chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc

@@ -70,12 +70,14 @@ class AppListServiceInteractiveTest : public InProcessBrowserTest {
     DISABLED_SwitchAppListProfilesDuringSearch
 #define MAYBE_ShowAppListNonDefaultProfile \
     DISABLED_ShowAppListNonDefaultProfile
+#define MAYBE_DeleteShowingAppList DISABLED_DeleteShowingAppList
 #else
 #define MAYBE_ShowAndDismiss ShowAndDismiss
 #define MAYBE_SwitchAppListProfiles SwitchAppListProfiles
 #define MAYBE_SwitchAppListProfilesDuringSearch \
     SwitchAppListProfilesDuringSearch
 #define MAYBE_ShowAppListNonDefaultProfile ShowAppListNonDefaultProfile
+#define MAYBE_DeleteShowingAppList DeleteShowingAppList
 #endif
 
 // Show the app list, then dismiss it.
@@ -260,3 +262,26 @@ IN_PROC_BROWSER_TEST_F(ShowAppListNonDefaultInteractiveTest,
 
   service->DismissAppList();
 }
+
+// Test showing the app list for a profile then deleting that profile while the
+// app list is visible.
+IN_PROC_BROWSER_TEST_F(ShowAppListNonDefaultInteractiveTest,
+                       MAYBE_DeleteShowingAppList) {
+  AppListService* service = test::GetAppListService();
+  EXPECT_TRUE(service->IsAppListVisible());
+  EXPECT_EQ(second_profile_name_.value(),
+            service->GetCurrentAppListProfile()->GetPath().BaseName().value());
+
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+
+  // Create a browser for the Default profile.
+  CreateBrowser(profile_manager->GetLastUsedProfile());
+
+  // Delete the profile being used by the app list.
+  profile_manager->ScheduleProfileForDeletion(
+      service->GetCurrentAppListProfile()->GetPath(),
+      ProfileManager::CreateCallback());
+
+  // App Launcher should get closed immediately and nothing should explode.
+  EXPECT_FALSE(service->IsAppListVisible());
+}

chrome/browser/ui/app_list/app_list_service_mac.h

@@ -64,6 +64,9 @@ class AppListServiceMac : public AppListServiceImpl,
   virtual Profile* GetCurrentAppListProfile() OVERRIDE;
   virtual void CreateShortcut() OVERRIDE;
 
+  // AppListServiceImpl overrides:
+  virtual void DestroyAppList() OVERRIDE;
+
   // AppShimHandler overrides:
   virtual void OnShimLaunch(apps::AppShimHandler::Host* host,
                             apps::AppShimLaunchType launch_type,

chrome/browser/ui/app_list/app_list_service_mac.mm

@@ -452,6 +452,17 @@ void AppListServiceMac::CreateShortcut() {
       g_browser_process->profile_manager()->user_data_dir()));
 }
 
+void AppListServiceMac::DestroyAppList() {
+  // Due to reference counting, Mac can't guarantee that the widget is deleted,
+  // but mac supports a visible app list with a NULL profile, so there's also no
+  // need to tear it down completely.
+  DismissAppList();
+  [[window_controller_ appListViewController]
+      setDelegate:scoped_ptr<app_list::AppListViewDelegate>()];
+
+  profile_ = NULL;
+}
+
 NSWindow* AppListServiceMac::GetAppListWindow() {
   return [window_controller_ window];
 }

chrome/browser/ui/app_list/app_list_service_unittest.cc

@@ -26,16 +26,22 @@ class TestingAppListServiceImpl : public AppListServiceImpl {
                             PrefService* local_state,
                             scoped_ptr<ProfileStore> profile_store)
       : AppListServiceImpl(command_line, local_state, profile_store.Pass()),
-        showing_for_profile_(NULL) {}
+        showing_for_profile_(NULL),
+        destroy_app_list_call_count_(0) {}
 
   Profile* showing_for_profile() const {
     return showing_for_profile_;
   }
 
+  int destroy_app_list_call_count() const {
+    return destroy_app_list_call_count_;
+  }
+
   void PerformStartupChecks(Profile* profile) {
     AppListServiceImpl::PerformStartupChecks(profile);
   }
 
+  // AppListService overrides:
   virtual Profile* GetCurrentAppListProfile() OVERRIDE {
     // We don't return showing_for_profile_ here because that is only defined if
     // the app list is visible.
@@ -66,8 +72,14 @@ class TestingAppListServiceImpl : public AppListServiceImpl {
     return NULL;
   }
 
+  // AppListServiceImpl overrides:
+  virtual void DestroyAppList() OVERRIDE { ++destroy_app_list_call_count_; }
+
  private:
   Profile* showing_for_profile_;
+  int destroy_app_list_call_count_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestingAppListServiceImpl);
 };
 
 class AppListServiceUnitTest : public testing::Test {
@@ -145,11 +157,18 @@ TEST_F(AppListServiceUnitTest,
        RemovedProfileResetsToLastUsedProfileIfExists) {
   local_state_->SetString(prefs::kProfileLastUsed, "last-used");
   EnableAppList();
+  EXPECT_EQ(0, service_->destroy_app_list_call_count());
   profile_store_->RemoveProfile(profile1_.get());
+
   base::FilePath last_used_profile_path =
       user_data_dir_.AppendASCII("last-used");
   EXPECT_EQ(last_used_profile_path,
             service_->GetProfilePath(profile_store_->GetUserDataDir()));
+
+  // Ensure a tear-down was triggered, since there would be references to the
+  // destroyed Profile, and the last-used profile could be getting loaded
+  // asynchronously.
+  EXPECT_EQ(1, service_->destroy_app_list_call_count());
 }
 
 TEST_F(AppListServiceUnitTest, SwitchingProfilesPersists) {

chrome/browser/ui/app_list/app_list_service_views.cc

@@ -6,6 +6,7 @@
 
 #include "chrome/browser/ui/app_list/app_list_controller_delegate.h"
 #include "chrome/browser/ui/app_list/scoped_keep_alive.h"
+#include "ui/app_list/views/app_list_view.h"
 
 AppListServiceViews::AppListServiceViews(
     scoped_ptr<AppListControllerDelegate> controller_delegate)
@@ -63,6 +64,16 @@ AppListControllerDelegate* AppListServiceViews::GetControllerDelegate() {
   return controller_delegate_.get();
 }
 
+void AppListServiceViews::DestroyAppList() {
+  if (!shower_.HasView())
+    return;
+
+  // Use CloseNow(). This can't be asynchronous because the profile will be
+  // deleted once this function returns.
+  shower_.app_list()->GetWidget()->CloseNow();
+  DCHECK(!shower_.HasView());
+}
+
 AppListControllerDelegate*
 AppListServiceViews::GetControllerDelegateForCreate() {
   return controller_delegate_.get();

chrome/browser/ui/app_list/app_list_service_views.h

@@ -40,6 +40,9 @@ class AppListServiceViews : public AppListServiceImpl,
   virtual Profile* GetCurrentAppListProfile() OVERRIDE;
   virtual AppListControllerDelegate* GetControllerDelegate() OVERRIDE;
 
+  // AppListServiceImpl overrides:
+  virtual void DestroyAppList() OVERRIDE;
+
   // AppListShowerDelegate overrides:
   virtual AppListControllerDelegate* GetControllerDelegateForCreate() OVERRIDE;
 

chrome/browser/ui/app_list/app_list_view_delegate.cc

@@ -157,7 +157,7 @@ void GetCustomLauncherPageUrls(content::BrowserContext* browser_context,
 AppListViewDelegate::AppListViewDelegate(Profile* profile,
                                          AppListControllerDelegate* controller)
     : controller_(controller),
-      profile_(profile),
+      profile_(NULL),
       model_(NULL),
       scoped_observer_(this) {
   CHECK(controller_);
@@ -181,11 +181,64 @@ AppListViewDelegate::AppListViewDelegate(Profile* profile,
   }
 
   profile_manager->GetProfileInfoCache().AddObserver(this);
+  SetProfile(profile);
+}
 
-  app_list::StartPageService* service =
+AppListViewDelegate::~AppListViewDelegate() {
+  SetProfile(NULL);
+  g_browser_process->profile_manager()->GetProfileInfoCache().RemoveObserver(
+      this);
+
+  SigninManagerFactory* factory = SigninManagerFactory::GetInstance();
+  if (factory)
+    factory->RemoveObserver(this);
+}
+
+void AppListViewDelegate::SetProfile(Profile* new_profile) {
+  if (profile_) {
+    // Note: |search_controller_| has a reference to |speech_ui_| so must be
+    // destroyed first.
+    search_controller_.reset();
+    speech_ui_.reset();
+    custom_page_contents_.clear();
+    app_list::StartPageService* start_page_service =
+        app_list::StartPageService::Get(profile_);
+    if (start_page_service)
+      start_page_service->RemoveObserver(this);
+#if defined(USE_ASH)
+    app_sync_ui_state_watcher_.reset();
+#endif
+    model_ = NULL;
+  }
+
+  profile_ = new_profile;
+  if (!profile_)
+    return;
+
+  model_ =
+      app_list::AppListSyncableServiceFactory::GetForProfile(profile_)->model();
+
+#if defined(USE_ASH)
+  app_sync_ui_state_watcher_.reset(new AppSyncUIStateWatcher(profile_, model_));
+#endif
+
+  SetUpSearchUI();
+  SetUpProfileSwitcher();
+  SetUpCustomLauncherPages();
+
+  // Clear search query.
+  model_->search_box()->SetText(base::string16());
+}
+
+void AppListViewDelegate::SetUpSearchUI() {
+  app_list::StartPageService* start_page_service =
       app_list::StartPageService::Get(profile_);
+  if (start_page_service)
+    start_page_service->AddObserver(this);
+
   speech_ui_.reset(new app_list::SpeechUIModel(
-      service ? service->state() : app_list::SPEECH_RECOGNITION_OFF));
+      start_page_service ? start_page_service->state()
+                         : app_list::SPEECH_RECOGNITION_OFF));
 
 #if defined(GOOGLE_CHROME_BUILD)
   speech_ui_->set_logo(
@@ -193,13 +246,32 @@ AppListViewDelegate::AppListViewDelegate(Profile* profile,
       GetImageSkiaNamed(IDR_APP_LIST_GOOGLE_LOGO_VOICE_SEARCH));
 #endif
 
-  OnProfileChanged();  // sets model_
-  if (service)
-    service->AddObserver(this);
+  search_controller_.reset(new app_list::SearchController(profile_,
+                                                          model_->search_box(),
+                                                          model_->results(),
+                                                          speech_ui_.get(),
+                                                          controller_));
+}
+
+void AppListViewDelegate::SetUpProfileSwitcher() {
+  // Don't populate the app list users if we are on the ash desktop.
+  chrome::HostDesktopType desktop = chrome::GetHostDesktopTypeForNativeWindow(
+      controller_->GetAppListWindow());
+  if (desktop == chrome::HOST_DESKTOP_TYPE_ASH)
+    return;
+
+  // Populate the app list users.
+  PopulateUsers(g_browser_process->profile_manager()->GetProfileInfoCache(),
+                profile_->GetPath(),
+                &users_);
+
+  FOR_EACH_OBSERVER(
+      app_list::AppListViewDelegateObserver, observers_, OnProfilesChanged());
+}
 
-  // Set up the custom launcher pages.
+void AppListViewDelegate::SetUpCustomLauncherPages() {
   std::vector<GURL> custom_launcher_page_urls;
-  GetCustomLauncherPageUrls(profile, &custom_launcher_page_urls);
+  GetCustomLauncherPageUrls(profile_, &custom_launcher_page_urls);
   for (std::vector<GURL>::const_iterator it = custom_launcher_page_urls.begin();
        it != custom_launcher_page_urls.end();
        ++it) {
@@ -208,27 +280,11 @@ AppListViewDelegate::AppListViewDelegate(Profile* profile,
         new apps::CustomLauncherPageContents(
             scoped_ptr<extensions::AppDelegate>(new ChromeAppDelegate),
             extension_id);
-    page_contents->Initialize(profile, *it);
+    page_contents->Initialize(profile_, *it);
     custom_page_contents_.push_back(page_contents);
   }
 }
 
-AppListViewDelegate::~AppListViewDelegate() {
-  app_list::StartPageService* service =
-      app_list::StartPageService::Get(profile_);
-  if (service)
-    service->RemoveObserver(this);
-  g_browser_process->
-      profile_manager()->GetProfileInfoCache().RemoveObserver(this);
-
-  SigninManagerFactory* factory = SigninManagerFactory::GetInstance();
-  if (factory)
-    factory->RemoveObserver(this);
-
-  // Ensure search controller is released prior to speech_ui_.
-  search_controller_.reset();
-}
-
 void AppListViewDelegate::OnHotwordStateChanged(bool started) {
   if (started) {
     if (speech_ui_->state() == app_list::SPEECH_RECOGNITION_READY) {
@@ -258,43 +314,32 @@ void AppListViewDelegate::SigninManagerShutdown(SigninManagerBase* manager) {
 
 void AppListViewDelegate::GoogleSigninFailed(
     const GoogleServiceAuthError& error) {
-  OnProfileChanged();
+  SetUpProfileSwitcher();
 }
 
 void AppListViewDelegate::GoogleSigninSucceeded(const std::string& username,
                                                 const std::string& password) {
-  OnProfileChanged();
+  SetUpProfileSwitcher();
 }
 
 void AppListViewDelegate::GoogleSignedOut(const std::string& username) {
-  OnProfileChanged();
+  SetUpProfileSwitcher();
 }
 
-void AppListViewDelegate::OnProfileChanged() {
-  model_ = app_list::AppListSyncableServiceFactory::GetForProfile(
-      profile_)->model();
-
-  search_controller_.reset(new app_list::SearchController(
-      profile_, model_->search_box(), model_->results(),
-      speech_ui_.get(), controller_));
-
-#if defined(USE_ASH)
-  app_sync_ui_state_watcher_.reset(new AppSyncUIStateWatcher(profile_, model_));
-#endif
-
-  // Don't populate the app list users if we are on the ash desktop.
-  chrome::HostDesktopType desktop = chrome::GetHostDesktopTypeForNativeWindow(
-      controller_->GetAppListWindow());
-  if (desktop == chrome::HOST_DESKTOP_TYPE_ASH)
-    return;
+void AppListViewDelegate::OnProfileAdded(const base::FilePath& profile_path) {
+  SetUpProfileSwitcher();
+}
 
-  // Populate the app list users.
-  PopulateUsers(g_browser_process->profile_manager()->GetProfileInfoCache(),
-                profile_->GetPath(), &users_);
+void AppListViewDelegate::OnProfileWasRemoved(
+    const base::FilePath& profile_path,
+    const base::string16& profile_name) {
+  SetUpProfileSwitcher();
+}
 
-  FOR_EACH_OBSERVER(app_list::AppListViewDelegateObserver,
-                    observers_,
-                    OnProfilesChanged());
+void AppListViewDelegate::OnProfileNameChanged(
+    const base::FilePath& profile_path,
+    const base::string16& old_profile_name) {
+  SetUpProfileSwitcher();
 }
 
 bool AppListViewDelegate::ForceNativeDesktop() const {
@@ -303,16 +348,9 @@ bool AppListViewDelegate::ForceNativeDesktop() const {
 
 void AppListViewDelegate::SetProfileByPath(const base::FilePath& profile_path) {
   DCHECK(model_);
-
   // The profile must be loaded before this is called.
-  profile_ =
-      g_browser_process->profile_manager()->GetProfileByPath(profile_path);
-  DCHECK(profile_);
-
-  OnProfileChanged();
-
-  // Clear search query.
-  model_->search_box()->SetText(base::string16());
+  SetProfile(
+      g_browser_process->profile_manager()->GetProfileByPath(profile_path));
 }
 
 app_list::AppListModel* AppListViewDelegate::GetModel() {
@@ -499,21 +537,6 @@ void AppListViewDelegate::OnSpeechRecognitionStateChanged(
   }
 }
 
-void AppListViewDelegate::OnProfileAdded(const base::FilePath& profile_path) {
-  OnProfileChanged();
-}
-
-void AppListViewDelegate::OnProfileWasRemoved(
-    const base::FilePath& profile_path, const base::string16& profile_name) {
-  OnProfileChanged();
-}
-
-void AppListViewDelegate::OnProfileNameChanged(
-    const base::FilePath& profile_path,
-    const base::string16& old_profile_name) {
-  OnProfileChanged();
-}
-
 #if defined(TOOLKIT_VIEWS)
 views::View* AppListViewDelegate::CreateStartPageWebView(
     const gfx::Size& size) {

chrome/browser/ui/app_list/app_list_view_delegate.h

@@ -52,13 +52,24 @@ class AppListViewDelegate : public app_list::AppListViewDelegate,
                             public SigninManagerBase::Observer,
                             public SigninManagerFactory::Observer {
  public:
+  // Constructs Chrome's AppListViewDelegate, initially for |profile|.
+  // Does not take ownership of |controller|. TODO(tapted): It should.
   AppListViewDelegate(Profile* profile,
                       AppListControllerDelegate* controller);
   virtual ~AppListViewDelegate();
 
  private:
-  // Updates the app list's current profile and ProfileMenuItems.
-  void OnProfileChanged();
+  // Configure the AppList for the given |profile|.
+  void SetProfile(Profile* profile);
+
+  // Updates the speech webview and start page for the current |profile_|.
+  void SetUpSearchUI();
+
+  // Updates the app list's ProfileMenuItems for the current |profile_|.
+  void SetUpProfileSwitcher();
+
+  // Updates the app list's custom launcher pages for the current |profile_|.
+  void SetUpCustomLauncherPages();
 
   // Overridden from app_list::AppListViewDelegate:
   virtual bool ForceNativeDesktop() const OVERRIDE;
@@ -130,7 +141,6 @@ class AppListViewDelegate : public app_list::AppListViewDelegate,
       const base::FilePath& profile_path,
       const base::string16& old_profile_name) OVERRIDE;
 
-  scoped_ptr<app_list::SearchController> search_controller_;
   // Unowned pointer to the controller.
   AppListControllerDelegate* controller_;
   // Unowned pointer to the associated profile. May change if SetProfileByPath
@@ -140,7 +150,9 @@ class AppListViewDelegate : public app_list::AppListViewDelegate,
   // if |profile_| changes.
   app_list::AppListModel* model_;
 
+  // Note: order ensures |search_controller_| is destroyed before |speech_ui_|.
   scoped_ptr<app_list::SpeechUIModel> speech_ui_;
+  scoped_ptr<app_list::SearchController> search_controller_;
 
   base::TimeDelta auto_launch_timeout_;
 

chrome/browser/ui/ash/app_list/app_list_service_ash.cc

@@ -63,6 +63,12 @@ AppListControllerDelegate* AppListServiceAsh::GetControllerDelegate() {
   return controller_delegate_.get();
 }
 
+void AppListServiceAsh::DestroyAppList() {
+  // On Ash, the app list is torn down whenever it is dismissed, so just ensure
+  // that it is dismissed.
+  DismissAppList();
+}
+
 // Windows and Linux Ash additionally supports a native UI. See
 // app_list_service_{win,linux}.cc.
 #if defined(OS_CHROMEOS)

chrome/browser/ui/ash/app_list/app_list_service_ash.h

@@ -36,6 +36,9 @@ class AppListServiceAsh : public AppListServiceImpl {
   virtual Profile* GetCurrentAppListProfile() OVERRIDE;
   virtual AppListControllerDelegate* GetControllerDelegate() OVERRIDE;
 
+  // ApplistServiceImpl overrides:
+  virtual void DestroyAppList() OVERRIDE;
+
   scoped_ptr<AppListControllerDelegateAsh> controller_delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(AppListServiceAsh);