Ensure all BrowserAction ExtensionHosts are destroyed in BrowserActionInteractiveTest

chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc

 // Copyright 2013 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 "base/test/test_timeouts.h"
 #include "build/build_config.h"
 #include "chrome/browser/extensions/browser_action_test_util.h"
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/extensions/extension_action_manager.h"
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/sessions/session_tab_helper.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_finder.h"
@@ skipping 15 matching lines @@
 #include "extensions/test/result_catcher.h"
 #include "ui/base/ui_features.h"
 
 #if defined(OS_WIN)
 #include "ui/views/win/hwnd_util.h"
 #endif
 
 namespace extensions {
 namespace {
 
+// Helper to ensure all extension hosts are destroyed during the test. If a host
+// is still alive, the Profile can not be destroyed in
+// BrowserProcessImpl::StartTearDown(). TODO(tapted): The existence of this

[Devlin, 2017/06/05 14:22:11]

I think I'd agree with this.  What happens if a user closes a browser with an
open popup?  Does it trigger the asserts?

[tapted, 2017/06/05 20:59:52]

The code in ProfileDestroyer::DestroyProfileWhenAppropriate(..) has a supported
path for destroying incognito renderer hosts before the Profile is destroyed. So
for users everything should work fine - extension hosts just fall into that
path. There's a comment around the DCHECK saying ~"this is bad maybe you're
leaking hosts" but with this code added we are checking that there is no leak.

+// helper is probably a bug. Extension hosts do not currently block shutdown the
+// way a browser tab does. Maybe they should. See http://crbug.com/729476.
+class ExtensionHostWatcher : public content::NotificationObserver {
+ public:
+  ExtensionHostWatcher() {
+    registrar_.Add(this, NOTIFICATION_EXTENSION_HOST_CREATED,
+                   content::NotificationService::AllSources());
+    registrar_.Add(this, NOTIFICATION_EXTENSION_HOST_DESTROYED,
+                   content::NotificationService::AllSources());
+  }
+
+  void Wait() {
+    if (created_ == destroyed_)
+      return;
+
+    base::RunLoop run_loop;
+    quit_closure_ = run_loop.QuitClosure();
+    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE, quit_closure_, TestTimeouts::action_timeout());

[Devlin, 2017/06/05 14:22:12]

Interesting!  I don't think I've seen this before.

+    run_loop.Run();
+  }
+
+  int created() const { return created_; }
+  int destroyed() const { return destroyed_; }
+
+  // NotificationObserver:
+  void Observe(int type,
+               const content::NotificationSource& source,
+               const content::NotificationDetails& details) override {
+    ++(type == NOTIFICATION_EXTENSION_HOST_CREATED ? created_ : destroyed_);
+    if (!quit_closure_.is_null() && created_ == destroyed_)
+      quit_closure_.Run();
+  }
+
+ private:
+  content::NotificationRegistrar registrar_;
+  base::Closure quit_closure_;
+  int created_ = 0;
+  int destroyed_ = 0;
+
+  DISALLOW_COPY_AND_ASSIGN(ExtensionHostWatcher);
+};
+
 // chrome.browserAction API tests that interact with the UI in such a way that
 // they cannot be run concurrently (i.e. openPopup API tests that require the
 // window be focused/active).
 class BrowserActionInteractiveTest : public ExtensionApiTest {
  public:
   BrowserActionInteractiveTest() {}
   ~BrowserActionInteractiveTest() override {}
 
   // BrowserTestBase:
   void SetUpOnMainThread() override {
+    host_watcher_ = base::MakeUnique<ExtensionHostWatcher>();
     ExtensionApiTest::SetUpOnMainThread();
     EXPECT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
   }
 
+  void TearDownOnMainThread() override {
+    // Note browser windows are closed in PostRunTestOnMainThread(), which is
+    // called after this. But relying on the window close to close the
+    // extension host can cause flakes. See http://crbug.com/729476.
+    // Waiting here requires individual tests to ensure their popup has closed.
+    ExtensionApiTest::TearDownOnMainThread();
+    host_watcher_->Wait();
+    EXPECT_EQ(host_watcher_->created(), host_watcher_->destroyed());
+  }
+
  protected:
   // Function to control whether to run popup tests for the current platform.
   // These tests require RunExtensionSubtest to work as expected and the browser
   // window to able to be made active automatically. Returns false for platforms
   // where these conditions are not met.
   bool ShouldRunPopupTest() {
     // TODO(justinlin): http://crbug.com/177163
 #if defined(OS_WIN) && !defined(NDEBUG)
     return false;
 #else
@@ skipping 24 matching lines @@
   // Open an extension popup by clicking the browser action button.
   void OpenPopupViaToolbar() {
     content::WindowedNotificationObserver frame_observer(
         content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME,
         content::NotificationService::AllSources());
     BrowserActionTestUtil(browser()).Press(0);
     frame_observer.Wait();
     EnsurePopupActive();
   }
 
+  // Close the popup window directly.
+  void ClosePopup() { BrowserActionTestUtil(browser()).HidePopup(); }
+
   // Trigger a focus loss to close the popup.
   void ClosePopupViaFocusLoss() {
     EXPECT_TRUE(BrowserActionTestUtil(browser()).HasPopup());
     content::WindowedNotificationObserver observer(
         extensions::NOTIFICATION_EXTENSION_HOST_DESTROYED,
         content::NotificationService::AllSources());
 
 #if defined(OS_MACOSX)
     // ClickOnView() in an inactive window is not robust on Mac. The click does
     // not guarantee window activation on trybots. So activate the browser
     // explicitly, thus causing the bubble to lose focus and dismiss itself.
     // This works because bubbles on Mac are always toplevel.
     EXPECT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
 #else
     // Elsewhere, click on the omnibox. Note that with aura, the browser may be
     // "active" the entire time when the popup is not a toplevel window. It's
     // aura::Window::Focus() that determines where key events go in this case.
     ui_test_utils::ClickOnView(browser(), VIEW_ID_OMNIBOX);
 #endif
 
     // The window disappears immediately.
     EXPECT_FALSE(BrowserActionTestUtil(browser()).HasPopup());
 
     // Wait for the notification to achieve a consistent state and verify that
     // the popup was properly torn down.
     observer.Wait();
     base::RunLoop().RunUntilIdle();
   }
+
+ private:
+  std::unique_ptr<ExtensionHostWatcher> host_watcher_;
+
+  DISALLOW_COPY_AND_ASSIGN(BrowserActionInteractiveTest);
 };
 
 // Tests opening a popup using the chrome.browserAction.openPopup API. This test
 // opens a popup in the starting window, closes the popup, creates a new window
 // and opens a popup in the new window. Both popups should succeed in opening.
 IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest, TestOpenPopup) {
   if (!ShouldRunPopupTest())
     return;
 
   BrowserActionTestUtil browserActionBar(browser());
@@ skipping 17 matching lines @@
                                WindowOpenDisposition::NEW_WINDOW,
                                ui::PAGE_TRANSITION_TYPED, false)));
     // Hide all the buttons to test that it opens even when the browser action
     // is in the overflow bucket.
     ToolbarActionsModel::Get(profile())->SetVisibleIconCount(0);
     frame_observer.Wait();
   }
 
   EXPECT_TRUE(new_browser != NULL);
 
-// Flaky on non-aura linux http://crbug.com/309749
-#if !(defined(OS_LINUX) && !defined(USE_AURA))
   ResultCatcher catcher;
   {
     content::WindowedNotificationObserver frame_observer(
         content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME,
         content::NotificationService::AllSources());
     // Show second popup in new window.
     listener.Reply("show another");
     frame_observer.Wait();
     EXPECT_TRUE(BrowserActionTestUtil(new_browser).HasPopup());
   }
   ASSERT_TRUE(catcher.GetNextResult()) << message_;
-#endif
+  BrowserActionTestUtil(new_browser).HidePopup();
 }
 
 // Tests opening a popup in an incognito window.
 IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest, TestOpenPopupIncognito) {
   if (!ShouldRunPopupTest())
     return;
 
   content::WindowedNotificationObserver frame_observer(
       content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME,
       content::NotificationService::AllSources());
   ASSERT_TRUE(RunExtensionSubtest("browser_action/open_popup",
                                   "open_popup_succeeds.html",
                                   kFlagEnableIncognito | kFlagUseIncognito))
       << message_;
   frame_observer.Wait();
   // Non-Aura Linux uses a singleton for the popup, so it looks like all windows
   // have popups if there is any popup open.
 #if !(defined(OS_LINUX) && !defined(USE_AURA))
   // Starting window does not have a popup.
   EXPECT_FALSE(BrowserActionTestUtil(browser()).HasPopup());
 #endif
   // Incognito window should have a popup.
-  EXPECT_TRUE(BrowserActionTestUtil(BrowserList::GetInstance()->GetLastActive())
-                  .HasPopup());
+  BrowserActionTestUtil test_util(BrowserList::GetInstance()->GetLastActive());
+  EXPECT_TRUE(test_util.HasPopup());
+  test_util.HidePopup();
 }
 
 // Tests that an extension can open a popup in the last active incognito window
 // even from a background page with a non-incognito profile.
 // (crbug.com/448853)
 #if defined(OS_WIN)
 // Fails on XP: http://crbug.com/515717
 #define MAYBE_TestOpenPopupIncognitoFromBackground \
   DISABLED_TestOpenPopupIncognitoFromBackground
 #else
 #define MAYBE_TestOpenPopupIncognitoFromBackground \
   TestOpenPopupIncognitoFromBackground
 #endif
 IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest,
                        MAYBE_TestOpenPopupIncognitoFromBackground) {
   if (!ShouldRunPopupTest())
     return;
 
   const Extension* extension =
       LoadExtensionIncognito(test_data_dir_.AppendASCII("browser_action").
           AppendASCII("open_popup_background"));
   ASSERT_TRUE(extension);
   ExtensionTestMessageListener listener(false);
   listener.set_extension_id(extension->id());
 
   Browser* incognito_browser =
       OpenURLOffTheRecord(profile(), GURL("chrome://newtab/"));
   listener.WaitUntilSatisfied();
   EXPECT_EQ(std::string("opened"), listener.message());
-  EXPECT_TRUE(BrowserActionTestUtil(incognito_browser).HasPopup());
+  BrowserActionTestUtil test_util(incognito_browser);
+  EXPECT_TRUE(test_util.HasPopup());
+  test_util.HidePopup();
 }
 
 #if defined(OS_LINUX)
 #define MAYBE_TestOpenPopupDoesNotCloseOtherPopups DISABLED_TestOpenPopupDoesNotCloseOtherPopups
 #else
 #define MAYBE_TestOpenPopupDoesNotCloseOtherPopups TestOpenPopupDoesNotCloseOtherPopups
 #endif
 // Tests if there is already a popup open (by a user click or otherwise), that
 // the openPopup API does not override it.
 IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest,
@@ skipping 11 matching lines @@
   // Load the test extension which will do nothing except notifyPass() to
   // return control here.
   ASSERT_TRUE(RunExtensionSubtest("browser_action/open_popup",
                                   "open_popup_fails.html")) << message_;
   EXPECT_TRUE(listener.WaitUntilSatisfied());
   OpenPopupViaToolbar();
   ResultCatcher catcher;
   // Return control to javascript to validate that opening a popup fails now.
   listener.Reply("show another");
   ASSERT_TRUE(catcher.GetNextResult()) << message_;
+  ClosePopup();
 }
 
 // Test that openPopup does not grant tab permissions like for browser action
 // clicks if the activeTab permission is set.
 IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest,
                        TestOpenPopupDoesNotGrantTabPermissions) {
   if (!ShouldRunPopupTest())
     return;
 
   OpenPopupViaAPI();
   ExtensionService* service = extensions::ExtensionSystem::Get(
       browser()->profile())->extension_service();
   ASSERT_FALSE(
       service->GetExtensionById(last_loaded_extension_id(), false)
           ->permissions_data()
           ->HasAPIPermissionForTab(
               SessionTabHelper::IdForTab(
                   browser()->tab_strip_model()->GetActiveWebContents()),
               APIPermission::kTab));
+  ClosePopup();
 }
 
 // Test that the extension popup is closed when the browser window is focused.
 IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest, FocusLossClosesPopup1) {
   if (!ShouldRunPopupTest())
     return;
   OpenPopupViaAPI();
   ClosePopupViaFocusLoss();
 }
 
@@ skipping 119 matching lines @@
 
   // Forcibly closing the browser HWND should not cause a crash.
   EXPECT_EQ(TRUE, ::CloseWindow(hwnd));
   EXPECT_EQ(TRUE, ::DestroyWindow(hwnd));
   EXPECT_EQ(FALSE, ::IsWindow(hwnd));
 }
 #endif  // OS_WIN
 
 }  // namespace
 }  // namespace extensions