[js]: don't drop blocked callbacks from the task queue until they are pulled for

execution

Fixes SeleniumHQ#1207

Author: jleyba

javascript/node/selenium-webdriver/CHANGES.md

@@ -1,4 +1,7 @@
-## v2.49.0-dev
+## v2.48.1
+
+* FIXED: Adjusted how the control flow tracks promise callbacks to avoid a
+    potential deadlock.
 
 ## v2.48.0
 

javascript/node/selenium-webdriver/package.json

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

javascript/webdriver/promise.js

@@ -330,12 +330,12 @@
  *       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!
  *
@@ -1296,7 +1296,7 @@ promise.Promise = goog.defineClass(null, {
         this.callbacks_ = [];
       }
       this.callbacks_.push(cb);
-      cb.isVolatile = true;
+      cb.blocked = true;
       this.flow_.getActiveQueue_().enqueue(cb);
     }
 
@@ -2491,12 +2491,20 @@ var Task = goog.defineClass(promise.Deferred, {
     this.queue = null;
 
     /**
-     * Whether this task is volatile. Volatile tasks may be registered in a
-     * a task queue, but will be dropped on the next turn of the JS event loop
-     * if still marked volatile.
+     * Whether this task is considered block. A blocked task may be registered
+     * in a task queue, but will be dropped if it is still blocked when it
+     * reaches the front of the queue. A dropped task may always be rescheduled.
+     *
+     * Blocked tasks are used when a callback is attached to an unsettled
+     * promise to reserve a spot in line (in a manner of speaking). If the
+     * promise is not settled before the callback reaches the front of the
+     * of the queue, it will be dropped. Once the promise is settled, the
+     * dropped task will be rescheduled as an interrupt on the currently task
+     * queue.
+     *
      * @type {boolean}
      */
-    this.isVolatile = false;
+    this.blocked = false;
 
     if (opt_stackOptions) {
       this.promise.stack_ = promise.captureStackTrace(
@@ -2537,9 +2545,6 @@ var TaskQueue = goog.defineClass(EventEmitter, {
     /** @private {!Array<!Task>} */
     this.tasks_ = [];
 
-    /** @private {Array<!Task>} */
-    this.volatileTasks_ = null;
-
     /** @private {Array<!Task>} */
     this.interrupts_ = null;
 
@@ -2592,11 +2597,6 @@ var TaskQueue = goog.defineClass(EventEmitter, {
       throw Error('Task is already scheduled in another queue');
     }
 
-    if (task.isVolatile) {
-      this.volatileTasks_ = this.volatileTasks_ || [];
-      this.volatileTasks_.push(task);
-    }
-
     this.tasks_.push(task);
     task.queue = this;
     task.promise[CANCEL_HANDLER_SYMBOL] =
@@ -2628,10 +2628,9 @@ var TaskQueue = goog.defineClass(EventEmitter, {
       return;
     }
     promise.callbacks_.forEach(function(cb) {
-      cb.promise[CANCEL_HANDLER_SYMBOL] =
-        this.onTaskCancelled_.bind(this, cb);
+      cb.promise[CANCEL_HANDLER_SYMBOL] = this.onTaskCancelled_.bind(this, cb);
 
-      cb.isVolatile = false;
+      cb.blocked = false;
       if (cb.queue === this && this.tasks_.indexOf(cb) !== -1) {
         return;
       }
@@ -2711,7 +2710,6 @@ var TaskQueue = goog.defineClass(EventEmitter, {
       return;
     }
     this.state_ = TaskQueueState.STARTED;
-    this.dropVolatileTasks_();
 
     if (this.pending_ != null || this.processUnhandledRejections_()) {
       return;
@@ -2762,7 +2760,6 @@ var TaskQueue = goog.defineClass(EventEmitter, {
       return fn();
     } finally {
       this.flow_.activeQueue_ = null;
-      this.dropVolatileTasks_();
       activeFlows.pop();
     }
   },
@@ -2801,23 +2798,6 @@ var TaskQueue = goog.defineClass(EventEmitter, {
     }
   },
 
-  /**
-   * Drops any volatile tasks scheduled within this task queue.
-   * @private
-   */
-  dropVolatileTasks_: function() {
-    if (!this.volatileTasks_) {
-      return;
-    }
-    for (var task of this.volatileTasks_) {
-      if (task.isVolatile) {
-        vlog(2, () => this + ' dropping volatile task ' + task, this);
-        this.dropTask_(task);
-      }
-    }
-    this.volatileTasks_ = null;
-  },
-
   /**
    * @param {!Task} task The task to drop.
    * @private
@@ -2882,13 +2862,21 @@ var TaskQueue = goog.defineClass(EventEmitter, {
    */
   getNextTask_: function() {
     var task = undefined;
-    if (this.interrupts_) {
-      task = this.interrupts_.shift();
-    }
-    if (!task && this.tasks_) {
-      task = this.tasks_.shift();
+    while (true) {
+      if (this.interrupts_) {
+        task = this.interrupts_.shift();
+      }
+      if (!task && this.tasks_) {
+        task = this.tasks_.shift();
+      }
+      if (task && task.blocked) {
+        vlog(2, () => this + ' skipping blocked task ' + task, this);
+        task.queue = null;
+        task = null;
+        continue;
+      }
+      return task;
     }
-    return task;
   }
 });
 

javascript/webdriver/test/promise_flow_test.js

@@ -435,7 +435,7 @@ function testFraming_allCallbacksInAFrameAreScheduledWhenPromiseIsResolved() {
   schedule('e');
 
   return waitForIdle().then(function() {
-    assertFlowHistory('a', 'b', 'd', 'c', 'e');
+    assertFlowHistory('a', 'b', 'c', 'd', 'e');
   });
 }
 
@@ -2093,10 +2093,11 @@ function testCancellingAPendingTask() {
       return unresolved.promise;
     });
 
+    // Since the outerTask is cancelled below, innerTask should be cancelled
+    // with a DiscardedTaskError, which means its callbacks are silently
+    // dropped - so this should never execute.
     innerTask.thenCatch(function(e) {
       order.push(2);
-      assertTrue(e instanceof webdriver.promise.CancellationError);
-      assertEquals('no soup for you', e.message);
     });
   });
   schedule('b');
@@ -2119,7 +2120,7 @@ function testCancellingAPendingTask() {
     return waitForIdle();
   }).then(function() {
     assertFlowHistory('a', 'a.1', 'b');
-    assertArrayEquals([1, 4, 2, 3], order);
+    assertArrayEquals([1, 4, 3], order);
   });
 }
 
@@ -2312,3 +2313,29 @@ function testSettledPromiseCallbacksInsideATask_2() {
     assertArrayEquals(['a', 'c', 'b', 'fin'], order);
   });
 }
+
+function testTasksDoNotWaitForNewlyCreatedPromises() {
+  var order = [];
+
+  flow.execute(function() {
+    var d = webdriver.promise.defer();
+
+    // This is a normal promise, not a task, so the task for this callback is
+    // considered volatile. Volatile tasks should be skipped when they reach
+    // the front of the task queue.
+    d.promise.then(() => order.push('a'));
+
+    flow.execute(() => order.push('b'));
+    flow.execute(function() {
+      flow.execute(() => order.push('c'));
+      d.promise.then(() => order.push('d'));
+      d.fulfill();
+    });
+    flow.execute(() => order.push('e'));
+
+  }).then(() => order.push('fin'));
+
+  return waitForIdle().then(function() {
+    assertArrayEquals(['b', 'a', 'c', 'd', 'e', 'fin'], order);
+  });
+}