Bug 1666670: Fix beforeunload timeout handling to ignore prompt. r=mconley

Differential Revision: https://phabricator.services.mozilla.com/D91351

browser/base/content/browser.js

@@ -1563,8 +1563,8 @@ function _loadURI(browser, uri, params = {}) {
       browser.webNavigation.loadURI(uri, loadURIOptions);
     } else {
       // Check if the current browser is allowed to unload.
-      let { permitUnload, timedOut } = browser.permitUnload();
-      if (!timedOut && !permitUnload) {
+      let { permitUnload } = browser.permitUnload();
+      if (!permitUnload) {
         return;
       }
 
@@ -7878,27 +7878,13 @@ function CanCloseWindow() {
     return true;
   }
 
-  let timedOutProcesses = new WeakSet();
-
   for (let browser of gBrowser.browsers) {
     // Don't instantiate lazy browsers.
     if (!browser.isConnected) {
       continue;
     }
 
-    let pmm = browser.messageManager.processMessageManager;
-
-    if (timedOutProcesses.has(pmm)) {
-      continue;
-    }
-
-    let { permitUnload, timedOut } = browser.permitUnload();
-
-    if (timedOut) {
-      timedOutProcesses.add(pmm);
-      continue;
-    }
-
+    let { permitUnload } = browser.permitUnload();
     if (!permitUnload) {
       return false;
     }

browser/base/content/tabbrowser.js

@@ -2129,7 +2129,7 @@
             getter = () => browser.getAttribute("remote") == "true";
             break;
           case "permitUnload":
-            getter = () => () => ({ permitUnload: true, timedOut: false });
+            getter = () => () => ({ permitUnload: true });
             break;
           case "reload":
           case "reloadWithFlags":
@@ -3418,15 +3418,15 @@
         // processes the event queue and may lead to another removeTab()
         // call before permitUnload() returns.
         aTab._pendingPermitUnload = true;
-        let { permitUnload, timedOut } = browser.permitUnload();
-        delete aTab._pendingPermitUnload;
+        let { permitUnload } = browser.permitUnload();
+        aTab._pendingPermitUnload = false;
 
         TelemetryStopwatch.finish("FX_TAB_CLOSE_PERMIT_UNLOAD_TIME_MS", aTab);
 
         // If we were closed during onbeforeunload, we return false now
         // so we don't (try to) close the same tab again. Of course, we
         // also stop if the unload was cancelled by the user:
-        if (aTab.closing || (!timedOut && !permitUnload)) {
+        if (aTab.closing || !permitUnload) {
           return false;
         }
       }

docshell/test/browser/browser_bug1415918_beforeunload_options.js

@@ -6,11 +6,22 @@ const TEST_PATH = getRootDirectory(gTestPath).replace(
   "http://example.com"
 );
 
+SimpleTest.requestFlakyTimeout("Needs to test a timeout");
+
+function delay(msec) {
+  // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
+  return new Promise(resolve => setTimeout(resolve, msec));
+}
+
 add_task(async function test() {
   await SpecialPowers.pushPrefEnv({
     set: [["dom.require_user_interaction_for_beforeunload", false]],
   });
 
+  const permitUnloadTimeout = Services.prefs.getIntPref(
+    "dom.beforeunload_timeout_ms"
+  );
+
   let url = TEST_PATH + "dummy_page.html";
   let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, url);
   let browser = tab.linkedBrowser;
@@ -23,15 +34,22 @@ add_task(async function test() {
 
   let allowNavigation;
   let promptShown = false;
+  let promptDismissed = false;
+  let promptTimeout;
 
   const DIALOG_TOPIC = "tabmodal-dialog-loaded";
-  function observer(node) {
+  async function observer(node) {
     promptShown = true;
 
+    if (promptTimeout) {
+      await delay(promptTimeout);
+    }
+
     let button = node.querySelector(
       `.tabmodalprompt-button${allowNavigation ? 0 : 1}`
     );
     button.click();
+    promptDismissed = true;
   }
   Services.obs.addObserver(observer, DIALOG_TOPIC);
 
@@ -69,6 +87,31 @@ add_task(async function test() {
   );
   ok(!promptShown, "prompt should not have been displayed");
 
+  promptShown = false;
+  promptDismissed = false;
+  promptTimeout = 3 * permitUnloadTimeout;
+  let promise = browser.asyncPermitUnload();
+
+  let promiseResolved = false;
+  promise.then(() => {
+    promiseResolved = true;
+  });
+
+  await TestUtils.waitForCondition(() => promptShown);
+  ok(!promptDismissed, "Should not have dismissed prompt yet");
+  ok(!promiseResolved, "Should not have resolved promise yet");
+
+  await delay(permitUnloadTimeout * 1.5);
+
+  ok(!promptDismissed, "Should not have dismissed prompt yet");
+  ok(!promiseResolved, "Should not have resolved promise yet");
+
+  let { permitUnload } = await promise;
+  ok(promptDismissed, "Should have dismissed prompt");
+  ok(!permitUnload, "Should not have permitted unload");
+
+  promptTimeout = null;
+
   /*
    * Check condition where no one requests a prompt.  In all cases,
    * permitUnload should be true, and all handlers fired.

dom/chrome-webidl/WindowGlobalActors.webidl

@@ -61,8 +61,15 @@ interface WindowGlobalParent : WindowContext {
   // dispatched to them. If any of those request to block the navigation,
   // displays a prompt to the user. Returns a boolean which resolves to true
   // if the navigation should be allowed.
+  //
+  // If `timeout` is greater than 0, it is the maximum time (in milliseconds)
+  // we will wait for a child process to respond with a request to block
+  // navigation before proceeding. If the user needs to be prompted, however,
+  // the promise will not resolve until the user has responded, regardless of
+  // the timeout.
   [Throws]
-  Promise<boolean> permitUnload(optional PermitUnloadAction action = "prompt");
+  Promise<boolean> permitUnload(optional PermitUnloadAction action = "prompt",
+                                optional unsigned long timeout = 0);
 
   // Information about the currently loaded document.
   readonly attribute Principal documentPrincipal;

dom/ipc/WindowGlobalParent.cpp

@@ -42,6 +42,7 @@
 #include "nsSerializationHelper.h"
 #include "nsIBrowser.h"
 #include "nsIPromptCollection.h"
+#include "nsITimer.h"
 #include "nsITransportSecurityInfo.h"
 #include "nsISharePicker.h"
 #include "mozilla/Telemetry.h"
@@ -674,7 +675,8 @@ WindowGlobalParent::RecvSubmitLoadInputEventResponsePreloadTelemetry(
 
 namespace {
 
-class CheckPermitUnloadRequest final : public PromiseNativeHandler {
+class CheckPermitUnloadRequest final : public PromiseNativeHandler,
+                                       public nsITimerCallback {
  public:
   CheckPermitUnloadRequest(WindowGlobalParent* aWGP, bool aHasInProcessBlocker,
                            nsIContentViewer::PermitUnloadAction aAction,
@@ -684,7 +686,7 @@ class CheckPermitUnloadRequest final : public PromiseNativeHandler {
         mAction(aAction),
         mFoundBlocker(aHasInProcessBlocker) {}
 
-  void Run(ContentParent* aIgnoreProcess = nullptr) {
+  void Run(ContentParent* aIgnoreProcess = nullptr, uint32_t aTimeout = 0) {
     MOZ_ASSERT(mState == State::UNINITIALIZED);
     mState = State::WAITING;
 
@@ -719,6 +721,11 @@ class CheckPermitUnloadRequest final : public PromiseNativeHandler {
       }
     });
 
+    if (mPendingRequests && aTimeout) {
+      Unused << NS_NewTimerWithCallback(getter_AddRefs(mTimer), this, aTimeout,
+                                        nsITimer::TYPE_ONE_SHOT);
+    }
+
     CheckDoneWaiting();
   }
 
@@ -727,16 +734,30 @@ class CheckPermitUnloadRequest final : public PromiseNativeHandler {
     CheckDoneWaiting();
   }
 
+  NS_IMETHODIMP Notify(nsITimer* aTimer) override {
+    MOZ_ASSERT(aTimer == mTimer);
+    if (mState == State::WAITING) {
+      mState = State::TIMED_OUT;
+      CheckDoneWaiting();
+    }
+    return NS_OK;
+  }
+
   void CheckDoneWaiting() {
     // If we've found a blocker, we prompt immediately without waiting for
     // further responses. The user's response applies to the entire navigation
     // attempt, regardless of how many "beforeunload" listeners we call.
-    if (mState != State::WAITING || (mPendingRequests && !mFoundBlocker)) {
+    if (mState != State::TIMED_OUT &&
+        (mState != State::WAITING || (mPendingRequests && !mFoundBlocker))) {
       return;
     }
 
     mState = State::PROMPTING;
 
+    // Clearing our reference to the timer will automatically cancel it if it's
+    // still running.
+    mTimer = nullptr;
+
     if (!mFoundBlocker) {
       SendReply(true);
       return;
@@ -815,6 +836,7 @@ class CheckPermitUnloadRequest final : public PromiseNativeHandler {
   enum class State : uint8_t {
     UNINITIALIZED,
     WAITING,
+    TIMED_OUT,
     PROMPTING,
     REPLIED,
   };
@@ -822,6 +844,7 @@ class CheckPermitUnloadRequest final : public PromiseNativeHandler {
   std::function<void(bool)> mResolver;
 
   RefPtr<WindowGlobalParent> mWGP;
+  nsCOMPtr<nsITimer> mTimer;
 
   uint32_t mPendingRequests = 0;
 
@@ -832,7 +855,7 @@ class CheckPermitUnloadRequest final : public PromiseNativeHandler {
   bool mFoundBlocker = false;
 };
 
-NS_IMPL_ISUPPORTS0(CheckPermitUnloadRequest)
+NS_IMPL_ISUPPORTS(CheckPermitUnloadRequest, nsITimerCallback)
 
 }  // namespace
 
@@ -852,7 +875,7 @@ mozilla::ipc::IPCResult WindowGlobalParent::RecvCheckPermitUnload(
 }
 
 already_AddRefed<Promise> WindowGlobalParent::PermitUnload(
-    PermitUnloadAction aAction, mozilla::ErrorResult& aRv) {
+    PermitUnloadAction aAction, uint32_t aTimeout, mozilla::ErrorResult& aRv) {
   nsIGlobalObject* global = GetParentObject();
   RefPtr<Promise> promise = Promise::Create(global, aRv);
   if (NS_WARN_IF(aRv.Failed())) {
@@ -863,7 +886,7 @@ already_AddRefed<Promise> WindowGlobalParent::PermitUnload(
       this, /* aHasInProcessBlocker */ false,
       nsIContentViewer::PermitUnloadAction(aAction),
       [promise](bool aAllow) { promise->MaybeResolve(aAllow); });
-  request->Run();
+  request->Run(/* aIgnoreProcess */ nullptr, aTimeout);
 
   return promise.forget();
 }

dom/ipc/WindowGlobalParent.h

@@ -145,7 +145,8 @@ class WindowGlobalParent final : public WindowContext,
   bool IsInitialDocument() { return mIsInitialDocument; }
 
   already_AddRefed<mozilla::dom::Promise> PermitUnload(
-      PermitUnloadAction aAction, mozilla::ErrorResult& aRv);
+      PermitUnloadAction aAction, uint32_t aTimeout,
+      mozilla::ErrorResult& aRv);
 
   already_AddRefed<mozilla::dom::Promise> DrawSnapshot(
       const DOMRect* aRect, double aScale, const nsACString& aBackgroundColor,

toolkit/content/widgets/browser-custom-element.js

@@ -1717,23 +1717,14 @@
       }
 
       this._inPermitUnload.add(wgp);
-      let timeout;
       try {
-        let result = await Promise.race([
-          new Promise(resolve => {
-            timeout = setTimeout(() => {
-              resolve({ permitUnload: true, timedOut: true });
-            }, lazyPrefs.unloadTimeoutMs);
-          }),
-          wgp
-            .permitUnload(action)
-            .then(permitUnload => ({ permitUnload, timedOut: false })),
-        ]);
-
-        return result;
+        let permitUnload = await wgp.permitUnload(
+          action,
+          lazyPrefs.unloadTimeoutMs
+        );
+        return { permitUnload };
       } finally {
         this._inPermitUnload.delete(wgp);
-        clearTimeout(timeout);
       }
     }
 
@@ -1750,7 +1741,7 @@
     permitUnload(action) {
       if (this.isRemoteBrowser) {
         if (!this.hasBeforeUnload) {
-          return { permitUnload: true, timedOut: false };
+          return { permitUnload: true };
         }
 
         let result;
@@ -1775,11 +1766,10 @@
       }
 
       if (!this.docShell || !this.docShell.contentViewer) {
-        return { permitUnload: true, timedOut: false };
+        return { permitUnload: true };
       }
       return {
         permitUnload: this.docShell.contentViewer.permitUnload(),
-        timedOut: false,
       };
     }