For Mac, add AppListViewDelegateObserver::OnShutdown; deflake tests

We used to just leak everything on Mac. Since r295194, we clean up
references to Profiles being deleted, in Chrome's AppListViewDelegate.
This has caught out the Mac app list trying to access stuff it shouldn't
if Cocoa is still drawing it while Chrome is shutting down. Tests
started failing flakily.

This CL adds AppListViewDelegateObserver::OnShutdown to that Chrome's
AppListViewDelegate can notify the AppListView that it's shutting down.

On Views platforms, the Widget is just closed (which also happens just
after the chrome::NOTIFICATION_APP_TERMINATING broadcast). However, (a)
Cocoa doesn't have a `CloseAllSecondaryWidgets` call to trigger this and
(b) just closing the NSWindow isn't usually enough on Cocoa to avoid
accessing C++ objects due to reference counting.

We could also go through the platform-specific AppListService (e.g. by
passing in the AppListService that creates a Chrome
AppListViewDelegate), but this seemed neater.

This de-flakes a bunch of applist tests on Mac. The Mac-specific
ShowAppListUsingShim test is also augmented with extra checks for the
OnShutdown stuff.

BUG=415264
TEST=AppListServiceMacInteractiveTest.ShowAppListUsingShim,
AppListServiceInteractiveTest.*

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

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

10 files changed, 77 insertions, 12 deletions

diff --git a/chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc b/chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc
index 4ee32b3..e354cb9 100644
--- a/chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc
+++ b/chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc
@@ -46,24 +46,30 @@ class AppListServiceInteractiveTest : public InProcessBrowserTest {
 
 // ChromeOS does not support ShowForProfile(), or profile switching within the
 // app list. Profile switching on CrOS goes through a different code path.
-// The tests are flaky on Mac, so disable there too. See http://crbug.com/415264
-#if defined(OS_CHROMEOS) || defined(OS_MACOSX)
+#if defined(OS_CHROMEOS)
 #define MAYBE_ShowAndDismiss DISABLED_ShowAndDismiss
 #define MAYBE_SwitchAppListProfiles DISABLED_SwitchAppListProfiles
-#define MAYBE_SwitchAppListProfilesDuringSearch \
-    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
 
+// SwitchAppListProfilesDuringSearch disabled on ChromeOS for reasons above.
+// Disabled on Mac due to an AppKit bug which makes it flaky in rare cases.
+// http://crbug.com/417148.
+#if defined(OS_CHROMEOS) || defined(OS_MACOSX)
+#define MAYBE_SwitchAppListProfilesDuringSearch \
+    DISABLED_SwitchAppListProfilesDuringSearch
+#else
+#define MAYBE_SwitchAppListProfilesDuringSearch \
+    SwitchAppListProfilesDuringSearch
+#endif
+
 // Show the app list, then dismiss it.
 IN_PROC_BROWSER_TEST_F(AppListServiceInteractiveTest, MAYBE_ShowAndDismiss) {
   AppListService* service = test::GetAppListService();
diff --git a/chrome/browser/ui/app_list/app_list_service_mac.h b/chrome/browser/ui/app_list/app_list_service_mac.h
index b9aa2c8..e18b744 100644
--- a/chrome/browser/ui/app_list/app_list_service_mac.h
+++ b/chrome/browser/ui/app_list/app_list_service_mac.h
@@ -23,6 +23,10 @@ class Display;
 class Point;
 }
 
+namespace test {
+class AppListServiceMacTestApi;
+}
+
 // AppListServiceMac manages global resources needed for the app list to
 // operate, and controls when the app list is opened and closed.
 class AppListServiceMac : public AppListServiceImpl,
@@ -80,6 +84,7 @@ class AppListServiceMac : public AppListServiceImpl,
   virtual void OnShimQuit(apps::AppShimHandler::Host* host) OVERRIDE;
 
  private:
+  friend class test::AppListServiceMacTestApi;
   friend struct DefaultSingletonTraits<AppListServiceMac>;
 
   AppListServiceMac();
diff --git a/chrome/browser/ui/app_list/app_list_service_mac_interactive_uitest.mm b/chrome/browser/ui/app_list/app_list_service_mac_interactive_uitest.mm
index 959271c..17f5b4e 100644
--- a/chrome/browser/ui/app_list/app_list_service_mac_interactive_uitest.mm
+++ b/chrome/browser/ui/app_list/app_list_service_mac_interactive_uitest.mm
@@ -2,7 +2,7 @@
 // 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_service_mac.h"
+#import "chrome/browser/ui/app_list/app_list_service_mac.h"
 
 #include <vector>
 
@@ -12,9 +12,21 @@
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/mac/app_mode_common.h"
 #include "chrome/test/base/in_process_browser_test.h"
+#import "ui/app_list/cocoa/app_list_window_controller.h"
 
 using apps::AppShimHandler;
 
+namespace test {
+
+class AppListServiceMacTestApi {
+ public:
+  static AppListWindowController* window_controller() {
+    return AppListServiceMac::GetInstance()->window_controller_;
+  }
+};
+
+}  // namespace test
+
 namespace {
 
 // Browser test for mac-specific AppListService functionality.
@@ -32,6 +44,23 @@ class AppListServiceMacInteractiveTest : public InProcessBrowserTest,
                      std::vector<base::FilePath>());
   }
 
+  AppListViewController* GetViewController() {
+    return [test::AppListServiceMacTestApi::window_controller()
+        appListViewController];
+  }
+
+  // testing::Test overrides:
+  virtual void TearDown() OVERRIDE {
+    // At tear-down, NOTIFICATION_APP_TERMINATING should have been sent for the
+    // browser shutdown. References to browser-owned objects must be removed
+    // from the app list UI.
+    AppListViewController* view_controller = GetViewController();
+    // Note this first check will fail if the test doesn't ever show the
+    // app list, but currently all tests in this file do.
+    EXPECT_TRUE(view_controller);
+    EXPECT_FALSE([view_controller delegate]);
+  }
+
   // AppShimHandler::Host overrides:
   virtual void OnAppLaunchComplete(apps::AppShimLaunchResult result) OVERRIDE {
     // AppList shims are always given APP_SHIM_LAUNCH_DUPLICATE_HOST, indicating
@@ -61,9 +90,7 @@ class AppListServiceMacInteractiveTest : public InProcessBrowserTest,
 
 }  // namespace
 
-// Flaky on Mac. See https://crbug.com/415264
-IN_PROC_BROWSER_TEST_F(AppListServiceMacInteractiveTest,
-                       DISABLED_ShowAppListUsingShim) {
+IN_PROC_BROWSER_TEST_F(AppListServiceMacInteractiveTest, ShowAppListUsingShim) {
   // Check that AppListService has registered as a shim handler for "app_list".
   EXPECT_TRUE(AppShimHandler::GetForAppMode(app_mode::kAppListModeId));
 
@@ -71,6 +98,9 @@ IN_PROC_BROWSER_TEST_F(AppListServiceMacInteractiveTest,
       AppListService::Get(chrome::HOST_DESKTOP_TYPE_NATIVE);
   EXPECT_FALSE(service->IsAppListVisible());
 
+  // Creation should be lazy.
+  EXPECT_FALSE(GetViewController());
+
   // With no saved profile, the default profile should be chosen and saved.
   service->Show();
   EXPECT_EQ(browser()->profile(), service->GetCurrentAppListProfile());
@@ -96,4 +126,9 @@ IN_PROC_BROWSER_TEST_F(AppListServiceMacInteractiveTest,
   EXPECT_EQ(3, launch_count_);
   service->DismissAppList();
   EXPECT_FALSE(service->IsAppListVisible());
+
+  // View sticks around until shutdown.
+  AppListViewController* view_controller = GetViewController();
+  EXPECT_TRUE(view_controller);
+  EXPECT_TRUE([view_controller delegate]);
 }
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 af012b2..ab45f72 100644
--- a/chrome/browser/ui/app_list/app_list_view_delegate.cc
+++ b/chrome/browser/ui/app_list/app_list_view_delegate.cc
@@ -646,6 +646,9 @@ void AppListViewDelegate::Observe(int type,
                                   const content::NotificationDetails& details) {
   switch (type) {
     case chrome::NOTIFICATION_APP_TERMINATING:
+      FOR_EACH_OBSERVER(
+          app_list::AppListViewDelegateObserver, observers_, OnShutdown());
+
       SetProfile(NULL);  // Ensures launcher page web contents are torn down.
 
       // SigninManagerFactory is not a leaky singleton (unlike this class), and
diff --git a/ui/app_list/app_list_item_list.h b/ui/app_list/app_list_item_list.h
index 66b6553..2e67f1c 100644
--- a/ui/app_list/app_list_item_list.h
+++ b/ui/app_list/app_list_item_list.h
@@ -102,7 +102,7 @@ class APP_LIST_EXPORT AppListItemList {
   void FixItemPosition(size_t index);
 
   ScopedVector<AppListItem> app_list_items_;
-  ObserverList<AppListItemListObserver> observers_;
+  ObserverList<AppListItemListObserver, true> observers_;
 
   DISALLOW_COPY_AND_ASSIGN(AppListItemList);
 };
diff --git a/ui/app_list/app_list_model.h b/ui/app_list/app_list_model.h
index 20e6728..07ab33c 100644
--- a/ui/app_list/app_list_model.h
+++ b/ui/app_list/app_list_model.h
@@ -169,7 +169,7 @@ class APP_LIST_EXPORT AppListModel : public AppListItemListObserver {
   scoped_ptr<SearchResults> results_;
 
   Status status_;
-  ObserverList<AppListModelObserver> observers_;
+  ObserverList<AppListModelObserver, true> observers_;
   bool folders_enabled_;
 
   DISALLOW_COPY_AND_ASSIGN(AppListModel);
diff --git a/ui/app_list/app_list_view_delegate_observer.h b/ui/app_list/app_list_view_delegate_observer.h
index a612334..abc4fe4 100644
--- a/ui/app_list/app_list_view_delegate_observer.h
+++ b/ui/app_list/app_list_view_delegate_observer.h
@@ -15,6 +15,11 @@ class APP_LIST_EXPORT AppListViewDelegateObserver {
   // profile changes its signin status.
   virtual void OnProfilesChanged() = 0;
 
+  // Invoked on Chrome shutdown. This is only needed on Mac, since reference-
+  // counting in Objective-C means that simply closing the window isn't enough
+  // to guarantee references to Chrome objects are gone.
+  virtual void OnShutdown() = 0;
+
  protected:
   virtual ~AppListViewDelegateObserver() {}
 };
diff --git a/ui/app_list/cocoa/app_list_view_controller.mm b/ui/app_list/cocoa/app_list_view_controller.mm
index 0ca22f4..b831f9a 100644
--- a/ui/app_list/cocoa/app_list_view_controller.mm
+++ b/ui/app_list/cocoa/app_list_view_controller.mm
@@ -88,6 +88,7 @@ class AppListModelObserverBridge : public AppListViewDelegateObserver {
  private:
   // Overridden from app_list::AppListViewDelegateObserver:
   virtual void OnProfilesChanged() OVERRIDE;
+  virtual void OnShutdown() OVERRIDE;
 
   AppListViewController* parent_;  // Weak. Owns us.
 
@@ -108,6 +109,10 @@ void AppListModelObserverBridge::OnProfilesChanged() {
   [parent_ onProfilesChanged];
 }
 
+void AppListModelObserverBridge::OnShutdown() {
+  [parent_ setDelegate:nil];
+}
+
 }  // namespace app_list
 
 @implementation AppListViewController
diff --git a/ui/app_list/views/app_list_view.cc b/ui/app_list/views/app_list_view.cc
index 4d7df42..2a01e82 100644
--- a/ui/app_list/views/app_list_view.cc
+++ b/ui/app_list/views/app_list_view.cc
@@ -282,6 +282,11 @@ void AppListView::OnProfilesChanged() {
   app_list_main_view_->search_box_view()->InvalidateMenu();
 }
 
+void AppListView::OnShutdown() {
+  // Nothing to do on views - the widget will soon be closed, which will tear
+  // everything down.
+}
+
 void AppListView::SetProfileByPath(const base::FilePath& profile_path) {
   delegate_->SetProfileByPath(profile_path);
   app_list_main_view_->ModelChanged();
diff --git a/ui/app_list/views/app_list_view.h b/ui/app_list/views/app_list_view.h
index d1ee338..0ee715a 100644
--- a/ui/app_list/views/app_list_view.h
+++ b/ui/app_list/views/app_list_view.h
@@ -100,6 +100,7 @@ class APP_LIST_EXPORT AppListView : public views::BubbleDelegateView,
 
   // Overridden from AppListViewDelegateObserver:
   virtual void OnProfilesChanged() OVERRIDE;
+  virtual void OnShutdown() OVERRIDE;
 
   void Prerender();