src: clear all linked module caches once instantiated

There are two phases in module linking: link, and instantiate. These
two operations are required to be separated to allow cyclic
dependencies.

`v8::Module::InstantiateModule` is only required to be invoked on the
root module. The global references created by `ModuleWrap::Link` are
only cleared at `ModuleWrap::Instantiate`. So the global references
created for depended modules are usually not cleared because
`ModuleWrap::Instantiate` is not invoked for each of depended modules,
and caused memory leak.

The change references the linked modules in an object internal slot.

This is not an issue for Node.js ESM support as these modules can not be
off-loaded. However, this could be outstanding for `vm.Module`.

PR-URL: #59117
Fixes: #50113
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

Author: legendecas

src/module_wrap.cc

@@ -133,12 +133,23 @@ ModuleWrap::ModuleWrap(Realm* realm,
   object->SetInternalField(kSyntheticEvaluationStepsSlot,
                            synthetic_evaluation_step);
   object->SetInternalField(kContextObjectSlot, context_object);
+  object->SetInternalField(kLinkedRequestsSlot,
+                           v8::Undefined(realm->isolate()));
 
   if (!synthetic_evaluation_step->IsUndefined()) {
     synthetic_ = true;
   }
   MakeWeak();
   module_.SetWeak();
+
+  HandleScope scope(realm->isolate());
+  Local<Context> context = realm->context();
+  Local<FixedArray> requests = module->GetModuleRequests();
+  for (int i = 0; i < requests->Length(); i++) {
+    ModuleCacheKey module_cache_key = ModuleCacheKey::From(
+        context, requests->Get(context, i).As<ModuleRequest>());
+    resolve_cache_[module_cache_key] = i;
+  }
 }
 
 ModuleWrap::~ModuleWrap() {
@@ -159,6 +170,30 @@ Local<Context> ModuleWrap::context() const {
   return obj.As<Object>()->GetCreationContextChecked();
 }
 
+ModuleWrap* ModuleWrap::GetLinkedRequest(uint32_t index) {
+  DCHECK(IsLinked());
+  Isolate* isolate = env()->isolate();
+  EscapableHandleScope scope(isolate);
+  Local<Data> linked_requests_data =
+      object()->GetInternalField(kLinkedRequestsSlot);
+  DCHECK(linked_requests_data->IsValue() &&
+         linked_requests_data.As<Value>()->IsArray());
+  Local<Array> requests = linked_requests_data.As<Array>();
+
+  CHECK_LT(index, requests->Length());
+
+  Local<Value> module_value;
+  if (!requests->Get(context(), index).ToLocal(&module_value)) {
+    return nullptr;
+  }
+  CHECK(module_value->IsObject());
+  Local<Object> module_object = module_value.As<Object>();
+
+  ModuleWrap* module_wrap;
+  ASSIGN_OR_RETURN_UNWRAP(&module_wrap, module_object, nullptr);
+  return module_wrap;
+}
+
 ModuleWrap* ModuleWrap::GetFromModule(Environment* env,
                                       Local<Module> module) {
   auto range = env->hash_to_module_map.equal_range(module->GetIdentityHash());
@@ -571,34 +606,28 @@ void ModuleWrap::GetModuleRequests(const FunctionCallbackInfo<Value>& args) {
 void ModuleWrap::Link(const FunctionCallbackInfo<Value>& args) {
   Realm* realm = Realm::GetCurrent(args);
   Isolate* isolate = args.GetIsolate();
-  Local<Context> context = realm->context();
 
   ModuleWrap* dependent;
   ASSIGN_OR_RETURN_UNWRAP(&dependent, args.This());
 
   CHECK_EQ(args.Length(), 1);
 
+  Local<Data> linked_requests =
+      args.This()->GetInternalField(kLinkedRequestsSlot);
+  if (linked_requests->IsValue() &&
+      !linked_requests.As<Value>()->IsUndefined()) {
+    // If the module is already linked, we should not link it again.
+    THROW_ERR_VM_MODULE_LINK_FAILURE(realm->env(), "module is already linked");
+    return;
+  }
+
   Local<FixedArray> requests =
       dependent->module_.Get(isolate)->GetModuleRequests();
   Local<Array> modules = args[0].As<Array>();
   CHECK_EQ(modules->Length(), static_cast<uint32_t>(requests->Length()));
 
-  std::vector<Global<Value>> modules_buffer;
-  if (FromV8Array(context, modules, &modules_buffer).IsNothing()) {
-    return;
-  }
-
-  for (uint32_t i = 0; i < modules_buffer.size(); i++) {
-    Local<Object> module_object = modules_buffer[i].Get(isolate).As<Object>();
-
-    CHECK(
-        realm->isolate_data()->module_wrap_constructor_template()->HasInstance(
-            module_object));
-
-    ModuleCacheKey module_cache_key = ModuleCacheKey::From(
-        context, requests->Get(context, i).As<ModuleRequest>());
-    dependent->resolve_cache_[module_cache_key].Reset(isolate, module_object);
-  }
+  args.This()->SetInternalField(kLinkedRequestsSlot, modules);
+  dependent->linked_ = true;
 }
 
 void ModuleWrap::Instantiate(const FunctionCallbackInfo<Value>& args) {
@@ -612,9 +641,6 @@ void ModuleWrap::Instantiate(const FunctionCallbackInfo<Value>& args) {
   USE(module->InstantiateModule(
       context, ResolveModuleCallback, ResolveSourceCallback));
 
-  // clear resolve cache on instantiate
-  obj->resolve_cache_.clear();
-
   if (try_catch.HasCaught() && !try_catch.HasTerminated()) {
     CHECK(!try_catch.Message().IsEmpty());
     CHECK(!try_catch.Exception().IsEmpty());
@@ -722,9 +748,6 @@ void ModuleWrap::InstantiateSync(const FunctionCallbackInfo<Value>& args) {
     USE(module->InstantiateModule(
         context, ResolveModuleCallback, ResolveSourceCallback));
 
-    // clear resolve cache on instantiate
-    obj->resolve_cache_.clear();
-
     if (try_catch.HasCaught() && !try_catch.HasTerminated()) {
       CHECK(!try_catch.Message().IsEmpty());
       CHECK(!try_catch.Exception().IsEmpty());
@@ -965,48 +988,51 @@ void ModuleWrap::GetError(const FunctionCallbackInfo<Value>& args) {
   args.GetReturnValue().Set(module->GetException());
 }
 
+// static
 MaybeLocal<Module> ModuleWrap::ResolveModuleCallback(
     Local<Context> context,
     Local<String> specifier,
     Local<FixedArray> import_attributes,
     Local<Module> referrer) {
-  Isolate* isolate = context->GetIsolate();
-  Environment* env = Environment::GetCurrent(context);
-  if (env == nullptr) {
-    THROW_ERR_EXECUTION_ENVIRONMENT_NOT_AVAILABLE(isolate);
-    return MaybeLocal<Module>();
-  }
-
-  ModuleCacheKey cache_key =
-      ModuleCacheKey::From(context, specifier, import_attributes);
-
-  ModuleWrap* dependent = GetFromModule(env, referrer);
-  if (dependent == nullptr) {
-    THROW_ERR_VM_MODULE_LINK_FAILURE(
-        env, "request for '%s' is from invalid module", cache_key.specifier);
-    return MaybeLocal<Module>();
+  ModuleWrap* resolved_module;
+  if (!ResolveModule(context, specifier, import_attributes, referrer)
+           .To(&resolved_module)) {
+    return {};
   }
+  DCHECK_NOT_NULL(resolved_module);
+  return resolved_module->module_.Get(context->GetIsolate());
+}
 
-  if (dependent->resolve_cache_.count(cache_key) != 1) {
-    THROW_ERR_VM_MODULE_LINK_FAILURE(
-        env, "request for '%s' is not in cache", cache_key.specifier);
-    return MaybeLocal<Module>();
+// static
+MaybeLocal<Object> ModuleWrap::ResolveSourceCallback(
+    Local<Context> context,
+    Local<String> specifier,
+    Local<FixedArray> import_attributes,
+    Local<Module> referrer) {
+  ModuleWrap* resolved_module;
+  if (!ResolveModule(context, specifier, import_attributes, referrer)
+           .To(&resolved_module)) {
+    return {};
   }
+  DCHECK_NOT_NULL(resolved_module);
 
-  Local<Object> module_object =
-      dependent->resolve_cache_[cache_key].Get(isolate);
-  if (module_object.IsEmpty() || !module_object->IsObject()) {
-    THROW_ERR_VM_MODULE_LINK_FAILURE(
-        env, "request for '%s' did not return an object", cache_key.specifier);
-    return MaybeLocal<Module>();
+  Local<Value> module_source_object =
+      resolved_module->object()
+          ->GetInternalField(ModuleWrap::kModuleSourceObjectSlot)
+          .As<Value>();
+  if (module_source_object->IsUndefined()) {
+    Local<String> url = resolved_module->object()
+                            ->GetInternalField(ModuleWrap::kURLSlot)
+                            .As<String>();
+    THROW_ERR_SOURCE_PHASE_NOT_DEFINED(context->GetIsolate(), url);
+    return {};
   }
-
-  ModuleWrap* module;
-  ASSIGN_OR_RETURN_UNWRAP(&module, module_object, MaybeLocal<Module>());
-  return module->module_.Get(isolate);
+  CHECK(module_source_object->IsObject());
+  return module_source_object.As<Object>();
 }
 
-MaybeLocal<Object> ModuleWrap::ResolveSourceCallback(
+// static
+Maybe<ModuleWrap*> ModuleWrap::ResolveModule(
     Local<Context> context,
     Local<String> specifier,
     Local<FixedArray> import_attributes,
@@ -1015,46 +1041,38 @@ MaybeLocal<Object> ModuleWrap::ResolveSourceCallback(
   Environment* env = Environment::GetCurrent(context);
   if (env == nullptr) {
     THROW_ERR_EXECUTION_ENVIRONMENT_NOT_AVAILABLE(isolate);
-    return MaybeLocal<Object>();
+    return Nothing<ModuleWrap*>();
   }
+  // Check that the referrer is not yet been instantiated.
+  DCHECK(referrer->GetStatus() <= Module::kInstantiated);
 
   ModuleCacheKey cache_key =
       ModuleCacheKey::From(context, specifier, import_attributes);
 
-  ModuleWrap* dependent = GetFromModule(env, referrer);
+  ModuleWrap* dependent = ModuleWrap::GetFromModule(env, referrer);
   if (dependent == nullptr) {
     THROW_ERR_VM_MODULE_LINK_FAILURE(
         env, "request for '%s' is from invalid module", cache_key.specifier);
-    return MaybeLocal<Object>();
+    return Nothing<ModuleWrap*>();
   }
-
-  if (dependent->resolve_cache_.count(cache_key) != 1) {
+  if (!dependent->IsLinked()) {
     THROW_ERR_VM_MODULE_LINK_FAILURE(
-        env, "request for '%s' is not in cache", cache_key.specifier);
-    return MaybeLocal<Object>();
+        env,
+        "request for '%s' is from a module not been linked",
+        cache_key.specifier);
+    return Nothing<ModuleWrap*>();
   }
 
-  Local<Object> module_object =
-      dependent->resolve_cache_[cache_key].Get(isolate);
-  if (module_object.IsEmpty() || !module_object->IsObject()) {
+  auto it = dependent->resolve_cache_.find(cache_key);
+  if (it == dependent->resolve_cache_.end()) {
     THROW_ERR_VM_MODULE_LINK_FAILURE(
-        env, "request for '%s' did not return an object", cache_key.specifier);
-    return MaybeLocal<Object>();
+        env, "request for '%s' is not in cache", cache_key.specifier);
+    return Nothing<ModuleWrap*>();
   }
 
-  ModuleWrap* module;
-  ASSIGN_OR_RETURN_UNWRAP(&module, module_object, MaybeLocal<Object>());
-
-  Local<Value> module_source_object =
-      module->object()->GetInternalField(kModuleSourceObjectSlot).As<Value>();
-  if (module_source_object->IsUndefined()) {
-    Local<String> url =
-        module->object()->GetInternalField(kURLSlot).As<String>();
-    THROW_ERR_SOURCE_PHASE_NOT_DEFINED(isolate, url);
-    return MaybeLocal<Object>();
-  }
-  CHECK(module_source_object->IsObject());
-  return module_source_object.As<Object>();
+  ModuleWrap* module_wrap = dependent->GetLinkedRequest(it->second);
+  CHECK_NOT_NULL(module_wrap);
+  return Just(module_wrap);
 }
 
 static MaybeLocal<Promise> ImportModuleDynamicallyWithPhase(

src/module_wrap.h

@@ -90,13 +90,17 @@ struct ModuleCacheKey : public MemoryRetainer {
 };
 
 class ModuleWrap : public BaseObject {
+  using ResolveCache =
+      std::unordered_map<ModuleCacheKey, uint32_t, ModuleCacheKey::Hash>;
+
  public:
   enum InternalFields {
     kModuleSlot = BaseObject::kInternalFieldCount,
     kURLSlot,
     kModuleSourceObjectSlot,
     kSyntheticEvaluationStepsSlot,
-    kContextObjectSlot,  // Object whose creation context is the target Context
+    kContextObjectSlot,   // Object whose creation context is the target Context
+    kLinkedRequestsSlot,  // Array of linked requests
     kInternalFieldCount
   };
 
@@ -112,23 +116,25 @@ class ModuleWrap : public BaseObject {
       v8::Local<v8::Module> module,
       v8::Local<v8::Object> meta);
 
-  void MemoryInfo(MemoryTracker* tracker) const override {
-    tracker->TrackField("resolve_cache", resolve_cache_);
-  }
   static void HasTopLevelAwait(const v8::FunctionCallbackInfo<v8::Value>& args);
 
   v8::Local<v8::Context> context() const;
   v8::Maybe<bool> CheckUnsettledTopLevelAwait();
 
   SET_MEMORY_INFO_NAME(ModuleWrap)
   SET_SELF_SIZE(ModuleWrap)
+  SET_NO_MEMORY_INFO()
 
   bool IsNotIndicativeOfMemoryLeakAtExit() const override {
     // XXX: The garbage collection rules for ModuleWrap are *super* unclear.
     // Do these objects ever get GC'd? Are we just okay with leaking them?
     return true;
   }
 
+  bool IsLinked() const { return linked_; }
+
+  ModuleWrap* GetLinkedRequest(uint32_t index);
+
   static v8::Local<v8::PrimitiveArray> GetHostDefinedOptions(
       v8::Isolate* isolate, v8::Local<v8::Symbol> symbol);
 
@@ -199,13 +205,19 @@ class ModuleWrap : public BaseObject {
       v8::Local<v8::Module> referrer);
   static ModuleWrap* GetFromModule(node::Environment*, v8::Local<v8::Module>);
 
+  // This method may throw a JavaScript exception, so the return type is
+  // wrapped in a Maybe.
+  static v8::Maybe<ModuleWrap*> ResolveModule(
+      v8::Local<v8::Context> context,
+      v8::Local<v8::String> specifier,
+      v8::Local<v8::FixedArray> import_attributes,
+      v8::Local<v8::Module> referrer);
+
   v8::Global<v8::Module> module_;
-  std::unordered_map<ModuleCacheKey,
-                     v8::Global<v8::Object>,
-                     ModuleCacheKey::Hash>
-      resolve_cache_;
+  ResolveCache resolve_cache_;
   contextify::ContextifyContext* contextify_context_ = nullptr;
   bool synthetic_ = false;
+  bool linked_ = false;
   int module_hash_;
 };
 

test/parallel/test-internal-module-wrap.js

@@ -6,6 +6,21 @@ const assert = require('assert');
 const { internalBinding } = require('internal/test/binding');
 const { ModuleWrap } = internalBinding('module_wrap');
 
+const unlinked = new ModuleWrap('unlinked', undefined, 'export * from "bar";', 0, 0);
+assert.throws(() => {
+  unlinked.instantiate();
+}, {
+  code: 'ERR_VM_MODULE_LINK_FAILURE',
+});
+
+const dependsOnUnlinked = new ModuleWrap('dependsOnUnlinked', undefined, 'export * from "unlinked";', 0, 0);
+dependsOnUnlinked.link([unlinked]);
+assert.throws(() => {
+  dependsOnUnlinked.instantiate();
+}, {
+  code: 'ERR_VM_MODULE_LINK_FAILURE',
+});
+
 const foo = new ModuleWrap('foo', undefined, 'export * from "bar";', 0, 0);
 const bar = new ModuleWrap('bar', undefined, 'export const five = 5', 0, 0);
 
@@ -22,4 +37,5 @@ const bar = new ModuleWrap('bar', undefined, 'export const five = 5', 0, 0);
 
   // Check that the module requests are the same after linking, instantiate, and evaluation.
   assert.deepStrictEqual(moduleRequests, foo.getModuleRequests());
+
 })().then(common.mustCall());