Close all panels originated by the extension when extension unloads.

This fixes the bug where you're able to click on the wrench of panels whose originating extension has been unloaded, resulting in a crash.

BUG=101118
TEST=PanelBrowserTest.NonExtensionDomainPanelsCloseOnUninstall


Review URL: http://codereview.chromium.org/8387010

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@107837 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/extensions/window_open_apitest.cc

@@ -3,10 +3,13 @@
 // found in the LICENSE file.
 
 #include "base/command_line.h"
+#include "base/memory/scoped_vector.h"
 #include "chrome/browser/extensions/extension_apitest.h"
+#include "chrome/browser/extensions/extension_test_message_listener.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/common/extensions/extension.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "net/base/mock_host_resolver.h"
 
@@ -21,16 +24,19 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, DISABLED_WindowOpen) {
   EXPECT_TRUE(catcher.GetNextResult()) << catcher.message();
 }
 
-void WaitForTabsAndPopups(Browser* browser, int num_tabs, int num_popups) {
+void WaitForTabsAndPopups(Browser* browser,
+                          int num_tabs,
+                          int num_popups,
+                          int num_panels) {
   // We start with one tab and one browser already open.
   ++num_tabs;
-  size_t num_browsers = static_cast<size_t>(num_popups) + 1;
+  size_t num_browsers = static_cast<size_t>(num_popups + num_panels) + 1;
 
   const base::TimeDelta kWaitTime = base::TimeDelta::FromSeconds(15);
   base::TimeTicks end_time = base::TimeTicks::Now() + kWaitTime;
   while (base::TimeTicks::Now() < end_time) {
-    if (BrowserList::GetBrowserCount(browser->profile()) >= num_browsers &&
-        browser->tab_count() >= num_tabs)
+    if (BrowserList::GetBrowserCount(browser->profile()) == num_browsers &&
+        browser->tab_count() == num_tabs)
       break;
 
     MessageLoopForUI::current()->RunAllPending();
@@ -39,6 +45,8 @@ void WaitForTabsAndPopups(Browser* browser, int num_tabs, int num_popups) {
   EXPECT_EQ(num_browsers, BrowserList::GetBrowserCount(browser->profile()));
   EXPECT_EQ(num_tabs, browser->tab_count());
 
+  int num_popups_seen = 0;
+  int num_panels_seen = 0;
   for (BrowserList::const_iterator iter = BrowserList::begin();
        iter != BrowserList::end(); ++iter) {
     if (*iter == browser)
@@ -46,7 +54,13 @@ void WaitForTabsAndPopups(Browser* browser, int num_tabs, int num_popups) {
 
     // Check for TYPE_POPUP or TYPE_PANEL.
     EXPECT_TRUE((*iter)->is_type_popup() || (*iter)->is_type_panel());
+    if ((*iter)->is_type_popup())
+      ++num_popups_seen;
+    else
+      ++num_panels_seen;
   }
+  EXPECT_EQ(num_popups, num_popups_seen);
+  EXPECT_EQ(num_panels, num_panels_seen);
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionApiTest, BrowserIsApp) {
@@ -55,7 +69,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, BrowserIsApp) {
   ASSERT_TRUE(LoadExtension(
       test_data_dir_.AppendASCII("window_open").AppendASCII("browser_is_app")));
 
-  WaitForTabsAndPopups(browser(), 0, 2);
+  WaitForTabsAndPopups(browser(), 0, 2, 0);
 
   for (BrowserList::const_iterator iter = BrowserList::begin();
        iter != BrowserList::end(); ++iter) {
@@ -73,7 +87,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowOpenPopupDefault) {
 
   const int num_tabs = 1;
   const int num_popups = 0;
-  WaitForTabsAndPopups(browser(), num_tabs, num_popups);
+  WaitForTabsAndPopups(browser(), num_tabs, num_popups, 0);
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowOpenPopupLarge) {
@@ -90,7 +104,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowOpenPopupLarge) {
   const int num_tabs = 0;
   const int num_popups = 1;
 #endif
-  WaitForTabsAndPopups(browser(), num_tabs, num_popups);
+  WaitForTabsAndPopups(browser(), num_tabs, num_popups, 0);
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowOpenPopupSmall) {
@@ -102,7 +116,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowOpenPopupSmall) {
   // On other systems this should open a new popup window.
   const int num_tabs = 0;
   const int num_popups = 1;
-  WaitForTabsAndPopups(browser(), num_tabs, num_popups);
+  WaitForTabsAndPopups(browser(), num_tabs, num_popups, 0);
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionApiTest, PopupBlockingExtension) {
@@ -113,7 +127,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, PopupBlockingExtension) {
       test_data_dir_.AppendASCII("window_open").AppendASCII("popup_blocking")
       .AppendASCII("extension")));
 
-  WaitForTabsAndPopups(browser(), 5, 3);
+  WaitForTabsAndPopups(browser(), 5, 3, 0);
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionApiTest, PopupBlockingHostedApp) {
@@ -146,7 +160,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, PopupBlockingHostedApp) {
   browser()->OpenURL(open_popup, GURL(), NEW_FOREGROUND_TAB,
                      content::PAGE_TRANSITION_TYPED);
 
-  WaitForTabsAndPopups(browser(), 3, 1);
+  WaitForTabsAndPopups(browser(), 3, 1, 0);
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowArgumentsOverflow) {
@@ -175,6 +189,43 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowOpenFocus) {
 }
 #endif
 
+IN_PROC_BROWSER_TEST_F(ExtensionApiTest,
+                       CloseNonExtensionPanelsOnUninstall) {
+  ASSERT_TRUE(StartTestServer());
+
+  // Setup listeners to wait on strings we expect the extension pages to send.
+  std::vector<std::string> test_strings;
+  test_strings.push_back("content_tab");
+  test_strings.push_back("content_panel");
+  test_strings.push_back("content_popup");
+
+  ScopedVector<ExtensionTestMessageListener> listeners;
+  for (size_t i = 0; i < test_strings.size(); ++i) {
+    listeners.push_back(
+        new ExtensionTestMessageListener(test_strings[i], false));
+  }
+
+  const Extension* extension = LoadExtension(
+      test_data_dir_.AppendASCII("window_open").AppendASCII(
+          "close_panels_on_uninstall"));
+  ASSERT_TRUE(extension);
+
+  // Two tabs. One in extension domain and one in non-extension domain.
+  // Two popups - one in extension domain and one in non-extension domain.
+  // Two panels - one in extension domain and one in non-extension domain.
+  WaitForTabsAndPopups(browser(), 2, 2, 2);
+
+  // Wait on test messages to make sure the pages loaded.
+  for (size_t i = 0; i < listeners.size(); ++i)
+    ASSERT_TRUE(listeners[i]->WaitUntilSatisfied());
+
+  UninstallExtension(extension->id());
+
+  // Wait for one tab and one popup in non-extension domain to stay open.
+  // Expect everything else, including panels, to close.
+  WaitForTabsAndPopups(browser(), 1, 1, 0);
+}
+
 IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowOpener) {
   ASSERT_TRUE(RunExtensionTest("window_open/opener")) << message_;
 }

chrome/browser/ui/browser.cc

@@ -4250,8 +4250,14 @@ void Browser::Observe(int type,
       TabStripModel* model = tab_handler_->GetTabStripModel();
       for (int i = model->count() - 1; i >= 0; --i) {
         TabContents* tc = model->GetTabContentsAt(i)->tab_contents();
-        if (tc->GetURL().SchemeIs(chrome::kExtensionScheme) &&
-            tc->GetURL().host() == extension->id()) {
+        bool close_tab_contents =
+            tc->GetURL().SchemeIs(chrome::kExtensionScheme) &&
+            tc->GetURL().host() == extension->id();
+        // We want to close all panels originated by the unloaded extension.
+        close_tab_contents = close_tab_contents || (type_ == TYPE_PANEL &&
+            (web_app::GetExtensionIdFromApplicationName(app_name_) ==
+                extension->id()));
+        if (close_tab_contents) {
           CloseTabContents(tc);
         }
       }

chrome/browser/ui/panels/panel_browsertest.cc

@@ -21,10 +21,12 @@
 #include "chrome/browser/web_applications/web_app.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/pref_names.h"
+#include "chrome/common/url_constants.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/browser/download/download_manager.h"
 #include "content/browser/net/url_request_mock_http_job.h"
 #include "content/browser/tab_contents/tab_contents.h"
+#include "content/public/browser/notification_service.h"
 #include "content/public/common/url_constants.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -1313,6 +1315,67 @@ IN_PROC_BROWSER_TEST_F(PanelBrowserTest,
   panel2->Close();
 }
 
+IN_PROC_BROWSER_TEST_F(PanelBrowserTest,
+                       NonExtensionDomainPanelsCloseOnUninstall) {
+  // Create a test extension.
+  DictionaryValue empty_value;
+  scoped_refptr<Extension> extension =
+      CreateExtension(FILE_PATH_LITERAL("TestExtension"),
+      Extension::INVALID, empty_value);
+  std::string extension_app_name =
+      web_app::GenerateApplicationNameFromExtensionId(extension->id());
+
+  PanelManager* panel_manager = PanelManager::GetInstance();
+  EXPECT_EQ(0, panel_manager->num_panels());
+
+  // Create a panel with the extension as host.
+  CreatePanelParams params(extension_app_name, gfx::Rect(), SHOW_AS_INACTIVE);
+  std::string extension_domain_url(chrome::kExtensionScheme);
+  extension_domain_url += "://";
+  extension_domain_url += extension->id();
+  extension_domain_url += "/hello.html";
+  params.url = GURL(extension_domain_url);
+  Panel* panel = CreatePanelWithParams(params);
+  EXPECT_EQ(1, panel_manager->num_panels());
+
+  // Create a panel with a non-extension host.
+  CreatePanelParams params1(extension_app_name, gfx::Rect(), SHOW_AS_INACTIVE);
+  params1.url = GURL(chrome::kAboutBlankURL);
+  Panel* panel1 = CreatePanelWithParams(params1);
+  EXPECT_EQ(2, panel_manager->num_panels());
+
+  // Create another extension and a panel from that extension.
+  scoped_refptr<Extension> extension_other =
+      CreateExtension(FILE_PATH_LITERAL("TestExtensionOther"),
+      Extension::INVALID, empty_value);
+  std::string extension_app_name_other =
+      web_app::GenerateApplicationNameFromExtensionId(extension_other->id());
+  Panel* panel_other = CreatePanel(extension_app_name_other);
+
+  ui_test_utils::WindowedNotificationObserver signal(
+      chrome::NOTIFICATION_BROWSER_CLOSED,
+      content::Source<Browser>(panel->browser()));
+  ui_test_utils::WindowedNotificationObserver signal1(
+      chrome::NOTIFICATION_BROWSER_CLOSED,
+      content::Source<Browser>(panel1->browser()));
+
+  // Send unload notification on the first extension.
+  UnloadedExtensionInfo details(extension,
+                                extension_misc::UNLOAD_REASON_UNINSTALL);
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_EXTENSION_UNLOADED,
+      content::Source<Profile>(browser()->profile()),
+      content::Details<UnloadedExtensionInfo>(&details));
+
+  // Wait for the panels opened by the first extension to close.
+  signal.Wait();
+  signal1.Wait();
+
+  // Verify that the panel that's left is the panel from the second extension.
+  EXPECT_EQ(panel_other, panel_manager->panels()[0]);
+  panel_other->Close();
+}
+
 class PanelDownloadTest : public PanelBrowserTest {
  public:
   PanelDownloadTest() : PanelBrowserTest() { }

chrome/test/data/extensions/api_test/window_open/close_panels_on_uninstall/background.html

+<script>
+
+// Open a panel, popup and tab to non-extension content.
+chrome.windows.create({url: "about:blank", type: "panel"});
+window.open("about:blank",
+    "content_popup_non_extension", "height=200,width=200");
+window.open("about:blank", "", "");
+
+// Open a panel, popup and tab to extension content.
+chrome.windows.create({url: "content_panel.html", type: "panel"});
+chrome.windows.create({url: "content_popup.html", type: "popup"});
+chrome.tabs.create({url: "content_tab.html"});
+
+</script>

chrome/test/data/extensions/api_test/window_open/close_panels_on_uninstall/content_panel.html

+<html>
+  <head>
+    <script>
+      chrome.test.sendMessage('content_panel');
+    </script>
+  </head>
+  <body>
+    Non extension panels close on uninstall test.
+  </body>
+</html>

chrome/test/data/extensions/api_test/window_open/close_panels_on_uninstall/content_popup.html

+<html>
+  <head>
+    <script>
+      chrome.test.sendMessage('content_popup');
+    </script>
+  </head>
+  <body>
+    Non extension panels close on uninstall test.
+  </body>
+</html>

chrome/test/data/extensions/api_test/window_open/close_panels_on_uninstall/content_tab.html

+<html>
+  <head>
+    <script>
+      chrome.test.sendMessage('content_tab');
+    </script>
+  </head>
+  <body>
+    Non extension panels close on uninstall test.
+  </body>
+</html>

chrome/test/data/extensions/api_test/window_open/close_panels_on_uninstall/manifest.json

+{
+  "name": "Extension app for test non-extension panels close on Uninstall.",
+  "version": "1",
+  "description": "When an extension is uninstalled all panels opened by the extension should be closed.",
+  "background_page": "background.html",
+  "permissions": ["tabs"]
+}