Bug 1050638 - Should be able to close tab with onbeforeunload warning if clicking close a second time. r=ttaubert, a=lmandel
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Fri, 10 Oct 2014 08:36:51 +0100
changeset 225715 98fc091c4706
parent 225714 6524ec11ce53
child 225716 915073abfd8b
push id3988
push userryanvm@gmail.com
push date2014-10-17 01:37 +0000
treeherdermozilla-beta@c3fa7201e034 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersttaubert, lmandel
bugs1050638
milestone34.0
Bug 1050638 - Should be able to close tab with onbeforeunload warning if clicking close a second time. r=ttaubert, a=lmandel
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
@@ -1145,19 +1145,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]);
+                }
               }
 
               if (!gMultiProcessBrowser)
                 this._adjustFocusAfterTabSwitch(this.mCurrentTab, oldTab);
             }
 
             this.tabContainer._setPositionalAttributes();
 
@@ -1957,33 +1963,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 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 || !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
@@ -290,16 +291,18 @@ skip-if = e10s # Bug ????? - thumbnail c
 [browser_datareporting_notification.js]
 run-if = datareporting
 [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]
 [browser_findbarClose.js]
 skip-if = e10s # Bug ?????? - test directly manipulates content (tries to grab an iframe directly from content)
 [browser_fullscreen-window-open.js]
 skip-if = buildapp == 'mulet' || e10s || os == "linux" # Bug 933103 - mochitest's EventUtils.synthesizeMouse functions not e10s friendly. Linux: Intermittent failures - bug 941575.
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(dialogNode => {
+      waitForDialogDestroyed(dialogNode, () => {
+        let doCompletion = () => setTimeout(resolveOuter, 0);
+        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>