Observe chrome::NOTIFICATION_APP_TERMINATING in Chrome's AppListViewDelegate

chrome/browser/ui/app_list/app_list_view_delegate.cc

Index: chrome/browser/ui/app_list/app_list_view_delegate.cc
diff --git a/chrome/browser/ui/app_list/app_list_view_delegate.cc b/chrome/browser/ui/app_list/app_list_view_delegate.cc
index b7327eacb0087770dbae3129bc2520d0223a6f57..1156e4f92327d5e745974c91c9298420c5ad638d 100644
--- a/chrome/browser/ui/app_list/app_list_view_delegate.cc
+++ b/chrome/browser/ui/app_list/app_list_view_delegate.cc
@@ -35,6 +35,7 @@
 #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"
@@ -181,9 +182,16 @@ AppListViewDelegate::AppListViewDelegate(Profile* profile,
           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);
@@ -194,6 +202,9 @@ AppListViewDelegate::~AppListViewDelegate() {
 }
 
 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.
@@ -248,6 +259,11 @@ void AppListViewDelegate::SetUpSearchUI() {
 }
 
 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());
@@ -439,6 +455,9 @@ void AppListViewDelegate::Dismiss()  {
 void AppListViewDelegate::ViewClosing() {
   controller_->ViewClosing();
 
+  if (!profile_)
+    return;
+
   app_list::StartPageService* service =
       app_list::StartPageService::Get(profile_);
   if (service) {
@@ -623,3 +642,20 @@ 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();
+  }
+}