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}

4 files changed, 59 insertions, 10 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