[Extensions] Possibly de-flake BrowserActionApiTest.CloseBackgroundPage

Improve and possibly de-flake the browser test
BrowserActionApiTest, CloseBackgroundPage. This includes:
- Moving away from the content::NotificationService system to an
  ExtensionHostObserver; this ensures we receive events for the right
  ExtensionHost.
- Waiting for the setBadgeText() call to finish before closing the
  background page.

Neither of these is guaranteed to fix the problem (which I was unable
to reproduce locally), but they're still desirable, so may as well give
it a shot.

Bug: 1035075
Change-Id: I6b24d64d7ddd95b46bb26aae411e8a7ce269e3d6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1972292
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#726020}

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

@@ -9,6 +9,7 @@
 #include "base/files/scoped_temp_dir.h"
 #include "base/macros.h"
 #include "base/run_loop.h"
+#include "base/scoped_observer.h"
 #include "base/strings/stringprintf.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/threading/thread_restrictions.h"
@@ -47,6 +48,8 @@
 #include "content/public/test/download_test_observer.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/test_utils.h"
+#include "extensions/browser/extension_host.h"
+#include "extensions/browser/extension_host_observer.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/notification_types.h"
@@ -773,24 +776,55 @@ IN_PROC_BROWSER_TEST_F(BrowserActionApiTest, CloseBackgroundPage) {
   // There is a background page and a browser action with no badge text.
   extensions::ProcessManager* manager =
       extensions::ProcessManager::Get(browser()->profile());
-  ASSERT_TRUE(manager->GetBackgroundHostForExtension(extension->id()));
+
+  ExtensionHost* extension_host =
+      manager->GetBackgroundHostForExtension(extension->id());
+  ASSERT_TRUE(extension_host);
+
   ExtensionAction* action = GetBrowserAction(browser(), *extension);
   ASSERT_EQ("",
             action->GetExplicitlySetBadgeText(ExtensionAction::kDefaultTabId));
 
-  content::WindowedNotificationObserver host_destroyed_observer(
-      extensions::NOTIFICATION_EXTENSION_HOST_DESTROYED,
-      content::NotificationService::AllSources());
+  // A helper class to wait for the ExtensionHost to shut down.
+  // TODO(devlin): Hoist this somewhere more common and track down other similar
+  // usages.
+  class ExtensionHostDestructionObserver : public ExtensionHostObserver {
+   public:
+    explicit ExtensionHostDestructionObserver(ExtensionHost* host) {
+      host_observer_.Add(host);
+    }
+    ExtensionHostDestructionObserver(
+        const ExtensionHostDestructionObserver& other) = delete;
+    ExtensionHostDestructionObserver& operator=(
+        const ExtensionHostDestructionObserver& other) = delete;
+    ~ExtensionHostDestructionObserver() override = default;
+
+    void OnExtensionHostDestroyed(const ExtensionHost* host) override {
+      // TODO(devlin): It would be nice to
+      // ASSERT_TRUE(host_observer_.IsObserving(host));
+      // host_observer_.Remove(host);
+      // But we can't, because |host| is const. Work around it by just
+      // RemoveAll()ing.
+      host_observer_.RemoveAll();
+      run_loop_.QuitWhenIdle();
+    }
+
+    void Wait() { run_loop_.Run(); }
+
+   private:
+    base::RunLoop run_loop_;
+    ScopedObserver<ExtensionHost, ExtensionHostObserver> host_observer_{this};
+  };
+
+  ExtensionHostDestructionObserver host_destroyed_observer(extension_host);
 
   // Click the browser action.
   ExecuteExtensionAction(browser(), extension);
 
-  // It can take a moment for the background page to actually get destroyed
-  // so we wait for the notification before checking that it's really gone
-  // and the badge text has been set.
   host_destroyed_observer.Wait();
-  ASSERT_FALSE(manager->GetBackgroundHostForExtension(extension->id()));
-  ASSERT_EQ("X",
+
+  EXPECT_FALSE(manager->GetBackgroundHostForExtension(extension->id()));
+  EXPECT_EQ("X",
             action->GetExplicitlySetBadgeText(ExtensionAction::kDefaultTabId));
 }
 

chrome/test/data/extensions/api_test/browser_action/close_background/background.js

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 chrome.browserAction.onClicked.addListener(function(tab) {
-    chrome.browserAction.setBadgeText({text:"X"});
+  chrome.browserAction.setBadgeText({text: 'X'}, () => {
     window.close();
+  });
 });