Add test for uninstalling an extension while showing its popup.

This patch also fixes a race revealed by the new test. When the
extension was uninstalled, ExtensionHostQueue::ProcessOneHost
would race with ExtensionHostQueue::Remove (called from
~ExtensionHost), and if the former won it would crash trying to get the
missing extension's ID. This patch ensures that ExtensionPopup
immediately destroys its owned ExtensionHost when the extension is
uninstalled, avoiding the race.

Bug: 984654
Change-Id: Ib811a6fca559129f235e00f6091086cf7f2f968f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1980613Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#736818}

chrome/browser/ui/views/extensions/extension_popup.cc

@@ -8,6 +8,7 @@
 #include "chrome/browser/devtools/devtools_window.h"
 #include "chrome/browser/extensions/extension_view_host.h"
 #include "chrome/browser/ui/browser.h"
+#include "content/public/browser/browser_context.h"
 #include "content/public/browser/devtools_agent_host.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_source.h"
@@ -123,6 +124,16 @@ void ExtensionPopup::OnExtensionSizeChanged(ExtensionViewViews* view) {
   SizeToContents();
 }
 
+void ExtensionPopup::OnExtensionUnloaded(
+    content::BrowserContext* browser_context,
+    const extensions::Extension* extension,
+    extensions::UnloadedExtensionReason reason) {
+  if (extension->id() == host()->extension_id()) {
+    host_.reset();
+    GetWidget()->Close();
+  }
+}
+
 void ExtensionPopup::Observe(int type,
                              const content::NotificationSource& source,
                              const content::NotificationDetails& details) {
@@ -169,6 +180,7 @@ ExtensionPopup::ExtensionPopup(
                                arrow,
                                views::BubbleBorder::SMALL_SHADOW),
       host_(std::move(host)),
+      extension_registry_observer_(this),
       show_action_(show_action) {
   DialogDelegate::set_buttons(ui::DIALOG_BUTTON_NONE);
   DialogDelegate::set_use_round_corners(false);
@@ -188,6 +200,9 @@ ExtensionPopup::ExtensionPopup(
   content::DevToolsAgentHost::AddObserver(this);
   host_->browser()->tab_strip_model()->AddObserver(this);
 
+  extension_registry_observer_.Add(
+      extensions::ExtensionRegistry::Get(host_->browser_context()));
+
   // If the host had somehow finished loading, then we'd miss the notification
   // and not show.  This seems to happen in single-process mode.
   if (host_->has_loaded_once()) {

chrome/browser/ui/views/extensions/extension_popup.h

@@ -8,12 +8,15 @@
 #include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/macros.h"
+#include "base/scoped_observer.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
 #include "chrome/browser/ui/views/extensions/extension_view_views.h"
 #include "content/public/browser/devtools_agent_host_observer.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/browser/extension_registry_observer.h"
 #include "ui/views/bubble/bubble_dialog_delegate_view.h"
 #include "url/gurl.h"
 
@@ -24,11 +27,14 @@
 class ExtensionViewViews;
 
 namespace content {
+class BrowserContext;
 class DevToolsAgentHost;
 }
 
 namespace extensions {
+class Extension;
 class ExtensionViewHost;
+enum class UnloadedExtensionReason;
 }
 
 // The bubble used for hosting a browser-action popup provided by an extension.
@@ -37,6 +43,7 @@ class ExtensionPopup : public views::BubbleDialogDelegateView,
                        public wm::ActivationChangeObserver,
 #endif
                        public ExtensionViewViews::Container,
+                       public extensions::ExtensionRegistryObserver,
                        public content::NotificationObserver,
                        public TabStripModelObserver,
                        public content::DevToolsAgentHostObserver {
@@ -87,6 +94,11 @@ class ExtensionPopup : public views::BubbleDialogDelegateView,
   // ExtensionViewViews::Container:
   void OnExtensionSizeChanged(ExtensionViewViews* view) override;
 
+  // extensions::ExtensionRegistryObserver:
+  void OnExtensionUnloaded(content::BrowserContext* browser_context,
+                           const extensions::Extension* extension,
+                           extensions::UnloadedExtensionReason reason) override;
+
   // content::NotificationObserver:
   void Observe(int type,
                const content::NotificationSource& source,
@@ -121,6 +133,10 @@ class ExtensionPopup : public views::BubbleDialogDelegateView,
   // The contained host for the view.
   std::unique_ptr<extensions::ExtensionViewHost> host_;
 
+  ScopedObserver<extensions::ExtensionRegistry,
+                 extensions::ExtensionRegistryObserver>
+      extension_registry_observer_;
+
   ShowAction show_action_;
 
   content::NotificationRegistrar registrar_;

chrome/browser/ui/views/extensions/extensions_menu_view_browsertest.cc

@@ -12,9 +12,11 @@
 #include "build/build_config.h"
 #include "chrome/browser/extensions/chrome_test_extension_loader.h"
 #include "chrome/browser/extensions/extension_action_runner.h"
+#include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/scripting_permissions_modifier.h"
 #include "chrome/browser/ui/extensions/extension_installed_bubble.h"
 #include "chrome/browser/ui/test/test_browser_dialog.h"
+#include "chrome/browser/ui/toolbar/toolbar_action_view_controller.h"
 #include "chrome/browser/ui/ui_features.h"
 #include "chrome/browser/ui/views/extensions/extensions_menu_button.h"
 #include "chrome/browser/ui/views/extensions/extensions_menu_item_view.h"
@@ -28,6 +30,8 @@
 #include "chrome/common/webui_url_constants.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/test/test_navigation_observer.h"
+#include "extensions/browser/disable_reason.h"
+#include "extensions/browser/extension_system.h"
 #include "extensions/common/extension.h"
 #include "extensions/test/test_extension_dir.h"
 #include "net/dns/mock_host_resolver.h"
@@ -353,6 +357,31 @@ IN_PROC_BROWSER_TEST_F(ExtensionsMenuViewBrowserTest, TriggerPopup) {
   EXPECT_TRUE(GetVisibleToolbarActionViews().empty());
 }
 
+IN_PROC_BROWSER_TEST_F(ExtensionsMenuViewBrowserTest,
+                       RemoveExtensionShowingPopup) {
+  LoadTestExtension("extensions/simple_with_popup");
+  ShowUi("");
+  VerifyUi();
+  TriggerSingleExtensionButton();
+
+  ExtensionsContainer* const extensions_container =
+      BrowserView::GetBrowserViewForBrowser(browser())
+          ->toolbar()
+          ->extensions_container();
+  ToolbarActionViewController* action =
+      extensions_container->GetPoppedOutAction();
+  ASSERT_NE(nullptr, action);
+  ASSERT_EQ(1u, GetVisibleToolbarActionViews().size());
+
+  extensions::ExtensionSystem::Get(browser()->profile())
+      ->extension_service()
+      ->DisableExtension(action->GetId(),
+                         extensions::disable_reason::DISABLE_USER_ACTION);
+
+  EXPECT_EQ(nullptr, extensions_container->GetPoppedOutAction());
+  EXPECT_TRUE(GetVisibleToolbarActionViews().empty());
+}
+
 IN_PROC_BROWSER_TEST_F(ExtensionsMenuViewBrowserTest,
                        TriggeringExtensionClosesMenu) {
   LoadTestExtension("extensions/trigger_actions/browser_action");
@@ -542,6 +571,3 @@ INSTANTIATE_TEST_SUITE_P(AcceptDialog,
 INSTANTIATE_TEST_SUITE_P(CancelDialog,
                          ActivateWithReloadExtensionsMenuBrowserTest,
                          testing::Values(false));
-
-// TODO(pbos): Add test coverage that makes sure removing popped-out extensions
-// properly disposes of the popup.