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: #18509
Fixes: #18249
Refs: #18394
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

Author: devsnek

lib/internal/loader/ModuleWrap.js

@@ -0,0 +1,5 @@
+'use strict';
+
+// exposes ModuleWrap for testing
+
+module.exports = internalBinding('module_wrap').ModuleWrap;

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');

node.gyp

@@ -109,6 +109,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',

src/module_wrap.cc

@@ -197,7 +197,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);

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);
+})();