Bug 888753: Reschedule walker loop more often to avoid deadlocks with nested event loops; r=paolo

toolkit/modules/Promise.jsm

@@ -282,6 +282,16 @@ this.PromiseWalker = {
     }
   },
 
+  /**
+   * Sets up the PromiseWalker loop to start on the next tick of the event loop
+   */
+  scheduleWalkerLoop: function()
+  {
+    this.walkerLoopScheduled = true;
+    Services.tm.currentThread.dispatch(this.walkerLoop,
+                                       Ci.nsIThread.DISPATCH_NORMAL);
+  },
+
   /**
    * Schedules the resolution or rejection handlers registered on the provided
    * promise for processing.
@@ -300,9 +310,7 @@ this.PromiseWalker = {
 
     // Schedule the walker loop on the next tick of the event loop.
     if (!this.walkerLoopScheduled) {
-      this.walkerLoopScheduled = true;
-      Services.tm.currentThread.dispatch(this.walkerLoop,
-                                         Ci.nsIThread.DISPATCH_NORMAL);
+      this.scheduleWalkerLoop();
     }
   },
 
@@ -321,15 +329,21 @@ this.PromiseWalker = {
    */
   walkerLoop: function ()
   {
-    // Allow rescheduling the walker loop immediately.  This makes this walker
-    // resilient to the case where one handler does not return, but starts a
-    // nested event loop.  In that case, the newly scheduled walker will take
-    // over.  In the common case, the newly scheduled walker will be invoked
+    // If there is more than one handler waiting, reschedule the walker loop
+    // immediately.  Otherwise, use walkerLoopScheduled to tell schedulePromise()
+    // to reschedule the loop if it adds more handlers to the queue.  This makes
+    // this walker resilient to the case where one handler does not return, but
+    // starts a nested event loop.  In that case, the newly scheduled walker will
+    // take over.  In the common case, the newly scheduled walker will be invoked
     // after this one has returned, with no actual handler to process.  This
     // small overhead is required to make nested event loops work correctly, but
     // occurs at most once per resolution chain, thus having only a minor
     // impact on overall performance.
-    this.walkerLoopScheduled = false;
+    if (this.handlers.length > 1) {
+      this.scheduleWalkerLoop();
+    } else {
+      this.walkerLoopScheduled = false;
+    }
 
     // Process all the known handlers eagerly.
     while (this.handlers.length > 0) {

toolkit/modules/tests/xpcshell/test_Promise.js

@@ -3,6 +3,7 @@
 "use strict";
 
 Components.utils.import("resource://gre/modules/Promise.jsm");
+Components.utils.import("resource://gre/modules/Services.jsm");
 
 ////////////////////////////////////////////////////////////////////////////////
 //// Test runner
@@ -661,6 +662,59 @@ tests.push(
     return Promise.all([p1, p2]);
   }));
 
+// Test deadlock in Promise.jsm with nested event loops
+// The scenario being tested is:
+// promise_1.then({
+//   do some work that will asynchronously signal done
+//   start an event loop waiting for the done signal
+// }
+// where the async work uses resolution of a second promise to 
+// trigger the "done" signal. While this would likely work in a
+// naive implementation, our constant-stack implementation needs
+// a special case to avoid deadlock. Note that this test is
+// sensitive to the implementation-dependent order in which then()
+// clauses for two different promises are executed, so it is
+// possible for other implementations to pass this test and still
+// have similar deadlocks.
+tests.push(
+  make_promise_test(function promise_nested_eventloop_deadlock(test) {
+    // Set up a (long enough to be noticeable) timeout to
+    // exit the nested event loop and throw if the test run is hung
+    let shouldExitNestedEventLoop = false;
+
+    function event_loop() {
+      let thr = Services.tm.mainThread;
+      while(!shouldExitNestedEventLoop) {
+        thr.processNextEvent(true);
+      }
+    }
+
+    // I wish there was a way to cancel xpcshell do_timeout()s
+    do_timeout(2000, () => {
+      if (!shouldExitNestedEventLoop) {
+        shouldExitNestedEventLoop = true;
+        do_throw("Test timed out");
+      }
+    });
+
+    let promise1 = Promise.resolve(1);
+    let promise2 = Promise.resolve(2);
+
+    do_print("Setting wait for first promise");
+    promise1.then(value => {
+      do_print("Starting event loop");
+      event_loop();
+    }, null);
+
+    do_print("Setting wait for second promise");
+    return promise2.then(null, error => {return 3;})
+    .then(
+      count => {
+        shouldExitNestedEventLoop = true;
+      });
+  }));
+
+
 function run_test()
 {
   do_test_pending();