From e618459788481f7d4e2b7f53da7c35ed30399bf6 Mon Sep 17 00:00:00 2001
From: Gus Caplan <me@gus.host>
Date: Thu, 1 Feb 2018 09:44:27 -0600
Subject: [PATCH] loader: fix up #18394

This commit fixes up some issues in #18394.

* Switch vm.Module internals to use the new link method properly
* Fix bug with ModuleWrap::Link
* Add tests for ModuleWrap::Link

PR-URL: https://github.com/nodejs/node/pull/18509
Fixes: https://github.com/nodejs/node/issues/18249
Refs: https://github.com/nodejs/node/pull/18394
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
---
 lib/internal/loader/ModuleWrap.js          |  5 +++
 lib/internal/vm/Module.js                  | 36 +++++++++++-----------
 node.gyp                                   |  1 +
 src/module_wrap.cc                         |  2 +-
 test/parallel/test-internal-module-wrap.js | 29 +++++++++++++++++
 5 files changed, 54 insertions(+), 19 deletions(-)
 create mode 100644 lib/internal/loader/ModuleWrap.js
 create mode 100644 test/parallel/test-internal-module-wrap.js

diff --git a/lib/internal/loader/ModuleWrap.js b/lib/internal/loader/ModuleWrap.js
new file mode 100644
index 00000000000000..b2b11daead7dde
--- /dev/null
+++ b/lib/internal/loader/ModuleWrap.js
@@ -0,0 +1,5 @@
+'use strict';
+
+// exposes ModuleWrap for testing
+
+module.exports = internalBinding('module_wrap').ModuleWrap;
diff --git a/lib/internal/vm/Module.js b/lib/internal/vm/Module.js
index 2f139d8b260f86..a8fb7303aec131 100644
--- a/lib/internal/vm/Module.js
+++ b/lib/internal/vm/Module.js
@@ -8,6 +8,7 @@ const {
   getConstructorOf,
   customInspectSymbol,
 } = require('internal/util');
+const { SafePromise } = require('internal/safe_globals');
 
 const {
   ModuleWrap,
@@ -131,27 +132,26 @@ class Module {
     const wrap = wrapMap.get(this);
     if (wrap.getStatus() !== kUninstantiated)
       throw new errors.Error('ERR_VM_MODULE_STATUS', 'must be uninstantiated');
+
     linkingStatusMap.set(this, 'linking');
-    const promises = [];
-    wrap.link((specifier) => {
-      const p = (async () => {
-        const m = await linker(specifier, this);
-        if (!m || !wrapMap.has(m))
-          throw new errors.Error('ERR_VM_MODULE_NOT_MODULE');
-        if (m.context !== this.context)
-          throw new errors.Error('ERR_VM_MODULE_DIFFERENT_CONTEXT');
-        const childLinkingStatus = linkingStatusMap.get(m);
-        if (childLinkingStatus === 'errored')
-          throw new errors.Error('ERR_VM_MODULE_LINKING_ERRORED');
-        if (childLinkingStatus === 'unlinked')
-          await m.link(linker);
-        return wrapMap.get(m);
-      })();
-      promises.push(p);
-      return p;
+
+    const promises = wrap.link(async (specifier) => {
+      const m = await linker(specifier, this);
+      if (!m || !wrapMap.has(m))
+        throw new errors.Error('ERR_VM_MODULE_NOT_MODULE');
+      if (m.context !== this.context)
+        throw new errors.Error('ERR_VM_MODULE_DIFFERENT_CONTEXT');
+      const childLinkingStatus = linkingStatusMap.get(m);
+      if (childLinkingStatus === 'errored')
+        throw new errors.Error('ERR_VM_MODULE_LINKING_ERRORED');
+      if (childLinkingStatus === 'unlinked')
+        await m.link(linker);
+      return wrapMap.get(m);
     });
+
     try {
-      await Promise.all(promises);
+      if (promises !== undefined)
+        await SafePromise.all(promises);
       linkingStatusMap.set(this, 'linked');
     } catch (err) {
       linkingStatusMap.set(this, 'errored');
diff --git a/node.gyp b/node.gyp
index 160303f69fc589..9c398284939b50 100644
--- a/node.gyp
+++ b/node.gyp
@@ -107,6 +107,7 @@
       'lib/internal/loader/DefaultResolve.js',
       'lib/internal/loader/ModuleJob.js',
       'lib/internal/loader/ModuleMap.js',
+      'lib/internal/loader/ModuleWrap.js',
       'lib/internal/loader/Translators.js',
       'lib/internal/safe_globals.js',
       'lib/internal/net.js',
diff --git a/src/module_wrap.cc b/src/module_wrap.cc
index 0fda1250d701c0..4a9c847a6a0836 100644
--- a/src/module_wrap.cc
+++ b/src/module_wrap.cc
@@ -209,7 +209,7 @@ void ModuleWrap::Link(const FunctionCallbackInfo<Value>& args) {
     Local<Promise> resolve_promise = resolve_return_value.As<Promise>();
     obj->resolve_cache_[specifier_std].Reset(env->isolate(), resolve_promise);
 
-    promises->Set(mod_context, specifier, resolve_promise).FromJust();
+    promises->Set(mod_context, i, resolve_promise).FromJust();
   }
 
   args.GetReturnValue().Set(promises);
diff --git a/test/parallel/test-internal-module-wrap.js b/test/parallel/test-internal-module-wrap.js
new file mode 100644
index 00000000000000..634d1ebc6f678e
--- /dev/null
+++ b/test/parallel/test-internal-module-wrap.js
@@ -0,0 +1,29 @@
+'use strict';
+
+// Flags: --expose-internals
+
+const common = require('../common');
+common.crashOnUnhandledRejection();
+const assert = require('assert');
+
+const ModuleWrap = require('internal/loader/ModuleWrap');
+const { getPromiseDetails, isPromise } = process.binding('util');
+const setTimeoutAsync = require('util').promisify(setTimeout);
+
+const foo = new ModuleWrap('export * from "bar"; 6;', 'foo');
+const bar = new ModuleWrap('export const five = 5', 'bar');
+
+(async () => {
+  const promises = foo.link(() => setTimeoutAsync(1000).then(() => bar));
+  assert.strictEqual(promises.length, 1);
+  assert(isPromise(promises[0]));
+
+  await Promise.all(promises);
+
+  assert.strictEqual(getPromiseDetails(promises[0])[1], bar);
+
+  foo.instantiate();
+
+  assert.strictEqual(await foo.evaluate(), 6);
+  assert.strictEqual(foo.namespace().five, 5);
+})();