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}

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();

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();

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]);
 }

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

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);
 };

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);

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() {}
 };

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

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();

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();