From 1b6a655371babd5a99d85629b08e14dcb1d75a59 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Tue, 13 May 2025 11:35:36 +0100 Subject: [PATCH 1/3] vm: import call should return a promise in the current context A `import` call should returns a promise created in the context where the `import` was called, not the context of `importModuleDynamically` callback. --- src/module_wrap.cc | 23 +++--- .../test-vm-module-dynamic-import-promise.js | 72 +++++++++++++++++++ 2 files changed, 87 insertions(+), 8 deletions(-) create mode 100644 test/parallel/test-vm-module-dynamic-import-promise.js diff --git a/src/module_wrap.cc b/src/module_wrap.cc index ac9840cecb3ed5..1c7e9ff5a55474 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -1020,16 +1020,23 @@ static MaybeLocal ImportModuleDynamicallyWithPhase( }; Local result; - if (import_callback->Call( - context, - Undefined(isolate), - arraysize(import_args), - import_args).ToLocal(&result)) { - CHECK(result->IsPromise()); - return handle_scope.Escape(result.As()); + if (!import_callback + ->Call( + context, Undefined(isolate), arraysize(import_args), import_args) + .ToLocal(&result)) { + return {}; } - return MaybeLocal(); + // Wrap the returned value in a promise created in the referrer context to + // avoid dynamic scopes. + Local resolver; + if (!Promise::Resolver::New(context).ToLocal(&resolver)) { + return {}; + } + if (resolver->Resolve(context, result).IsNothing()) { + return {}; + } + return handle_scope.Escape(resolver->GetPromise()); } static MaybeLocal ImportModuleDynamically( diff --git a/test/parallel/test-vm-module-dynamic-import-promise.js b/test/parallel/test-vm-module-dynamic-import-promise.js new file mode 100644 index 00000000000000..b1ab99fdbe1988 --- /dev/null +++ b/test/parallel/test-vm-module-dynamic-import-promise.js @@ -0,0 +1,72 @@ +// Flags: --experimental-vm-modules +'use strict'; + +const common = require('../common'); + +const assert = require('assert'); +const { createContext, Script, SourceTextModule } = require('vm'); + +// Verifies that a `import` call returns a promise created in the context +// where the `import` was called, not the context of `importModuleDynamically` +// callback. + +async function testScript() { + const ctx = createContext(); + + const mod1 = new SourceTextModule('export const a = 1;', { + context: ctx, + }); + // No import statements, so must not link statically. + await mod1.link(common.mustNotCall()); + + const script2 = new Script(` + const promise = import("mod1"); + if (Object.getPrototypeOf(promise) !== Promise.prototype) { + throw new Error('Expected promise to be a Promise'); + } + globalThis.__result = promise; + `, { + importModuleDynamically: common.mustCall((specifier, referrer) => { + assert.strictEqual(specifier, 'mod1'); + assert.strictEqual(referrer, script2); + return mod1; + }), + }); + script2.runInContext(ctx); + + // Wait for the promise to resolve. + await ctx.__result; +} + +async function testModule() { + const ctx = createContext(); + + const mod1 = new SourceTextModule('export const a = 1;', { + context: ctx, + }); + // No import statements, so must not link statically. + await mod1.link(common.mustNotCall()); + + const mod2 = new SourceTextModule(` + const promise = import("mod1"); + if (Object.getPrototypeOf(promise) !== Promise.prototype) { + throw new Error('Expected promise to be a Promise'); + } + await promise; + `, { + context: ctx, + importModuleDynamically: common.mustCall((specifier, referrer) => { + assert.strictEqual(specifier, 'mod1'); + assert.strictEqual(referrer, mod2); + return mod1; + }), + }); + // No import statements, so must not link statically. + await mod2.link(common.mustNotCall()); + await mod2.evaluate(); +} + +Promise.all([ + testScript(), + testModule(), +]).then(common.mustCall()); From 25e03e6f637113300eb71c6faf718c98b4c575a4 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Tue, 13 May 2025 12:58:58 +0100 Subject: [PATCH 2/3] fixup! vm: import call should return a promise in the current context --- .../test-vm-module-dynamic-import-promise.js | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/test/parallel/test-vm-module-dynamic-import-promise.js b/test/parallel/test-vm-module-dynamic-import-promise.js index b1ab99fdbe1988..6129c85992ea6a 100644 --- a/test/parallel/test-vm-module-dynamic-import-promise.js +++ b/test/parallel/test-vm-module-dynamic-import-promise.js @@ -38,6 +38,34 @@ async function testScript() { await ctx.__result; } +async function testScriptImportFailed() { + const ctx = createContext(); + + const mod1 = new SourceTextModule('export const a = 1;', { + context: ctx, + }); + // No import statements, so must not link statically. + await mod1.link(common.mustNotCall()); + + const script2 = new Script(` + const promise = import("mod1"); + if (Object.getPrototypeOf(promise) !== Promise.prototype) { + throw new Error('Expected promise to be a Promise'); + } + globalThis.__result = promise; + `, { + importModuleDynamically: common.mustCall((specifier, referrer) => { + throw new Error('import failed'); + }), + }); + script2.runInContext(ctx); + + // Wait for the promise to reject. + await assert.rejects(ctx.__result, { + message: 'import failed', + }); +} + async function testModule() { const ctx = createContext(); @@ -66,7 +94,41 @@ async function testModule() { await mod2.evaluate(); } +async function testModuleImportFailed() { + const ctx = createContext(); + + const mod1 = new SourceTextModule('export const a = 1;', { + context: ctx, + }); + // No import statements, so must not link statically. + await mod1.link(common.mustNotCall()); + + const mod2 = new SourceTextModule(` + const promise = import("mod1"); + if (Object.getPrototypeOf(promise) !== Promise.prototype) { + throw new Error('Expected promise to be a Promise'); + } + await promise.then(() => { + throw new Error('Expected promise to be rejected'); + }, (e) => { + if (e.message !== 'import failed') { + throw new Error('Expected promise to be rejected with "import failed"'); + } + }); + `, { + context: ctx, + importModuleDynamically: common.mustCall((specifier, referrer) => { + throw new Error('import failed'); + }), + }); + // No import statements, so must not link statically. + await mod2.link(common.mustNotCall()); + await mod2.evaluate(); +} + Promise.all([ testScript(), + testScriptImportFailed(), testModule(), + testModuleImportFailed(), ]).then(common.mustCall()); From b464995d6f0e06073c0fe38e7de9020cc022afdd Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Wed, 14 May 2025 11:36:40 +0100 Subject: [PATCH 3/3] fixup! vm: import call should return a promise in the current context --- .../test-vm-module-dynamic-import-promise.js | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/test/parallel/test-vm-module-dynamic-import-promise.js b/test/parallel/test-vm-module-dynamic-import-promise.js index 6129c85992ea6a..63493212cd3585 100644 --- a/test/parallel/test-vm-module-dynamic-import-promise.js +++ b/test/parallel/test-vm-module-dynamic-import-promise.js @@ -22,7 +22,7 @@ async function testScript() { const script2 = new Script(` const promise = import("mod1"); if (Object.getPrototypeOf(promise) !== Promise.prototype) { - throw new Error('Expected promise to be a Promise'); + throw new Error('Expected promise to be created in the current context'); } globalThis.__result = promise; `, { @@ -47,23 +47,22 @@ async function testScriptImportFailed() { // No import statements, so must not link statically. await mod1.link(common.mustNotCall()); + const err = new Error('import failed'); const script2 = new Script(` const promise = import("mod1"); if (Object.getPrototypeOf(promise) !== Promise.prototype) { - throw new Error('Expected promise to be a Promise'); + throw new Error('Expected promise to be created in the current context'); } globalThis.__result = promise; `, { importModuleDynamically: common.mustCall((specifier, referrer) => { - throw new Error('import failed'); + throw err; }), }); script2.runInContext(ctx); // Wait for the promise to reject. - await assert.rejects(ctx.__result, { - message: 'import failed', - }); + await assert.rejects(ctx.__result, err); } async function testModule() { @@ -78,7 +77,7 @@ async function testModule() { const mod2 = new SourceTextModule(` const promise = import("mod1"); if (Object.getPrototypeOf(promise) !== Promise.prototype) { - throw new Error('Expected promise to be a Promise'); + throw new Error('Expected promise to be created in the current context'); } await promise; `, { @@ -103,22 +102,24 @@ async function testModuleImportFailed() { // No import statements, so must not link statically. await mod1.link(common.mustNotCall()); + const err = new Error('import failed'); + ctx.__err = err; const mod2 = new SourceTextModule(` const promise = import("mod1"); if (Object.getPrototypeOf(promise) !== Promise.prototype) { - throw new Error('Expected promise to be a Promise'); + throw new Error('Expected promise to be created in the current context'); } await promise.then(() => { throw new Error('Expected promise to be rejected'); }, (e) => { - if (e.message !== 'import failed') { + if (e !== globalThis.__err) { throw new Error('Expected promise to be rejected with "import failed"'); } }); `, { context: ctx, importModuleDynamically: common.mustCall((specifier, referrer) => { - throw new Error('import failed'); + throw err; }), }); // No import statements, so must not link statically.