Bug 1379174 - when closing multiple tabs, handle PermitUnload in parallel, r=Gijs.
authorFlorian Quèze <florian@queze.net>
Thu, 04 Mar 2021 14:04:57 +0000
changeset 569650 c7478f194bc603ec273c94c2bd7cca0c4922d3a6
parent 569649 d9e931d472bcd20cab54ed422ecfa509f0a66d2d
child 569651 58c7ccc6a701eadd62a99269ad9c6bd3403b11bb
push id38264
push usermalexandru@mozilla.com
push dateThu, 04 Mar 2021 21:55:42 +0000
treeherdermozilla-central@5199ec2d73fa [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs
bugs1379174
milestone88.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1379174 - when closing multiple tabs, handle PermitUnload in parallel, r=Gijs. Differential Revision: https://phabricator.services.mozilla.com/D106826
browser/base/content/tabbrowser.js
browser/base/content/test/tabs/browser.ini
browser/base/content/test/tabs/browser_removeTabs_order.js
--- a/browser/base/content/tabbrowser.js
+++ b/browser/base/content/tabbrowser.js
@@ -3346,34 +3346,101 @@
         return;
       }
 
       let initialTabCount = tabs.length;
       this._clearMultiSelectionLocked = true;
 
       // Guarantee that _clearMultiSelectionLocked lock gets released.
       try {
-        let tabsWithBeforeUnload = [];
+        let tabsWithBeforeUnloadPrompt = [];
+        let tabsWithoutBeforeUnload = [];
+        let beforeUnloadPromises = [];
         let lastToClose;
         let aParams = { animate, prewarmed: true };
 
         for (let tab of tabs) {
           if (tab.selected) {
             lastToClose = tab;
             let toBlurTo = this._findTabToBlurTo(lastToClose, tabs);
             if (toBlurTo) {
               this._getSwitcher().warmupTab(toBlurTo);
             }
           } else if (this._hasBeforeUnload(tab)) {
-            tabsWithBeforeUnload.push(tab);
+            TelemetryStopwatch.start("FX_TAB_CLOSE_PERMIT_UNLOAD_TIME_MS", tab);
+            // We need to block while calling permitUnload() because it
+            // processes the event queue and may lead to another removeTab()
+            // call before permitUnload() returns.
+            tab._pendingPermitUnload = true;
+            beforeUnloadPromises.push(
+              // To save time, we first run the beforeunload event listeners in all
+              // content processes in parallel. Tabs that would have shown a prompt
+              // will be handled again later.
+              tab.linkedBrowser.asyncPermitUnload("dontUnload").then(
+                ({ permitUnload }) => {
+                  tab._pendingPermitUnload = false;
+                  TelemetryStopwatch.finish(
+                    "FX_TAB_CLOSE_PERMIT_UNLOAD_TIME_MS",
+                    tab
+                  );
+                  if (tab.closing) {
+                    // The tab was closed by the user while we were in permitUnload, don't
+                    // attempt to close it a second time.
+                  } else if (permitUnload) {
+                    // OK to close without prompting, do it immediately.
+                    this.removeTab(tab, {
+                      animate,
+                      prewarmed: true,
+                      skipPermitUnload: true,
+                    });
+                  } else {
+                    // We will need to prompt, queue it so it happens sequentially.
+                    tabsWithBeforeUnloadPrompt.push(tab);
+                  }
+                },
+                err => {
+                  console.log("error while calling asyncPermitUnload", err);
+                  tab._pendingPermitUnload = false;
+                  TelemetryStopwatch.finish(
+                    "FX_TAB_CLOSE_PERMIT_UNLOAD_TIME_MS",
+                    tab
+                  );
+                }
+              )
+            );
           } else {
-            this.removeTab(tab, aParams);
+            tabsWithoutBeforeUnload.push(tab);
           }
         }
-        for (let tab of tabsWithBeforeUnload) {
+        // Now that all the beforeunload IPCs have been sent to content processes,
+        // we can queue unload messages for all the tabs without beforeunload listeners.
+        // Doing this first would cause content process main threads to be busy and delay
+        // beforeunload responses, which would be user-visible.
+        for (let tab of tabsWithoutBeforeUnload) {
+          this.removeTab(tab, aParams);
+        }
+
+        // Wait for all the beforeunload events to have been processed by content processes.
+        // The permitUnload() promise will, alas, not call its resolution
+        // callbacks after the browser window the promise lives in has closed,
+        // so we have to check for that case explicitly.
+        let done = false;
+        Promise.all(beforeUnloadPromises).then(() => {
+          done = true;
+        });
+        Services.tm.spinEventLoopUntilOrShutdown(
+          "tabbrowser.js:removeTabs",
+          () => done || window.closed
+        );
+        if (!done) {
+          return;
+        }
+
+        // Now run again sequentially the beforeunload listeners that will result in a prompt.
+        for (let tab of tabsWithBeforeUnloadPrompt) {
           this.removeTab(tab, aParams);
         }
 
         // Avoid changing the selected browser several times by removing it,
         // if appropriate, lastly.
         if (lastToClose) {
           this.removeTab(lastToClose, aParams);
         }
--- a/browser/base/content/test/tabs/browser.ini
+++ b/browser/base/content/test/tabs/browser.ini
@@ -88,16 +88,17 @@ support-files = file_anchor_elements.htm
 [browser_pinnedTabs_closeByKeyboard.js]
 [browser_pinnedTabs.js]
 [browser_positional_attributes.js]
 skip-if = (verify && (os == 'win' || os == 'mac'))
 [browser_preloadedBrowser_zoom.js]
 [browser_progress_keyword_search_handling.js]
 [browser_reload_deleted_file.js]
 skip-if = (debug && os == 'mac') || (debug && os == 'linux' && bits == 64) #Bug 1421183, disabled on Linux/OSX for leaked windows
+[browser_removeTabs_order.js]
 [browser_tabCloseSpacer.js]
 skip-if = (os == 'linux' && bits == 64) || (os == 'win' && debug) || (os == "mac") #Bug 1549985
 [browser_tab_a11y_description.js]
 [browser_tab_label_during_reload.js]
 [browser_tab_label_picture_in_picture.js]
 [browser_tab_manager_visibility.js]
 [browser_tabCloseProbes.js]
 [browser_tabContextMenu_keyboard.js]
new file mode 100644
--- /dev/null
+++ b/browser/base/content/test/tabs/browser_removeTabs_order.js
@@ -0,0 +1,38 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+const { sinon } = ChromeUtils.import("resource://testing-common/Sinon.jsm");
+
+add_task(async function setup() {
+  let tab1 = await addTab();
+  let tab2 = await addTab();
+  let tab3 = await addTab();
+  let tabs = [tab1, tab2, tab3];
+
+  // Add a beforeunload event listener in one of the tabs; it should be called
+  // before closing any of the tabs.
+  await ContentTask.spawn(tab2.linkedBrowser, null, async function() {
+    content.window.addEventListener("beforeunload", function(event) {}, true);
+  });
+
+  let permitUnloadSpy = sinon.spy(tab2.linkedBrowser, "asyncPermitUnload");
+  let removeTabSpy = sinon.spy(gBrowser, "removeTab");
+
+  gBrowser.removeTabs(tabs);
+
+  Assert.ok(permitUnloadSpy.calledOnce, "permitUnload was called only once");
+  Assert.equal(
+    removeTabSpy.callCount,
+    tabs.length,
+    "removeTab was called for every tab"
+  );
+  Assert.ok(
+    permitUnloadSpy.lastCall.calledBefore(removeTabSpy.firstCall),
+    "permitUnload was called before for first removeTab call"
+  );
+
+  removeTabSpy.restore();
+  permitUnloadSpy.restore();
+});