[Extensions] Don't run unload listeners when an extension is removed

chrome/browser/extensions/extension_unload_browsertest.cc

+// Copyright 2016 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 "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "ui/base/window_open_disposition.h"
+
+namespace extensions {
+
+using ExtensionUnloadBrowserTest = ExtensionBrowserTest;
+
+IN_PROC_BROWSER_TEST_F(ExtensionUnloadBrowserTest, TestUnload) {
+  // Load an extension that installs unload and beforeunload listeners.
+  const Extension* extension =
+      LoadExtension(test_data_dir_.AppendASCII("unload_listener"));
+  ASSERT_TRUE(extension);
+  std::string id = extension->id();
+  ASSERT_EQ(1, browser()->tab_strip_model()->count());
+  GURL initial_tab_url =
+      browser()->tab_strip_model()->GetWebContentsAt(0)->GetLastCommittedURL();
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), extension->GetResourceURL("page.html"),
+      WindowOpenDisposition::NEW_FOREGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+  EXPECT_EQ(2, browser()->tab_strip_model()->count());
+  extension_service()->UnloadExtension(id,
+                                       UnloadedExtensionInfo::REASON_DISABLE);
+  // There should only be one remaining web contents - the initial one.
+  ASSERT_EQ(1, browser()->tab_strip_model()->count());

[Charlie Reis, 2016/09/21 18:29:33]

Sanity check: This failed before because the removal was async, right?

[Devlin, 2016/09/22 00:35:21]

Correct.

+  EXPECT_EQ(
+      initial_tab_url,
+      browser()->tab_strip_model()->GetWebContentsAt(0)->GetLastCommittedURL());
+}
+
+// TODO(devlin): We should replicate this behavior for terminated extensions.

[Charlie Reis, 2016/09/21 18:29:33]

I'm less concerned about terminated extensions (which can't make network
requests after the uninstall) than embedded iframes (which can).

[Devlin, 2016/09/22 00:35:21]

Don't quite follow.  It wouldn't be the extension making the request here, it
would be the tab.  So for instance:
Tab open to Extension A's options page
Extension A crashes (is terminated)
Tab remains open
Tab does some network request
Illegal origin (Extension A isn't enabled)

It's not immediately clear what the right course is - we deliberately don't
close all tabs when an extension crashes.  Maybe we could relax the origin check
for terminated extensions?  In either case, a discussion for another CL. :)

(Also, added iframes to the TODO)

[Charlie Reis, 2016/09/22 20:37:47]

I'm confused.  In your example, the tab is showing extension A's options page,
which means that it becomes a sad tab when extension A crashes.  (There isn't a
separate renderer process for the tab.)  The tab remains open, but it can't make
network requests.  Am I missing something?

[Devlin, 2016/09/26 19:44:46]

Ah, right.  I was distracted by the browser code to deliberately *not* close
tabs for crashed extensions, and assumed it was so that reloading the extension
was a reasonable option.  But you're right that these would all be sad tabs at
this point.  Updated to not mention terminated extensions.

+
+}  // namespace extensions