Bug 1167575. Ensure that we don't exit modal state while we're still waiting on a Gecko runnable in the event-loop-spinning code in nsPrompter.js. r=dolske
authorBoris Zbarsky <bzbarsky@mit.edu>
Thu, 01 Sep 2016 21:31:22 -0400
changeset 312371 6d30a609081db505484615065263515faa225ca1
parent 312370 657ba2f2611c714d718fb0268acfd5e60ff44310
child 312372 b7e5426f358a1ffb9ca6ac458a8537ba794ea52a
push id20447
push userkwierso@gmail.com
push dateFri, 02 Sep 2016 20:36:44 +0000
treeherderfx-team@969397f22187 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
Bug 1167575. Ensure that we don't exit modal state while we're still waiting on a Gecko runnable in the event-loop-spinning code in nsPrompter.js. r=dolske If we synchronously shut down the prompt, that will exit modal state while we're still processing our mouse or key event, which happens _before_ Gecko runnable processing. Exiting modal state will generally post a runnable to run timeouts, so we can end up processing that runnable before we return from processNextEvent and unwind to whatever code did the alert() call on the web page. The upshot is that the webpage will see timeouts fire before the alert() call returns. If we exit modal state off a Gecko runnable instead, that will ensure that we return to the nsPrompter code immediately after exiting modal state, see that the prompt is no longer active, and return to the calling web page code _before_ processing any more runnables.
--- a/browser/base/content/test/tabPrompts/browser_multiplePrompts.js
+++ b/browser/base/content/test/tabPrompts/browser_multiplePrompts.js
@@ -51,16 +51,22 @@ add_task(function*() {
       if (i !== promptsCount) {
         is(prompt.hidden, true, "This prompt should be hidden.");
       is(prompt.hidden, false, "The last prompt should not be hidden.");
+      // The click is handled async; wait for an event loop turn for that to
+      // happen.
+      yield new Promise(function(resolve) {
+        Services.tm.mainThread.dispatch(resolve, Ci.nsIThread.DISPATCH_NORMAL);
+      });
   let prompts = tab.linkedBrowser.parentNode.querySelectorAll("tabmodalprompt");
   is(prompts.length, 0, "Prompts should all be dismissed.");
   yield BrowserTestUtils.removeTab(tab);
--- a/browser/base/content/test/tabPrompts/browser_openPromptInBackgroundTab.js
+++ b/browser/base/content/test/tabPrompts/browser_openPromptInBackgroundTab.js
@@ -37,16 +37,20 @@ add_task(function*() {
   let row = ourPrompt.querySelector("row");
   ok(row, "Should have found the row with our checkbox");
   let checkbox = row.querySelector("checkbox[label*='example.com']");
   ok(checkbox, "The checkbox should be there");
   ok(!checkbox.checked, "Checkbox shouldn't be checked");
   // tick box and accept dialog
   checkbox.checked = true;
+  // Wait for that click to actually be handled completely.
+  yield new Promise(function(resolve) {
+    Services.tm.mainThread.dispatch(resolve, Ci.nsIThread.DISPATCH_NORMAL);
+  });
   // check permission is set
   let ps = Services.perms;
   is(ps.ALLOW_ACTION, ps.testPermission(makeURI(pageWithAlert), "focus-tab-by-prompt"),
      "Tab switching should now be allowed");
   let openedTabSelectedPromise = BrowserTestUtils.waitForAttribute("selected", openedTab, "true");
   // switch to other tab again
   yield BrowserTestUtils.switchTab(gBrowser, firstTab);
--- a/toolkit/components/prompts/content/tabprompts.xml
+++ b/toolkit/components/prompts/content/tabprompts.xml
@@ -265,18 +265,31 @@
         <method name="onButtonClick">
             <parameter name="buttonNum"/>
-                this.Dialog["onButton" + buttonNum]();
-                this.shutdownPrompt();
+                // We want to do all the work her asynchronously off a Gecko
+                // runnable, because of situations like the one described in
+                // https://bugzilla.mozilla.org/show_bug.cgi?id=1167575#c35 : we
+                // get here off processing of an OS event and will also process
+                // one more Gecko runnable before we break out of the event loop
+                // spin whoever posted the prompt is doing.  If we do all our
+                // work sync, we will exit modal state _before_ processing that
+                // runnable, and if exiting moral state posts a runnable we will
+                // incorrectly process that runnable before leaving our event
+                // loop spin.
+                Services.tm.mainThread.dispatch(() => {
+                    this.Dialog["onButton" + buttonNum]();
+                    this.shutdownPrompt();
+                  },
+                  Ci.nsIThread.DISPATCH_NORMAL);
         <method name="onKeyAction">
             <parameter name="action"/>
             <parameter name="event"/>