Bug 1050638 - should be able to close tab with onbeforeunload warning if clicking close a second time, r=ttaubert
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Fri, 10 Oct 2014 08:36:51 +0100
changeset 209757 a0ee07b214da2f2ffe82d2d4d7c2d0801fa77c20
parent 209756 ef3a36ab3dedddd4952f3b9d31e0e80dbddeea62
child 209758 1a6016480e7bc562da98883b476b4fae15bd264e
push id27626
push userkwierso@gmail.com
push dateSat, 11 Oct 2014 01:34:27 +0000
treeherdermozilla-central@708b45d9b1b1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersttaubert
bugs1050638
milestone35.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 1050638 - should be able to close tab with onbeforeunload warning if clicking close a second time, r=ttaubert
browser/base/content/tabbrowser.xml
browser/base/content/test/general/browser.ini
browser/base/content/test/general/browser_double_close_tab.js
browser/base/content/test/general/file_double_close_tab.html
--- a/browser/base/content/tabbrowser.xml
+++ b/browser/base/content/tabbrowser.xml
@@ -1157,19 +1157,25 @@
                 // Since the user is switching away from a tab that has
                 // a beforeunload prompt active, we remove the prompt.
                 // This prevents confusing user flows like the following:
                 //   1. User attempts to close Firefox
                 //   2. User switches tabs (ingoring a beforeunload prompt)
                 //   3. User returns to tab, presses "Leave page"
                 let promptBox = this.getTabModalPromptBox(oldBrowser);
                 let prompts = promptBox.listPrompts();
-                // NB: This code assumes that the beforeunload prompt
-                //     is the top-most prompt on the tab.
-                promptBox.removePrompt(prompts[prompts.length - 1]);
+                // There might not be any prompts here if the tab was closed
+                // while in an onbeforeunload prompt, which will have
+                // destroyed aforementioned prompt already, so check there's
+                // something to remove, first:
+                if (prompts.length) {
+                  // NB: This code assumes that the beforeunload prompt
+                  //     is the top-most prompt on the tab.
+                  promptBox.removePrompt(prompts[prompts.length - 1]);
+                }
               }
 
               oldBrowser._urlbarFocused = (gURLBar && gURLBar.focused);
               if (this.isFindBarInitialized(oldTab)) {
                 let findBar = this.getFindBar(oldTab);
                 oldTab._findBarFocused = (!findBar.hidden &&
                   findBar._findField.getAttribute("focused") == "true");
               }
@@ -1933,33 +1939,35 @@
       <method name="_beginRemoveTab">
         <parameter name="aTab"/>
         <parameter name="aTabWillBeMoved"/>
         <parameter name="aCloseWindowWithLastTab"/>
         <parameter name="aCloseWindowFastpath"/>
         <body>
           <![CDATA[
             if (aTab.closing ||
-                aTab._pendingPermitUnload ||
                 this._windowIsClosing)
               return false;
 
             var browser = this.getBrowserForTab(aTab);
-
-            if (!aTabWillBeMoved) {
+            if (!aTab._pendingPermitUnload && !aTabWillBeMoved) {
               let ds = browser.docShell;
               if (ds && ds.contentViewer) {
                 // We need to block while calling permitUnload() because it
                 // processes the event queue and may lead to another removeTab()
                 // call before permitUnload() even returned.
                 aTab._pendingPermitUnload = true;
                 let permitUnload = ds.contentViewer.permitUnload();
                 delete aTab._pendingPermitUnload;
-
-                if (!permitUnload) {
+                // If we were closed during the unload, 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 || !permitUnload) {
+                  // NB: deliberately keep the _closedDuringPermitUnload set to
+                  // true so we keep exiting early in case of multiple calls.
                   return false;
                 }
               }
             }
 
             var closeWindow = false;
             var newTab = false;
             if (this.tabs.length - this._removingTabs.length == 1) {
--- a/browser/base/content/test/general/browser.ini
+++ b/browser/base/content/test/general/browser.ini
@@ -50,16 +50,17 @@ support-files =
   file_bug906190_redirected.html
   file_bug906190.js
   file_bug906190.sjs
   file_bug970276_popup1.html
   file_bug970276_popup2.html
   file_bug970276_favicon1.ico
   file_bug970276_favicon2.ico
   file_dom_notifications.html
+  file_double_close_tab.html
   file_favicon_change.html
   file_fullscreen-window-open.html
   get_user_media.html
   head.js
   healthreport_testRemoteCommands.html
   moz.png
   offlineQuotaNotification.cacheManifest
   offlineQuotaNotification.html
@@ -311,16 +312,18 @@ skip-if = e10s
 run-if = datareporting
 [browser_devedition.js]
 [browser_devices_get_user_media.js]
 skip-if = buildapp == 'mulet' || (os == "linux" && debug) || e10s # linux: bug 976544; e10s: Bug 973001 - appears user media notifications only happen in the child and don't make their way to the parent?
 [browser_devices_get_user_media_about_urls.js]
 skip-if = e10s # Bug 973001 - appears user media notifications only happen in the child and don't make their way to the parent?
 [browser_discovery.js]
 skip-if = e10s # Bug 918663 - DOMLinkAdded events don't make their way to chrome
+[browser_double_close_tab.js]
+skip-if = e10s
 [browser_duplicateIDs.js]
 [browser_drag.js]
 skip-if = true # browser_drag.js is disabled, as it needs to be updated for the new behavior from bug 320638.
 [browser_favicon_change.js]
 skip-if = e10s
 [browser_findbarClose.js]
 skip-if = e10s # Bug ?????? - test directly manipulates content (tries to grab an iframe directly from content)
 [browser_fullscreen-window-open.js]
new file mode 100644
--- /dev/null
+++ b/browser/base/content/test/general/browser_double_close_tab.js
@@ -0,0 +1,80 @@
+"use strict"
+const TEST_PAGE = "http://mochi.test:8888/browser/browser/base/content/test/general/file_double_close_tab.html";
+let testTab;
+
+function waitForDialog(callback) {
+  function onTabModalDialogLoaded(node) {
+    Services.obs.removeObserver(onTabModalDialogLoaded, "tabmodal-dialog-loaded");
+    callback(node);
+  }
+
+  // Listen for the dialog being created
+  Services.obs.addObserver(onTabModalDialogLoaded, "tabmodal-dialog-loaded", false);
+}
+
+function waitForDialogDestroyed(node, callback) {
+  // Now listen for the dialog going away again...
+  let observer = new MutationObserver(function(muts) {
+    if (!node.parentNode) {
+      ok(true, "Dialog is gone");
+      done();
+    }
+  });
+  observer.observe(node.parentNode, {childList: true});
+  let failureTimeout = setTimeout(function() {
+    ok(false, "Dialog should have been destroyed");
+    done();
+  }, 10000);
+
+  function done() {
+    clearTimeout(failureTimeout);
+    observer.disconnect();
+    observer = null;
+    callback();
+  }
+}
+
+add_task(function*() {
+  testTab = gBrowser.selectedTab = gBrowser.addTab();
+  yield promiseTabLoadEvent(testTab, TEST_PAGE);
+  //XXXgijs the reason this has nesting and callbacks rather than promises is
+  // that DOM promises resolve on the next tick. So they're scheduled
+  // in an event queue. So when we spin a new event queue for a modal dialog...
+  // everything gets messed up and the promise's .then callbacks never get
+  // called, despite resolve() being called just fine.
+  let dialogNode = yield new Promise(resolveOuter => {
+    waitForDialog(function(dialogNode) {
+      waitForDialogDestroyed(dialogNode, () => {
+        let doCompletion = () => setTimeout(resolveOuter, 10);
+        info("Now checking if dialog is destroyed");
+        ok(!dialogNode.parentNode, "onbeforeunload dialog should be gone.");
+        if (dialogNode.parentNode) {
+          // Failed to remove onbeforeunload dialog, so do it ourselves:
+          let leaveBtn = dialogNode.ui.button0;
+          waitForDialogDestroyed(dialogNode, doCompletion);
+          EventUtils.synthesizeMouseAtCenter(leaveBtn, {});
+          return;
+        }
+        doCompletion();
+      });
+      // Click again:
+      document.getAnonymousElementByAttribute(testTab, "anonid", "close-button").click();
+    });
+    // Click once:
+    document.getAnonymousElementByAttribute(testTab, "anonid", "close-button").click();
+  });
+  yield promiseWaitForCondition(() => !testTab.parentNode);
+  ok(!testTab.parentNode, "Tab should be closed completely");
+});
+
+registerCleanupFunction(function() {
+  if (testTab.parentNode) {
+    // Remove the handler, or closing this tab will prove tricky:
+    try {
+      testTab.linkedBrowser.contentWindow.onbeforeunload = null;
+    } catch (ex) {}
+    gBrowser.removeTab(testTab);
+  }
+});
+
+
new file mode 100644
--- /dev/null
+++ b/browser/base/content/test/general/file_double_close_tab.html
@@ -0,0 +1,15 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8">
+    <title>Test for bug 1050638 - clicking tab close button twice should close tab even in beforeunload case</title>
+  </head>
+  <body>
+    This page will block beforeunload. It should still be user-closable at all times.
+    <script>
+      window.onbeforeunload = function() {
+        return "stop";
+      };
+    </script>
+  </body>
+</html>