Observe chrome::NOTIFICATION_APP_TERMINATING 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 17 matching lines @@
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/chrome_pages.h"
 #include "chrome/browser/ui/host_desktop.h"
 #include "chrome/browser/ui/scoped_tabbed_browser_displayer.h"
 #include "chrome/browser/web_applications/web_app.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/url_constants.h"
 #include "components/signin/core/browser/signin_manager.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/notification_service.h"
 #include "content/public/browser/page_navigator.h"
 #include "content/public/browser/user_metrics.h"
 #include "content/public/browser/web_contents.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/extension_set.h"
 #include "extensions/common/manifest_constants.h"
 #include "extensions/common/manifest_handlers/launcher_page_info.h"
 #include "grit/theme_resources.h"
 #include "ui/app_list/app_list_switches.h"
@@ skipping 126 matching lines @@
 
   profile_manager->GetProfileInfoCache().AddObserver(this);
   speech_ui_.reset(new app_list::SpeechUIModel);
 
 #if defined(GOOGLE_CHROME_BUILD)
   speech_ui_->set_logo(
       *ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
           IDR_APP_LIST_GOOGLE_LOGO_VOICE_SEARCH));
 #endif
   SetProfile(profile);
+
+  registrar_.Add(this,
+                 chrome::NOTIFICATION_APP_TERMINATING,
+                 content::NotificationService::AllSources());
 }
 
 AppListViewDelegate::~AppListViewDelegate() {
+  // Note that the destructor is not always called. E.g. on Mac, this is owned
+  // by a leaky singleton. Essential shutdown work must be done by observing
+  // chrome::NOTIFICATION_APP_TERMINATING.
   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_ == new_profile)

[Matt Giuca, 2014/09/11 03:50:37]

Are these profile checks (this one, and the next two) related to the main change
here (notification observing and proper shutdown)? They seem unrelated.

Do they change any behaviour (like fixing any crashes)? If so, I would put them
up in a separate CL. If not, then it's fine.

[tapted, 2014/09/11 04:57:45]

They're needed, but only due to this change. Previously SetProfile(NULL) was
only called from the destructor, so nothing bad could result. It's still called
from there on platforms where it is not leaky whenever the window is closed (and
it may or may not already be NULL), and things like profile destruction and
widget teardown happen after SetProfile(NULL) is [now] called in Observe().

[Matt Giuca, 2014/09/11 08:17:58]

Acknowledged.

+    return;
+
   if (profile_) {
     // Note: |search_controller_| has a reference to |speech_ui_| so must be
     // destroyed first.
     search_controller_.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)
@@ skipping 34 matching lines @@
                                             : app_list::SPEECH_RECOGNITION_OFF);
 
   search_controller_.reset(new app_list::SearchController(profile_,
                                                           model_->search_box(),
                                                           model_->results(),
                                                           speech_ui_.get(),
                                                           controller_));
 }
 
 void AppListViewDelegate::SetUpProfileSwitcher() {
+  // If a profile change is observed when there is no app list, there is nothing
+  // to update until SetProfile() calls this function again.
+  if (!profile_)
+    return;
+
   // 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_);
@@ skipping 171 matching lines @@
   }
 }
 
 void AppListViewDelegate::Dismiss()  {
   controller_->DismissView();
 }
 
 void AppListViewDelegate::ViewClosing() {
   controller_->ViewClosing();
 
+  if (!profile_)
+    return;
+
   app_list::StartPageService* service =
       app_list::StartPageService::Get(profile_);
   if (service) {
     service->AppListHidden();
     if (service->HotwordEnabled()) {
       HotwordService* hotword_service =
           HotwordServiceFactory::GetForProfile(profile_);
       if (hotword_service)
         hotword_service->StopHotwordSession(this);
     }
@@ skipping 164 matching lines @@
 
 void AppListViewDelegate::AddObserver(
     app_list::AppListViewDelegateObserver* observer) {
   observers_.AddObserver(observer);
 }
 
 void AppListViewDelegate::RemoveObserver(
     app_list::AppListViewDelegateObserver* observer) {
   observers_.RemoveObserver(observer);
 }
+
+void AppListViewDelegate::Observe(int type,
+                                  const content::NotificationSource& source,
+                                  const content::NotificationDetails& details) {
+  switch (type) {
+    case chrome::NOTIFICATION_APP_TERMINATING:
+      SetProfile(NULL);  // Ensures launcher page web contents are torn down.
+
+      // Stop observing SigninManagers. SigninManagerFactory is not a leaky
+      // singleton like this, so will get offended without the following.

[Matt Giuca, 2014/09/11 03:50:37]

Cute :) But can you be clear instead of saying "get offended".

suggest: "SigninManagerFactory is not a leaky singleton (unlike this class), and
its destructor will check that it has no remaining observers."

[tapted, 2014/09/11 04:57:45]

Done.

+      scoped_observer_.RemoveAll();
+      SigninManagerFactory::GetInstance()->RemoveObserver(this);
+      break;
+    default:
+      NOTREACHED();
+  }
+}