deps: fixing microtask behavior with multiple contexts

Initially we were handling microtasks per-context since our promise
interface relies on per-library callbacks, but the v8 API actually
deals with microtasks on a per-isolate level. This meant that we
weren't properly firing microtasks / promise resolutions from
contexts other than the currently active one.

Now we also handle the microtask queue at the isolate level.

PR-URL: nodejs#539
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>

deps/chakrashim/lib/chakra_shim.js

@@ -409,9 +409,6 @@
     };
   }
 
-  // Simulate v8 micro tasks queue
-  const microTasks = [];
-
   function patchUtils(utils) {
     const isUintRegex = /^(0|[1-9]\d*)$/;
 
@@ -629,14 +626,6 @@
 
     utils.ensureDebug = ensureDebug;
 
-    utils.enqueueMicrotask = function(task) {
-      microTasks.push(task);
-    };
-
-    utils.dequeueMicrotask = function(task) {
-      return microTasks.shift();
-    };
-
     utils.getPropertyAttributes = function(object, value) {
       const descriptor = Object_getOwnPropertyDescriptor(object, value);
       if (descriptor === undefined) {

deps/chakrashim/src/jsrtcachedpropertyidref.inc

@@ -70,8 +70,6 @@ DEF(getStackTrace)
 DEF(getSymbolKeyFor)
 DEF(getSymbolFor)
 DEF(ensureDebug)
-DEF(enqueueMicrotask)
-DEF(dequeueMicrotask)
 DEF(saveInHandleScope)
 DEF(getPropertyAttributes)
 DEF(getOwnPropertyNames)

deps/chakrashim/src/jsrtcontextshim.cc

@@ -95,8 +95,6 @@ ContextShim::ContextShim(IsolateShim * isolateShim,
       getSymbolKeyForFunction(JS_INVALID_REFERENCE),
       getSymbolForFunction(JS_INVALID_REFERENCE),
       ensureDebugFunction(JS_INVALID_REFERENCE),
-      enqueueMicrotaskFunction(JS_INVALID_REFERENCE),
-      dequeueMicrotaskFunction(JS_INVALID_REFERENCE),
       getPropertyAttributesFunction(JS_INVALID_REFERENCE),
       getOwnPropertyNamesFunction(JS_INVALID_REFERENCE),
       jsonParseFunction(JS_INVALID_REFERENCE),
@@ -484,23 +482,6 @@ void ContextShim::SetAlignedPointerInEmbedderData(int index, void * value) {
   }
 }
 
-void ContextShim::RunMicrotasks() {
-  JsValueRef dequeueMicrotaskFunction = GetdequeueMicrotaskFunction();
-
-  for (;;) {
-    JsValueRef task;
-    if (jsrt::CallFunction(dequeueMicrotaskFunction, &task) != JsNoError ||
-      reinterpret_cast<v8::Value*>(task)->IsUndefined()) {
-      break;
-    }
-
-    JsValueRef notUsed;
-    if (jsrt::CallFunction(task, &notUsed) != JsNoError) {
-      JsGetAndClearException(&notUsed);  // swallow any exception from task
-    }
-  }
-}
-
 // check initialization state first instead of calling
 // InitializeCurrentContextShim to save a function call for cases where
 // contextshim is already initialized
@@ -615,8 +596,6 @@ CHAKRASHIM_FUNCTION_GETTER(getStackTrace)
 CHAKRASHIM_FUNCTION_GETTER(getSymbolKeyFor)
 CHAKRASHIM_FUNCTION_GETTER(getSymbolFor)
 CHAKRASHIM_FUNCTION_GETTER(ensureDebug)
-CHAKRASHIM_FUNCTION_GETTER(enqueueMicrotask);
-CHAKRASHIM_FUNCTION_GETTER(dequeueMicrotask);
 CHAKRASHIM_FUNCTION_GETTER(getPropertyAttributes);
 CHAKRASHIM_FUNCTION_GETTER(getOwnPropertyNames);
 CHAKRASHIM_FUNCTION_GETTER(jsonParse);

deps/chakrashim/src/jsrtcontextshim.h

@@ -97,7 +97,6 @@ class ContextShim {
 
   void * GetAlignedPointerFromEmbedderData(int index);
   void SetAlignedPointerInEmbedderData(int index, void * value);
-  void RunMicrotasks();
 
   static ContextShim * GetCurrent();
 
@@ -162,8 +161,6 @@ class ContextShim {
   DECLARE_CHAKRASHIM_FUNCTION_GETTER(getSymbolKeyFor);
   DECLARE_CHAKRASHIM_FUNCTION_GETTER(getSymbolFor);
   DECLARE_CHAKRASHIM_FUNCTION_GETTER(ensureDebug);
-  DECLARE_CHAKRASHIM_FUNCTION_GETTER(enqueueMicrotask);
-  DECLARE_CHAKRASHIM_FUNCTION_GETTER(dequeueMicrotask);
   DECLARE_CHAKRASHIM_FUNCTION_GETTER(getPropertyAttributes);
   DECLARE_CHAKRASHIM_FUNCTION_GETTER(getOwnPropertyNames);
   DECLARE_CHAKRASHIM_FUNCTION_GETTER(jsonParse);

deps/chakrashim/src/jsrtisolateshim.cc

@@ -229,7 +229,8 @@ IsolateShim::IsolateShim(JsRuntimeHandle runtime)
       isDisposing(false),
       contextScopeStack(nullptr),
       tryCatchStackTop(nullptr),
-      embeddedData() {
+      embeddedData(),
+      microtaskQueue() {
   // CHAKRA-TODO: multithread locking for s_isolateList?
   this->prevnext = &s_isolateList;
   this->next = s_isolateList;
@@ -664,6 +665,26 @@ void IsolateShim::SetPromiseRejectCallback(v8::PromiseRejectCallback callback) {
                                    reinterpret_cast<void*>(callback));
 }
 
+void IsolateShim::RunMicrotasks() {
+  // Loop until we've handled all the tasks, including the ones
+  // added by these tasks
+  while (!this->microtaskQueue.empty()) {
+    std::vector<MicroTask> batch;
+    std::swap(batch, this->microtaskQueue);
+
+    for (const MicroTask& mt : batch) {
+      JsValueRef notUsed = JS_INVALID_REFERENCE;
+      if (jsrt::CallFunction(mt.task, &notUsed) != JsNoError) {
+        JsGetAndClearException(&notUsed);  // swallow any exception from task
+      }
+    }
+  }
+}
+
+void IsolateShim::QueueMicrotask(JsValueRef task) {
+  this->microtaskQueue.emplace_back(task);
+}
+
 /*static*/
 bool IsolateShim::RunSingleStepOfReverseMoveLoop(v8::Isolate* isolate,
                                                  uint64_t* moveMode,

deps/chakrashim/src/jsrtisolateshim.h

@@ -122,6 +122,9 @@ class IsolateShim {
     }
   }
 
+  void RunMicrotasks();
+  void QueueMicrotask(JsValueRef task);
+
   JsValueRef GetChakraShimJsArrayBuffer();
   JsValueRef GetChakraInspectorShimJsArrayBuffer();
 
@@ -164,6 +167,27 @@ class IsolateShim {
   void SetPromiseRejectCallback(v8::PromiseRejectCallback callback);
 
  private:
+  struct MicroTask {
+    explicit MicroTask(JsValueRef task) : task(task) {
+      JsAddRef(this->task, nullptr);
+    }
+
+    ~MicroTask() {
+      if (this->task) {
+        JsRelease(this->task, nullptr);
+      }
+    }
+
+    MicroTask(MicroTask&& other)
+      : task(other.task)  {
+        other.task = nullptr;
+    }
+
+    MicroTask(const MicroTask&) = delete;
+
+    JsValueRef task;
+  };
+
   // Construction/Destruction should go thru New/Dispose
   explicit IsolateShim(JsRuntimeHandle runtime);
   ~IsolateShim();
@@ -200,6 +224,7 @@ class IsolateShim {
   uv_timer_t idleGc_timer_handle_;
   bool jsScriptExecuted = false;
   bool isIdleGcScheduled = false;
+  std::vector<MicroTask> microtaskQueue;
 };
 }  // namespace jsrt
 

deps/chakrashim/src/jsrtpromise.cc

@@ -25,11 +25,7 @@ namespace jsrt {
 
 static void CHAKRA_CALLBACK PromiseContinuationCallback(JsValueRef task,
                                                  void *callbackState) {
-  JsValueRef enqueueMicrotaskFunction =
-    ContextShim::GetCurrent()->GetenqueueMicrotaskFunction();
-
-  JsValueRef result;
-  jsrt::CallFunction(enqueueMicrotaskFunction, task, &result);
+  jsrt::IsolateShim::GetCurrent()->QueueMicrotask(task);
 }
 
 JsErrorCode InitializePromise() {

deps/chakrashim/src/v8isolate.cc

@@ -153,7 +153,7 @@ void Isolate::EnqueueMicrotask(MicrotaskCallback microtask, void* data) {
 }
 
 void Isolate::RunMicrotasks() {
-  jsrt::ContextShim::GetCurrent()->RunMicrotasks();
+  jsrt::IsolateShim::FromIsolate(this)->RunMicrotasks();
 }
 
 void Isolate::SetMicrotasksPolicy(MicrotasksPolicy policy) {

test/parallel/parallel.status

@@ -184,6 +184,10 @@ test-v8-stats : SKIP
 # This test uses the V8 untrusted code mitigation and accompanying V8 option
 test-v8-untrusted-code-mitigations : SKIP
 
+# This test should be enabled once chakracore fixes
+# https://github.com/Microsoft/ChakraCore/issues/5196
+test-repl-top-level-await : SKIP
+
 # This test seems to fail under system stress, marking it flaky for now
 # https://github.com/nodejs/node-chakracore/issues/494
 test-async-hooks-destroy-on-gc : PASS,FLAKY

test/parallel/test-promise-context.js

@@ -0,0 +1,32 @@
+'use strict';
+
+require('../common');
+const assert = require('assert');
+const vm = require('vm');
+
+const responses = [];
+function saveResult(x) {
+  responses.push(x);
+}
+
+const p1 = new Promise((resolve, reject) => resolve(1));
+p1.then(saveResult).then(() => { saveResult(6); });
+
+const p2 = new Promise((resolve, reject) => reject(2));
+p2.catch(saveResult);
+
+const p5 = vm.runInNewContext(`
+const p3 = new Promise((resolve, reject) => resolve(3)).then(saveResult);
+p3.then(() => { saveResult(7); });
+const p4 = new Promise((resolve, reject) => reject(4)).catch(saveResult);
+new Promise((resolve, reject) => resolve(5));
+`, { saveResult });
+p5.then(saveResult);
+
+// Note: using setImmediate here since that is a macrotask and will happen
+// after the microtask/promise queue is completely drained.
+setImmediate(() => {
+  console.log(responses);
+  assert.strictEqual(responses.length, 7);
+  assert.deepStrictEqual(responses, [1, 2, 3, 4, 5, 6, 7]);
+});