Fix Profile* lifetime issues in Chrome's AppListViewDelegate

chrome/browser/ui/app_list/app_list_view_delegate.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/app_list/app_list_view_delegate.h"
 
 #include <vector>
 
 #include "apps/custom_launcher_page_contents.h"
 #include "base/callback.h"
@@ skipping 136 matching lines @@
                  << extensions::manifest_keys::kLauncherPage
                  << " has no 'page' attribute; will be ignored.";
       continue;
     }
     urls->push_back(extension->GetResourceURL(launcher_page_page));
   }
 }
 
 }  // namespace
 
-AppListViewDelegate::AppListViewDelegate(Profile* profile,
-                                         AppListControllerDelegate* controller)
-    : controller_(controller),
-      profile_(profile),
+AppListViewDelegate::AppListViewDelegate(
+    AppListControllerDelegate* controller_delegate)
+    : controller_(controller_delegate),
+      profile_(NULL),
       model_(NULL),
       scoped_observer_(this) {
   CHECK(controller_);
   // The SigninManagerFactor and the SigninManagers are observed to keep the
   // profile switcher menu up to date, with the correct list of profiles and the
   // correct email address (or none for signed out users) for each.
   SigninManagerFactory::GetInstance()->AddObserver(this);
 
   // Start observing all already-created SigninManagers.
   ProfileManager* profile_manager = g_browser_process->profile_manager();
   std::vector<Profile*> profiles = profile_manager->GetLoadedProfiles();
   for (std::vector<Profile*>::iterator i = profiles.begin();
        i != profiles.end();
        ++i) {
     SigninManagerBase* manager =
         SigninManagerFactory::GetForProfileIfExists(*i);
     if (manager) {
       DCHECK(!scoped_observer_.IsObserving(manager));
       scoped_observer_.Add(manager);
     }
   }
 
   profile_manager->GetProfileInfoCache().AddObserver(this);
+}
 
-  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.

[Matt Giuca, 2014/08/25 04:48:46]

Should you duplicate this comment where |search_controller_| is declared in the
header file?

[tapted, 2014/08/25 06:17:00]

Done.

+    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(
       *ui::ResourceBundle::GetSharedInstance().
       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_));
+}
 
-  // Set up the custom launcher pages.
+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());
+}
+
+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) {
     std::string extension_id = it->host();
     apps::CustomLauncherPageContents* page_contents =
         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) {
       OnSpeechRecognitionStateChanged(
           app_list::SPEECH_RECOGNITION_HOTWORD_LISTENING);
     }
   } else {
     if (speech_ui_->state() == app_list::SPEECH_RECOGNITION_HOTWORD_LISTENING)
       OnSpeechRecognitionStateChanged(app_list::SPEECH_RECOGNITION_READY);
   }
 }
 
 void AppListViewDelegate::OnHotwordRecognized() {
   DCHECK_EQ(app_list::SPEECH_RECOGNITION_HOTWORD_LISTENING,
             speech_ui_->state());
   ToggleSpeechRecognition();
 }
 
 void AppListViewDelegate::SigninManagerCreated(SigninManagerBase* manager) {
   scoped_observer_.Add(manager);
 }
 
 void AppListViewDelegate::SigninManagerShutdown(SigninManagerBase* manager) {
   if (scoped_observer_.IsObserving(manager))
     scoped_observer_.Remove(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();
+void AppListViewDelegate::OnProfileAdded(const base::FilePath& profile_path) {
+  SetUpProfileSwitcher();
+}
 
-  search_controller_.reset(new app_list::SearchController(
-      profile_, model_->search_box(), model_->results(),
-      speech_ui_.get(), controller_));
+void AppListViewDelegate::OnProfileWasRemoved(
+    const base::FilePath& profile_path,
+    const base::string16& profile_name) {
+  SetUpProfileSwitcher();
+}
 
-#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;
-
-  // Populate the app list users.
-  PopulateUsers(g_browser_process->profile_manager()->GetProfileInfoCache(),
-                profile_->GetPath(), &users_);
-
-  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 {
   return controller_->ForceNativeDesktop();
 }
 
 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() {
   return model_;
 }
 
 app_list::SpeechUIModel* AppListViewDelegate::GetSpeechUI() {
   return speech_ui_.get();
 }
 
@@ skipping 166 matching lines @@
       HotwordService::IsExperimentalHotwordingEnabled() &&
       service && service->HotwordEnabled()) {
     HotwordService* hotword_service =
         HotwordServiceFactory::GetForProfile(profile_);
     if (hotword_service) {
       hotword_service->RequestHotwordSession(this);
     }
   }
 }
 
-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) {
   app_list::StartPageService* service =
       app_list::StartPageService::Get(profile_);
   if (!service)
     return NULL;
 
   content::WebContents* web_contents = service->GetStartPageContents();
   if (!web_contents)
@@ skipping 70 matching lines @@
 
 void AppListViewDelegate::AddObserver(
     app_list::AppListViewDelegateObserver* observer) {
   observers_.AddObserver(observer);
 }
 
 void AppListViewDelegate::RemoveObserver(
     app_list::AppListViewDelegateObserver* observer) {
   observers_.RemoveObserver(observer);
 }