[js] More adjustments to promise callback tracking.

Fixes #1215

Author: jleyba

javascript/node/selenium-webdriver/CHANGES.md

@@ -1,6 +1,7 @@
-## v2.49.0-dev
+## v2.48.2-dev
 
 * Added `WebElement#takeScreenshot()`.
+* More adjustments to promise callback tracking.
 
 ## v2.48.1
 

javascript/node/selenium-webdriver/package.json

@@ -1,6 +1,6 @@
 {
   "name": "selenium-webdriver",
-  "version": "2.49.0-dev",
+  "version": "2.48.2-dev",
   "description": "The official WebDriver JavaScript bindings from the Selenium project",
   "license": "Apache-2.0",
   "keywords": [

javascript/webdriver/promise.js

@@ -215,11 +215,35 @@
  *     // D
  *     // fin
  *
+ * Callbacks attached to an _unresolved_ promise within a task function are
+ * only weakly scheduled as subtasks and will be dropped if they reach the
+ * front of the queue before the promise is resolved. In the example below, the
+ * callbacks for `B` & `D` are dropped as sub-tasks since they are attached to
+ * an unresolved promise when they reach the front of the task queue.
+ *
+ *     var d = promise.defer();
+ *     flow.execute(function() {
+ *       flow.execute(  () => console.log('A'));
+ *       d.promise.then(() => console.log('B'));
+ *       flow.execute(  () => console.log('C'));
+ *       d.promise.then(() => console.log('D'));
+ *
+ *       setTimeout(d.fulfill, 20);
+ *     }).then(function() {
+ *       console.log('fin')
+ *     });
+ *     // A
+ *     // C
+ *     // fin
+ *     // B
+ *     // D
+ *
  * If a promise is resolved while a task function is on the call stack, any
- * previously registered callbacks (i.e. attached while the task was _not_ on
- * the call stack), act as _interrupts_ and are inserted at the front of the
- * task queue. If multiple promises are fulfilled, their interrupts are enqueued
- * in the order the promises are resolved.
+ * previously registered and unqueued callbacks (i.e. either attached while no
+ * task was on the call stack, or previously dropped as described above) act as
+ * _interrupts_ and are inserted at the front of the task queue. If multiple
+ * promises are fulfilled, their interrupts are enqueued in the order the
+ * promises are resolved.
  *
  *     var d1 = promise.defer();
  *     d1.promise.then(() => console.log('A'));
@@ -228,17 +252,23 @@
  *     d2.promise.then(() => console.log('B'));
  *
  *     flow.execute(function() {
- *       flow.execute(() => console.log('C'));
+ *       d1.promise.then(() => console.log('C'));
  *       flow.execute(() => console.log('D'));
+ *     });
+ *     flow.execute(function() {
+ *       flow.execute(() => console.log('E'));
+ *       flow.execute(() => console.log('F'));
  *       d1.fulfill();
  *       d2.fulfill();
  *     }).then(function() {
  *       console.log('fin');
  *     });
+ *     // D
  *     // A
- *     // B
  *     // C
- *     // D
+ *     // B
+ *     // E
+ *     // F
  *     // fin
  *
  * Within a task function (or callback), each step of a promise chain acts as
@@ -321,24 +351,6 @@
  *     // F
  *     // fin
  *
- * __Note__: while the ControlFlow will wait for
- * {@linkplain webdriver.promise.ControlFlow#execute tasks} and
- * {@linkplain webdriver.promise.Promise#then callbacks} to complete, it
- * _will not_ wait for unresolved promises created within a task:
- *
- *     flow.execute(function() {
- *       var p = new promise.Promise(function(fulfill) {
- *         setTimeout(fulfill, 100);
- *       });
- *       p.then(() => console.log('promise resolved!'));
- *       flow.execute(() => console.log('sub-task!'));
- *     }).then(function() {
- *       console.log('task complete!');
- *     });
- *     // sub-task!
- *     // task complete!
- *     // promise resolved!
- *
  * Finally, consider the following:
  *
  *     var d = promise.defer();
@@ -2628,9 +2640,13 @@ var TaskQueue = goog.defineClass(EventEmitter, {
       return;
     }
     promise.callbacks_.forEach(function(cb) {
+      cb.blocked = false;
+      if (cb.queue) {
+        return;
+      }
+
       cb.promise[CANCEL_HANDLER_SYMBOL] = this.onTaskCancelled_.bind(this, cb);
 
-      cb.blocked = false;
       if (cb.queue === this && this.tasks_.indexOf(cb) !== -1) {
         return;
       }

javascript/webdriver/test/promise_flow_test.js

@@ -1248,7 +1248,7 @@ function testWait_unboundedWaitOnPromiseResolution() {
     messages.push('a');
   }, 5);
 
-  webdriver.promise.delayed(10).then(function() {
+  timeout(10).then(function() {
     assertArrayEquals(['a'], messages);
     assertTrue(waitResult.isPending());
     d.fulfill(1234);
@@ -2110,6 +2110,7 @@ function testCancellingAPendingTask() {
 
   unresolved.promise.thenCatch(function(e) {
     order.push(4);
+    assertTrue(e instanceof webdriver.promise.CancellationError);
   });
 
   return timeout(10).then(function() {
@@ -2120,7 +2121,7 @@ function testCancellingAPendingTask() {
     return waitForIdle();
   }).then(function() {
     assertFlowHistory('a', 'a.1', 'b');
-    assertArrayEquals([1, 4, 3], order);
+    assertArrayEquals([1, 3, 4], order);
   });
 }
 
@@ -2339,3 +2340,26 @@ function testTasksDoNotWaitForNewlyCreatedPromises() {
     assertArrayEquals(['b', 'a', 'c', 'd', 'e', 'fin'], order);
   });
 }
+
+function testCallbackDependenciesDoNotDeadlock() {
+  var order = [];
+  var root = webdriver.promise.defer();
+  var dep = webdriver.promise.fulfilled().then(function() {
+    order.push('a');
+    return root.promise.then(function() {
+      order.push('b');
+    });
+  });
+  // This callback depends on |dep|, which depends on another callback
+  // attached to |root| via a chain.
+  root.promise.then(function() {
+    order.push('c');
+    return dep.then(() => order.push('d'));
+  }).then(() => order.push('fin'));
+
+  setTimeout(() => root.fulfill(), 20);
+
+  return waitForIdle().then(function() {
+    assertArrayEquals(['a', 'b', 'c', 'd', 'fin'], order);
+  });
+}

javascript/webdriver/test/webdriver_test.js

@@ -1146,12 +1146,12 @@ function testCustomFunctionDoesNotCompleteUntilReturnedPromiseIsResolved() {
 
   // timeout to ensure the first function starts its execution before we
   // trigger d's callbacks.
-  webdriver.promise.delayed(0).then(function() {
+  return new Promise(f => setTimeout(f, 0)).then(function() {
     assertArrayEquals(['a'], order);
     d.fulfill();
-  });
-  return waitForIdle().then(function() {
-    assertArrayEquals(['a', 'b', 'c'], order);
+    return waitForIdle().then(function() {
+      assertArrayEquals(['a', 'b', 'c'], order);
+    });
   });
 }