From 7c7f30ed17654f29293f586ad009f0f8340213d4 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 16 Sep 2025 18:28:29 +0200 Subject: [PATCH] deps: V8: cherry-pick 3d0f462a17ff Original commit message: [api] Add index-based module resolution in InstantiateModule() Add new InstantiateModule() overload that allows embedders to identify modules requests by index in the module requests array rather than using specifier and import attributes. When embedders want to fetch all the modules using information from module->GetModuleRequests() before calling InstantiateModule() instead of having to do the fetching inside the InstantiateModule() callback, they could just maintain a simple array of modules indexed by module request positions and look up the fetched the module by index in the new callback. Previously this has to be done by mapping from specifier and import attributes to module objects cached on the embedder side, leading to an overhead to hash the specifier and import attributes for each module request. Refs: https://github.com/nodejs/node/pull/59396 Bug: 435317398 Change-Id: Ie017d2f3ccc605e0f58aa423504b5fa5fdbcc633 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6804466 Commit-Queue: Joyee Cheung Reviewed-by: Camillo Bruni Cr-Commit-Position: refs/heads/main@{#102613} Refs: https://github.com/v8/v8/commit/3d0f462a17ffa08869805874ac46726783512fef PR-URL: https://github.com/nodejs/node/pull/59396 Refs: https://chromium-review.googlesource.com/c/v8/v8/+/6804466 Reviewed-By: Chengzhong Wu --- common.gypi | 2 +- deps/v8/include/v8-script.h | 17 + deps/v8/src/api/api.cc | 22 +- deps/v8/src/objects/module.cc | 17 +- deps/v8/src/objects/module.h | 14 +- deps/v8/src/objects/source-text-module.cc | 53 +++- deps/v8/src/objects/source-text-module.h | 3 +- .../unittests/objects/modules-unittest.cc | 292 ++++++++++++++++++ 8 files changed, 386 insertions(+), 34 deletions(-) diff --git a/common.gypi b/common.gypi index 592c4e9294e..63301644b94 100644 --- a/common.gypi +++ b/common.gypi @@ -38,7 +38,7 @@ # Reset this number to 0 on major V8 upgrades. # Increment by one for each non-official patch applied to deps/v8. - 'v8_embedder_string': '-node.10', + 'v8_embedder_string': '-node.11', ##### V8 defaults for Node.js ##### diff --git a/deps/v8/include/v8-script.h b/deps/v8/include/v8-script.h index 9d47b751f27..debe89b5084 100644 --- a/deps/v8/include/v8-script.h +++ b/deps/v8/include/v8-script.h @@ -220,6 +220,13 @@ class V8_EXPORT Module : public Data { Local context, Local specifier, Local import_attributes, Local referrer); + using ResolveModuleByIndexCallback = MaybeLocal (*)( + Local context, size_t module_request_index, + Local referrer); + using ResolveSourceByIndexCallback = MaybeLocal (*)( + Local context, size_t module_request_index, + Local referrer); + /** * Instantiates the module and its dependencies. * @@ -231,6 +238,16 @@ class V8_EXPORT Module : public Data { Local context, ResolveModuleCallback module_callback, ResolveSourceCallback source_callback = nullptr); + /** + * Similar to the variant that takes ResolveModuleCallback and + * ResolveSourceCallback, but uses the index into the array that is returned + * by GetModuleRequests() instead of the specifier and import attributes to + * identify the requests. + */ + V8_WARN_UNUSED_RESULT Maybe InstantiateModule( + Local context, ResolveModuleByIndexCallback module_callback, + ResolveSourceByIndexCallback source_callback = nullptr); + /** * Evaluates the module and its dependencies. * diff --git a/deps/v8/src/api/api.cc b/deps/v8/src/api/api.cc index e5f703bb5e7..1ec43cb202a 100644 --- a/deps/v8/src/api/api.cc +++ b/deps/v8/src/api/api.cc @@ -2266,14 +2266,34 @@ int Module::GetIdentityHash() const { return self->hash(); } +Maybe Module::InstantiateModule( + Local context, ResolveModuleByIndexCallback module_callback, + ResolveSourceByIndexCallback source_callback) { + auto i_isolate = i::Isolate::Current(); + EnterV8Scope<> api_scope{i_isolate, context, + RCCId::kAPI_Module_InstantiateModule}; + + i::Module::UserResolveCallbacks callbacks; + callbacks.module_callback_by_index = module_callback; + callbacks.source_callback_by_index = source_callback; + if (!i::Module::Instantiate(i_isolate, Utils::OpenHandle(this), context, + callbacks)) { + return {}; + } + return Just(true); +} + Maybe Module::InstantiateModule(Local context, ResolveModuleCallback module_callback, ResolveSourceCallback source_callback) { auto i_isolate = i::Isolate::Current(); EnterV8Scope<> api_scope{i_isolate, context, RCCId::kAPI_Module_InstantiateModule}; + i::Module::UserResolveCallbacks callbacks; + callbacks.module_callback = module_callback; + callbacks.source_callback = source_callback; if (!i::Module::Instantiate(i_isolate, Utils::OpenHandle(this), context, - module_callback, source_callback)) { + callbacks)) { return {}; } return Just(true); diff --git a/deps/v8/src/objects/module.cc b/deps/v8/src/objects/module.cc index 6ce67230b41..b232db1fcb3 100644 --- a/deps/v8/src/objects/module.cc +++ b/deps/v8/src/objects/module.cc @@ -205,14 +205,12 @@ MaybeHandle Module::ResolveExport(Isolate* isolate, Handle module, bool Module::Instantiate(Isolate* isolate, Handle module, v8::Local context, - v8::Module::ResolveModuleCallback module_callback, - v8::Module::ResolveSourceCallback source_callback) { + const Module::UserResolveCallbacks& callbacks) { #ifdef DEBUG PrintStatusMessage(*module, "Instantiating module "); #endif // DEBUG - if (!PrepareInstantiate(isolate, module, context, module_callback, - source_callback)) { + if (!PrepareInstantiate(isolate, module, context, callbacks)) { ResetGraph(isolate, module); DCHECK_EQ(module->status(), kUnlinked); return false; @@ -231,11 +229,9 @@ bool Module::Instantiate(Isolate* isolate, Handle module, return true; } -bool Module::PrepareInstantiate( - Isolate* isolate, DirectHandle module, - v8::Local context, - v8::Module::ResolveModuleCallback module_callback, - v8::Module::ResolveSourceCallback source_callback) { +bool Module::PrepareInstantiate(Isolate* isolate, DirectHandle module, + v8::Local context, + const UserResolveCallbacks& callbacks) { DCHECK_NE(module->status(), kEvaluating); DCHECK_NE(module->status(), kLinking); if (module->status() >= kPreLinking) return true; @@ -244,8 +240,7 @@ bool Module::PrepareInstantiate( if (IsSourceTextModule(*module)) { return SourceTextModule::PrepareInstantiate( - isolate, Cast(module), context, module_callback, - source_callback); + isolate, Cast(module), context, callbacks); } else { return SyntheticModule::PrepareInstantiate( isolate, Cast(module), context); diff --git a/deps/v8/src/objects/module.h b/deps/v8/src/objects/module.h index 7e6b3bb690a..cab99ea314b 100644 --- a/deps/v8/src/objects/module.h +++ b/deps/v8/src/objects/module.h @@ -57,14 +57,20 @@ class Module : public TorqueGeneratedModule { // i.e. has a top-level await. V8_WARN_UNUSED_RESULT bool IsGraphAsync(Isolate* isolate) const; + struct UserResolveCallbacks { + v8::Module::ResolveModuleCallback module_callback = nullptr; + v8::Module::ResolveSourceCallback source_callback = nullptr; + v8::Module::ResolveModuleByIndexCallback module_callback_by_index = nullptr; + v8::Module::ResolveSourceByIndexCallback source_callback_by_index = nullptr; + }; + // Implementation of spec operation ModuleDeclarationInstantiation. // Returns false if an exception occurred during instantiation, true // otherwise. (In the case where the callback throws an exception, that // exception is propagated.) static V8_WARN_UNUSED_RESULT bool Instantiate( Isolate* isolate, Handle module, v8::Local context, - v8::Module::ResolveModuleCallback module_callback, - v8::Module::ResolveSourceCallback source_callback); + const UserResolveCallbacks& callbacks); // Implementation of spec operation ModuleEvaluation. static V8_WARN_UNUSED_RESULT MaybeDirectHandle Evaluate( @@ -99,9 +105,7 @@ class Module : public TorqueGeneratedModule { static V8_WARN_UNUSED_RESULT bool PrepareInstantiate( Isolate* isolate, DirectHandle module, - v8::Local context, - v8::Module::ResolveModuleCallback module_callback, - v8::Module::ResolveSourceCallback source_callback); + v8::Local context, const UserResolveCallbacks& callbacks); static V8_WARN_UNUSED_RESULT bool FinishInstantiate( Isolate* isolate, Handle module, ZoneForwardList>* stack, unsigned* dfs_index, diff --git a/deps/v8/src/objects/source-text-module.cc b/deps/v8/src/objects/source-text-module.cc index 1237c90a1e2..7e87cd010b0 100644 --- a/deps/v8/src/objects/source-text-module.cc +++ b/deps/v8/src/objects/source-text-module.cc @@ -346,9 +346,10 @@ MaybeHandle SourceTextModule::ResolveExportUsingStarExports( bool SourceTextModule::PrepareInstantiate( Isolate* isolate, DirectHandle module, v8::Local context, - v8::Module::ResolveModuleCallback module_callback, - v8::Module::ResolveSourceCallback source_callback) { - DCHECK_NE(module_callback, nullptr); + const Module::UserResolveCallbacks& callbacks) { + // One of the callbacks must be set, otherwise we cannot resolve. + DCHECK_IMPLIES(callbacks.module_callback == nullptr, + callbacks.module_callback_by_index != nullptr); // Obtain requested modules. DirectHandle module_info(module->info(), isolate); DirectHandle module_requests(module_info->module_requests(), @@ -364,11 +365,23 @@ bool SourceTextModule::PrepareInstantiate( switch (module_request->phase()) { case ModuleImportPhase::kEvaluation: { v8::Local api_requested_module; - if (!module_callback(context, v8::Utils::ToLocal(specifier), - v8::Utils::FixedArrayToLocal(import_attributes), - v8::Utils::ToLocal(Cast(module))) - .ToLocal(&api_requested_module)) { - return false; + if (callbacks.module_callback != nullptr) { + if (!callbacks + .module_callback( + context, v8::Utils::ToLocal(specifier), + v8::Utils::FixedArrayToLocal(import_attributes), + v8::Utils::ToLocal(Cast(module))) + .ToLocal(&api_requested_module)) { + return false; + } + } else { + DCHECK_NOT_NULL(callbacks.module_callback_by_index); + if (!callbacks + .module_callback_by_index( + context, i, v8::Utils::ToLocal(Cast(module))) + .ToLocal(&api_requested_module)) { + return false; + } } DirectHandle requested_module = Utils::OpenDirectHandle(*api_requested_module); @@ -378,11 +391,23 @@ bool SourceTextModule::PrepareInstantiate( case ModuleImportPhase::kSource: { DCHECK(v8_flags.js_source_phase_imports); v8::Local api_requested_module_source; - if (!source_callback(context, v8::Utils::ToLocal(specifier), - v8::Utils::FixedArrayToLocal(import_attributes), - v8::Utils::ToLocal(Cast(module))) - .ToLocal(&api_requested_module_source)) { - return false; + if (callbacks.source_callback != nullptr) { + if (!callbacks + .source_callback( + context, v8::Utils::ToLocal(specifier), + v8::Utils::FixedArrayToLocal(import_attributes), + v8::Utils::ToLocal(Cast(module))) + .ToLocal(&api_requested_module_source)) { + return false; + } + } else { + DCHECK_NOT_NULL(callbacks.source_callback_by_index); + if (!callbacks + .source_callback_by_index( + context, i, v8::Utils::ToLocal(Cast(module))) + .ToLocal(&api_requested_module_source)) { + return false; + } } DirectHandle requested_module_source = Utils::OpenDirectHandle(*api_requested_module_source); @@ -404,7 +429,7 @@ bool SourceTextModule::PrepareInstantiate( DirectHandle requested_module( Cast(requested_modules->get(i)), isolate); if (!Module::PrepareInstantiate(isolate, requested_module, context, - module_callback, source_callback)) { + callbacks)) { return false; } } diff --git a/deps/v8/src/objects/source-text-module.h b/deps/v8/src/objects/source-text-module.h index 05b4576bfa4..e74de1ecfc1 100644 --- a/deps/v8/src/objects/source-text-module.h +++ b/deps/v8/src/objects/source-text-module.h @@ -172,8 +172,7 @@ class SourceTextModule static V8_WARN_UNUSED_RESULT bool PrepareInstantiate( Isolate* isolate, DirectHandle module, v8::Local context, - v8::Module::ResolveModuleCallback module_callback, - v8::Module::ResolveSourceCallback source_callback); + const Module::UserResolveCallbacks& callbacks); static V8_WARN_UNUSED_RESULT bool FinishInstantiate( Isolate* isolate, Handle module, ZoneForwardList>* stack, unsigned* dfs_index, diff --git a/deps/v8/test/unittests/objects/modules-unittest.cc b/deps/v8/test/unittests/objects/modules-unittest.cc index 08663f975e6..07a6fee32b6 100644 --- a/deps/v8/test/unittests/objects/modules-unittest.cc +++ b/deps/v8/test/unittests/objects/modules-unittest.cc @@ -6,6 +6,9 @@ #include "src/flags/flags.h" #include "test/unittests/test-utils.h" #include "testing/gtest/include/gtest/gtest.h" +#if V8_ENABLE_WEBASSEMBLY +#include "include/v8-wasm.h" +#endif // V8_ENABLE_WEBASSEMBLY namespace { @@ -1259,4 +1262,293 @@ TEST_F(ModuleTest, AsyncEvaluatingInEvaluateEntryPoint) { CHECK_EQ(v8::Promise::kFulfilled, promise1->State()); } +// Test data for index-based module resolution +static std::vector> index_modules_global; + +static MaybeLocal ResolveModuleByIndexCallback( + Local context, size_t module_request_index, + Local referrer) { + Isolate* isolate = Isolate::GetCurrent(); + CHECK_LE(module_request_index, index_modules_global.size()); + return index_modules_global[module_request_index].Get(isolate); +} + +static MaybeLocal ResolveSourceByIndexUnreachableCallback( + Local context, size_t module_request_index, + Local referrer) { + UNREACHABLE(); +} + +TEST_F(ModuleTest, ModuleInstantiationByIndex) { + HandleScope scope(isolate()); + v8::TryCatch try_catch(isolate()); + + Local module; + { + Local source_text = NewString( + "import { x } from './dep1.js';\n" + "export { y } from './dep2.js';\n" + "export const z = x;\n"); + ScriptOrigin origin = ModuleOrigin(NewString("main.js"), isolate()); + ScriptCompiler::Source source(source_text, origin); + module = ScriptCompiler::CompileModule(isolate(), &source).ToLocalChecked(); + CHECK_EQ(Module::kUninstantiated, module->GetStatus()); + + Local module_requests = module->GetModuleRequests(); + CHECK_EQ(2, module_requests->Length()); + + // Verify the requests are in expected order + Local module_request_0 = + module_requests->Get(context(), 0).As(); + CHECK( + NewString("./dep1.js")->StrictEquals(module_request_0->GetSpecifier())); + + Local module_request_1 = + module_requests->Get(context(), 1).As(); + CHECK( + NewString("./dep2.js")->StrictEquals(module_request_1->GetSpecifier())); + } + + // Create dependency modules to be resolved by index + { + Local source_text = NewString("export const x = 42;"); + ScriptOrigin origin = ModuleOrigin(NewString("dep1.js"), isolate()); + ScriptCompiler::Source source(source_text, origin); + Local dep1 = + ScriptCompiler::CompileModule(isolate(), &source).ToLocalChecked(); + index_modules_global.emplace_back(isolate(), dep1); + } + + { + Local source_text = NewString("export const y = 24;"); + ScriptOrigin origin = ModuleOrigin(NewString("dep2.js"), isolate()); + ScriptCompiler::Source source(source_text, origin); + Local dep2 = + ScriptCompiler::CompileModule(isolate(), &source).ToLocalChecked(); + index_modules_global.emplace_back(isolate(), dep2); + } + + // Instantiate using index-based callback + CHECK(module + ->InstantiateModule(context(), ResolveModuleByIndexCallback, + ResolveSourceByIndexUnreachableCallback) + .FromJust()); + CHECK_EQ(Module::kInstantiated, module->GetStatus()); + + // Verify evaluation works + MaybeLocal result = module->Evaluate(context()); + CHECK_EQ(Module::kEvaluated, module->GetStatus()); + Local promise = Local::Cast(result.ToLocalChecked()); + CHECK_EQ(promise->State(), v8::Promise::kFulfilled); + + CHECK(!try_catch.HasCaught()); + + CHECK_EQ(42, module->GetModuleNamespace() + .As() + ->Get(context(), NewString("z")) + .ToLocalChecked() + ->Int32Value(context()) + .ToChecked()); + CHECK_EQ(24, module->GetModuleNamespace() + .As() + ->Get(context(), NewString("y")) + .ToLocalChecked() + ->Int32Value(context()) + .ToChecked()); + // Clean up + for (auto& mod : index_modules_global) { + mod.Reset(); + } + index_modules_global.clear(); +} + +static bool resolve_module_by_index_failure_called = false; +static MaybeLocal ResolveModuleByIndexFailureCallback( + Local context, size_t module_request_index, + Local referrer) { + CHECK(!resolve_module_by_index_failure_called); + resolve_module_by_index_failure_called = true; + v8::Isolate* isolate = v8::Isolate::GetCurrent(); + std::string message = + "module " + std::to_string(module_request_index) + " not found"; + isolate->ThrowException( + String::NewFromUtf8(isolate, message.c_str()).ToLocalChecked()); + return MaybeLocal(); +} + +TEST_F(ModuleTest, ModuleInstantiationByIndexFailure) { + HandleScope scope(isolate()); + v8::TryCatch try_catch(isolate()); + + Local module; + { + Local source_text = NewString( + "import './dep1.js';\n" + "import './dep2.js';\n" + "import './dep3.js';"); + ScriptOrigin origin = ModuleOrigin(NewString("main.js"), isolate()); + ScriptCompiler::Source source(source_text, origin); + module = ScriptCompiler::CompileModule(isolate(), &source).ToLocalChecked(); + CHECK_EQ(Module::kUninstantiated, module->GetStatus()); + + Local module_requests = module->GetModuleRequests(); + CHECK_EQ(3, module_requests->Length()); + } + + // Instantiation should fail and the callback should only be called once. + { + v8::TryCatch inner_try_catch(isolate()); + CHECK(module + ->InstantiateModule(context(), + ResolveModuleByIndexFailureCallback, + ResolveSourceByIndexUnreachableCallback) + .IsNothing()); + CHECK(inner_try_catch.HasCaught()); + CHECK(inner_try_catch.Exception()->StrictEquals( + NewString("module 0 not found"))); + CHECK_EQ(Module::kUninstantiated, module->GetStatus()); + } + + // Should not leak to the outer try-catch. + CHECK(!try_catch.HasCaught()); +} + +static bool resolve_source_by_index_failure_called = false; +MaybeLocal ResolveSourceByIndexFailureCallback( + Local context, size_t module_request_index, + Local referrer) { + CHECK(!resolve_source_by_index_failure_called); + resolve_source_by_index_failure_called = true; + v8::Isolate* isolate = v8::Isolate::GetCurrent(); + std::string message = + "module source " + std::to_string(module_request_index) + " not found"; + isolate->ThrowException( + String::NewFromUtf8(isolate, message.c_str()).ToLocalChecked()); + return MaybeLocal(); +} + +static MaybeLocal ResolveModuleByIndexUnreachableCallback( + Local context, size_t module_request_index, + Local referrer) { + UNREACHABLE(); +} + +TEST_F(ModuleTest, ModuleInstantiationByIndexWithSourceFaliure) { + bool prev_import_attributes = i::v8_flags.js_source_phase_imports; + i::v8_flags.js_source_phase_imports = true; + HandleScope scope(isolate()); + v8::TryCatch try_catch(isolate()); + + Local module; + { + Local source_text = NewString( + "import source mod from './foo.wasm;'\n" + "export { mod };\n"); + ScriptOrigin origin = ModuleOrigin(NewString("main.js"), isolate()); + ScriptCompiler::Source source(source_text, origin); + module = ScriptCompiler::CompileModule(isolate(), &source).ToLocalChecked(); + CHECK_EQ(Module::kUninstantiated, module->GetStatus()); + + Local module_requests = module->GetModuleRequests(); + CHECK_EQ(1, module_requests->Length()); + } + + // Instantiation should fail and the callback should only be called once. + { + v8::TryCatch inner_try_catch(isolate()); + CHECK(module + ->InstantiateModule(context(), + ResolveModuleByIndexUnreachableCallback, + ResolveSourceByIndexFailureCallback) + .IsNothing()); + CHECK(inner_try_catch.HasCaught()); + CHECK(inner_try_catch.Exception()->StrictEquals( + NewString("module source 0 not found"))); + CHECK_EQ(Module::kUninstantiated, module->GetStatus()); + } + + // Should not leak to the outer try-catch. + CHECK(!try_catch.HasCaught()); + i::v8_flags.js_source_phase_imports = prev_import_attributes; +} + +#if V8_ENABLE_WEBASSEMBLY + +// The bytes of a minimal WebAssembly module. +static const uint8_t kMinimalWasmModuleBytes[]{0x00, 0x61, 0x73, 0x6d, + 0x01, 0x00, 0x00, 0x00}; + +static bool resolve_source_by_index_called = false; +static v8::Global wasm_module_global; +MaybeLocal ResolveSourceByIndexCallback(Local context, + size_t module_request_index, + Local referrer) { + CHECK(!resolve_source_by_index_called); + resolve_source_by_index_called = true; + v8::Isolate* isolate = v8::Isolate::GetCurrent(); + return wasm_module_global.Get(isolate); +} + +TEST_F(ModuleTest, ModuleInstantiationByIndexWithSource) { + bool prev_import_attributes = i::v8_flags.js_source_phase_imports; + i::v8_flags.js_source_phase_imports = true; + HandleScope scope(isolate()); + v8::TryCatch try_catch(isolate()); + + Local module; + { + Local source_text = NewString( + "import source mod from './foo.wasm';\n" + "export { mod };\n"); + ScriptOrigin origin = ModuleOrigin(NewString("main.js"), isolate()); + ScriptCompiler::Source source(source_text, origin); + module = ScriptCompiler::CompileModule(isolate(), &source).ToLocalChecked(); + CHECK_EQ(Module::kUninstantiated, module->GetStatus()); + + Local module_requests = module->GetModuleRequests(); + CHECK_EQ(1, module_requests->Length()); + + Local module_request_0 = + module_requests->Get(context(), 0).As(); + CHECK(NewString("./foo.wasm") + ->StrictEquals(module_request_0->GetSpecifier())); + CHECK_EQ(v8::ModuleImportPhase::kSource, module_request_0->GetPhase()); + } + + { + Local wasm_module = + v8::WasmModuleObject::Compile( + isolate(), + {kMinimalWasmModuleBytes, arraysize(kMinimalWasmModuleBytes)}) + .ToLocalChecked(); + wasm_module_global.Reset(isolate(), wasm_module); + } + + // Instantiate using index-based callback + CHECK(module + ->InstantiateModule(context(), + ResolveModuleByIndexUnreachableCallback, + ResolveSourceByIndexCallback) + .FromJust()); + CHECK_EQ(Module::kInstantiated, module->GetStatus()); + + // Verify evaluation works + MaybeLocal result = module->Evaluate(context()); + CHECK_EQ(Module::kEvaluated, module->GetStatus()); + Local promise = Local::Cast(result.ToLocalChecked()); + CHECK_EQ(promise->State(), v8::Promise::kFulfilled); + + CHECK(!try_catch.HasCaught()); + Local mod = module->GetModuleNamespace() + .As() + ->Get(context(), NewString("mod")) + .ToLocalChecked(); + CHECK(mod->StrictEquals(wasm_module_global.Get(isolate()))); + + i::v8_flags.js_source_phase_imports = prev_import_attributes; + wasm_module_global.Reset(); +} + +#endif // V8_ENABLE_WEBASSEMBLY + } // anonymous namespace