Wait for WebContentsDestroyed before returning on tabs.remove Extensions API

If a page has beforeunload/unload handlers (and soon, pagehide and
visibilitychange handlers too), tabs.remove might not be able to close
a tab immediately. We shouldn't trigger the callback passed to
tabs.remove until the tabs actually closed. This CL waits for
WebContentsDestroyed to be called on all the tabs that we're removing
before responding/triggering callbacks.

Bug: 987409
Change-Id: Ibdf526307c8c2652bc04a7aca4c379aca6d11922
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2319927
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#794852}

chrome/browser/extensions/api/tabs/tabs_api.cc

@@ -1596,6 +1596,9 @@ ExtensionFunction::ResponseAction TabsReloadFunction::Run() {
   return RespondNow(NoArguments());
 }
 
+TabsRemoveFunction::TabsRemoveFunction() = default;
+TabsRemoveFunction::~TabsRemoveFunction() = default;
+
 ExtensionFunction::ResponseAction TabsRemoveFunction::Run() {
   std::unique_ptr<tabs::Remove::Params> params(
       tabs::Remove::Params::Create(*args_));
@@ -1613,7 +1616,16 @@ ExtensionFunction::ResponseAction TabsRemoveFunction::Run() {
     if (!RemoveTab(*params->tab_ids.as_integer, &error))
       return RespondNow(Error(std::move(error)));
   }
-  return RespondNow(NoArguments());
+  triggered_all_tab_removals_ = true;
+  DCHECK(!did_respond());
+  // WebContentsDestroyed will return the response in most cases, except when
+  // the last tab closed immediately (it won't return a response because
+  // |triggered_all_tab_removals_| will still be false). In this case we should
+  // return the response from here.
+  if (remaining_tabs_count_ == 0) {
+    return RespondNow(NoArguments());
+  }
+  return RespondLater();
 }
 
 bool TabsRemoveFunction::RemoveTab(int tab_id, std::string* error) {
@@ -1629,6 +1641,17 @@ bool TabsRemoveFunction::RemoveTab(int tab_id, std::string* error) {
     *error = tabs_constants::kTabStripNotEditableError;
     return false;
   }
+  // The tab might not immediately close after calling Close() below, so we
+  // should wait until WebContentsDestroyed is called before responding.
+  web_contents_destroyed_observers_.push_back(
+      std::make_unique<WebContentsDestroyedObserver>(this, contents));
+  // Ensure that we're going to keep this class alive until
+  // |remaining_tabs_count| reaches zero. This relies on WebContents::Close()
+  // always (eventually) resulting in a WebContentsDestroyed() call; otherwise,
+  // this function will never respond and may leak.
+  AddRef();
+  remaining_tabs_count_++;
+
   // There's a chance that the tab is being dragged, or we're in some other
   // nested event loop. This code path ensures that the tab is safely closed
   // under such circumstances, whereas |TabStripModel::CloseWebContentsAt()|
@@ -1637,6 +1660,40 @@ bool TabsRemoveFunction::RemoveTab(int tab_id, std::string* error) {
   return true;
 }
 
+void TabsRemoveFunction::TabDestroyed() {
+  DCHECK_GT(remaining_tabs_count_, 0);
+  // One of the tabs we wanted to remove had been destroyed.
+  remaining_tabs_count_--;
+  // If we've triggered all the tab removals we need, and this is the last tab
+  // we're waiting for and we haven't sent a response (it's possible that we've
+  // responded earlier in case of errors, etc.), send a response.
+  if (triggered_all_tab_removals_ && remaining_tabs_count_ == 0 &&
+      !did_respond()) {
+    Respond(NoArguments());
+  }
+  Release();
+}
+
+class TabsRemoveFunction::WebContentsDestroyedObserver
+    : public content::WebContentsObserver {
+ public:
+  WebContentsDestroyedObserver(extensions::TabsRemoveFunction* owner,
+                               content::WebContents* watched_contents)
+      : content::WebContentsObserver(watched_contents), owner_(owner) {}
+
+  ~WebContentsDestroyedObserver() override = default;
+  WebContentsDestroyedObserver(const WebContentsDestroyedObserver&) = delete;
+  WebContentsDestroyedObserver& operator=(const WebContentsDestroyedObserver&) =
+      delete;
+
+  // WebContentsObserver
+  void WebContentsDestroyed() override { owner_->TabDestroyed(); }
+
+ private:
+  // Guaranteed to outlive this object.
+  extensions::TabsRemoveFunction* owner_;
+};
+
 TabsCaptureVisibleTabFunction::TabsCaptureVisibleTabFunction()
     : chrome_details_(this) {
 }

chrome/browser/extensions/api/tabs/tabs_api.h

@@ -24,7 +24,6 @@
 class GURL;
 class SkBitmap;
 class TabStripModel;
-
 namespace content {
 class WebContents;
 }
@@ -175,9 +174,20 @@ class TabsReloadFunction : public ExtensionFunction {
   DECLARE_EXTENSION_FUNCTION("tabs.reload", TABS_RELOAD)
 };
 class TabsRemoveFunction : public ExtensionFunction {
-  ~TabsRemoveFunction() override {}
+ public:
+  TabsRemoveFunction();
+  void TabDestroyed();
+
+ private:
+  class WebContentsDestroyedObserver;
+  ~TabsRemoveFunction() override;
   ResponseAction Run() override;
   bool RemoveTab(int tab_id, std::string* error);
+
+  int remaining_tabs_count_ = 0;
+  bool triggered_all_tab_removals_ = false;
+  std::vector<std::unique_ptr<WebContentsDestroyedObserver>>
+      web_contents_destroyed_observers_;
   DECLARE_EXTENSION_FUNCTION("tabs.remove", TABS_REMOVE)
 };
 class TabsDetectLanguageFunction

chrome/browser/extensions/extension_tabs_apitest.cc

@@ -119,6 +119,15 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTabTest, TabOpener) {
   ASSERT_TRUE(RunExtensionSubtest("tabs/basics", "opener.html")) << message_;
 }
 
+IN_PROC_BROWSER_TEST_F(ExtensionApiTabTest, TabRemove) {
+  ASSERT_TRUE(RunExtensionSubtest("tabs/basics", "remove.html")) << message_;
+}
+
+IN_PROC_BROWSER_TEST_F(ExtensionApiTabTest, TabRemoveMultiple) {
+  ASSERT_TRUE(RunExtensionSubtest("tabs/basics", "remove-multiple.html"))
+      << message_;
+}
+
 IN_PROC_BROWSER_TEST_F(ExtensionApiTabTest, TabGetCurrent) {
   ASSERT_TRUE(RunExtensionTest("tabs/get_current")) << message_;
 }

chrome/test/data/extensions/api_test/tabs/basics/remove-multiple.html

+<!--
+ * Copyright 2020 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.
+-->
+
+<script src="tabs_util.js"></script>
+<script src="remove-multiple.js"></script>

chrome/test/data/extensions/api_test/tabs/basics/remove-multiple.js

+// Copyright 2020 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.
+
+var firstTabId;
+var secondTabId;
+var thirdTabId;
+var fourthTabId;
+
+function createTab(createParams) {
+  return new Promise((resolve) => {
+    chrome.tabs.create(createParams, (tab) => {
+      var createdTabId = tab.id;
+      chrome.tabs.onUpdated.addListener((tabId, changeInfo, tab) => {
+        // Wait for the tab to finish loading.
+        if (tabId == createdTabId && changeInfo.status == 'complete') {
+          resolve(tab);
+        }
+      });
+    });
+  });
+}
+
+chrome.test.runTests([
+  function getFirstTabId() {
+    chrome.tabs.query({ windowId: chrome.windows.WINDOW_ID_CURRENT },
+      (tabs) => {
+        // Make sure we start the test with one tab.
+        assertEq(1, tabs.length);
+        firstTabId = tabs[0].id;
+        chrome.test.succeed();
+    })
+  },
+  function createTabs() {
+    // Create a second tab that has an unload handler.
+    createTab({index: 1, active: false, url: 'unload-storage-1.html'})
+      .then((tab) => {
+        secondTabId = tab.id;
+        assertFalse(tab.active);
+        assertEq(1, tab.index);
+        // Create and switch to a third tab that has an unload handler.
+        return createTab(
+          {index: 2, active: true, url: 'unload-storage-2.html'});
+      }).then((tab) => {
+        thirdTabId = tab.id;
+        assertTrue(tab.active);
+        assertEq(2, tab.index);
+        // Create a fourth tab that does not have an unload handler (it will
+        // open the default New Tab Page).
+        return createTab({index: 3, active: false });
+      }).then((tab) => {
+        fourthTabId = tab.id;
+        assertFalse(tab.active);
+        assertEq(3, tab.index);
+        chrome.test.succeed();
+      });
+  },
+  function removeCreatedTabs() {
+    chrome.tabs.remove([secondTabId, thirdTabId, fourthTabId], () => {
+      // The tabs should've set the 'did_run_unload_1' and
+      // 'did_run_unload_2' values to 'yes' from their unload handler,
+      //  which are accessible from the first tab.
+      assertEq('yes', localStorage.getItem('did_run_unload_1'));
+      assertEq('yes', localStorage.getItem('did_run_unload_2'));
+      chrome.tabs.query({ windowId: chrome.windows.WINDOW_ID_CURRENT },
+        (tabs) => {
+          // Make sure we only have one tab left (the first tab) in the window.
+          assertEq(1, tabs.length);
+          assertEq(firstTabId, tabs[0].id);
+          chrome.test.succeed();
+      });
+    });
+  }
+]);

chrome/test/data/extensions/api_test/tabs/basics/remove.html

+<!--
+ * Copyright 2020 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.
+-->
+
+<script src="tabs_util.js"></script>
+<script src="remove.js"></script>

chrome/test/data/extensions/api_test/tabs/basics/remove.js

+// Copyright 2020 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.
+
+var secondTabId;
+chrome.test.runTests([
+  function createSecondTab() {
+    // Create and switch to a second tab that has an unload handler.
+    chrome.tabs.create({index: 1, active: true, url: 'unload-storage-1.html'},
+      (tab) => {
+        secondTabId = tab.id;
+        assertTrue(tab.active);
+        assertEq(1, tab.index);
+        chrome.tabs.onUpdated.addListener((tabId, changeInfo, tab) => {
+          // Wait for the second tab to finish loading before moving on.
+          if (tabId == secondTabId && changeInfo.status == 'complete') {
+            chrome.test.succeed();
+          }
+        });
+      });
+  },
+  function removeSecondTab() {
+    chrome.tabs.remove(secondTabId, () => {
+      // The second tab should've set the 'did_run_unload_1' value from
+      // its unload handler, which is accessible from the first tab too.
+      assertEq('yes', localStorage.getItem('did_run_unload_1'));
+      chrome.test.succeed();
+    });
+  }
+]);

chrome/test/data/extensions/api_test/tabs/basics/unload-storage-1.html

+<!--
+ * Copyright 2020 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.
+-->
+
+<script src="unload-storage-1.js"></script>
+<body>
+  Page with unload handler that stores {"did_run_unload_1": "yes"}.
+</body>

chrome/test/data/extensions/api_test/tabs/basics/unload-storage-1.js

+// Copyright 2020 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.
+
+window.addEventListener('unload', (e) => {
+  localStorage.setItem('did_run_unload_1', 'yes');
+});

chrome/test/data/extensions/api_test/tabs/basics/unload-storage-2.html

+<!--
+ * Copyright 2020 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.
+-->
+
+<script src="unload-storage-2.js"></script>
+<body>
+  Page with unload handler that stores {"did_run_unload_2": "yes"}.
+</body>

chrome/test/data/extensions/api_test/tabs/basics/unload-storage-2.js

+// Copyright 2020 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.
+
+window.addEventListener('unload', (e) => {
+  localStorage.setItem('did_run_unload_2', 'yes');
+});