module: fix the leak in SourceTextModule and ContextifySript

Replace the persistent handles to v8::Module and v8::UnboundScript with an internal reference that V8's GC is aware of to fix the leaks. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
joyeecheung · Dec 1, 2023 · 9a4a6b2 · 9a4a6b2
1 parent 1542010
commit 9a4a6b2
Show file tree

Hide file tree

Showing 6 changed files with 59 additions and 4 deletions.
diff --git a/src/module_wrap.cc b/src/module_wrap.cc
@@ -55,7 +55,10 @@ ModuleWrap::ModuleWrap(Environment* env,
                        Local<String> url,
                        Local<Object> context_object,
                        Local<Value> synthetic_evaluation_step)
-    : BaseObject(env, object), module_(env->isolate(), module) {
+    : BaseObject(env, object),
+      module_(env->isolate(), module),
+      module_hash_(module->GetIdentityHash()) {
+  object->SetInternalFieldForNodeCore(kModuleSlot, module);
   object->SetInternalField(kURLSlot, url);
   object->SetInternalField(kSyntheticEvaluationStepsSlot,
                            synthetic_evaluation_step);
@@ -65,12 +68,12 @@ ModuleWrap::ModuleWrap(Environment* env,
     synthetic_ = true;
   }
   MakeWeak();
+  module_.SetWeak();
 }
 
 ModuleWrap::~ModuleWrap() {
   HandleScope scope(env()->isolate());
-  Local<Module> module = module_.Get(env()->isolate());
-  auto range = env()->hash_to_module_map.equal_range(module->GetIdentityHash());
+  auto range = env()->hash_to_module_map.equal_range(module_hash_);
   for (auto it = range.first; it != range.second; ++it) {
     if (it->second == this) {
       env()->hash_to_module_map.erase(it);

diff --git a/src/module_wrap.h b/src/module_wrap.h
@@ -33,7 +33,7 @@ enum HostDefinedOptions : int {
 class ModuleWrap : public BaseObject {
  public:
   enum InternalFields {
-    kModuleWrapBaseField = BaseObject::kInternalFieldCount,
+    kModuleSlot = BaseObject::kInternalFieldCount,
     kURLSlot,
     kSyntheticEvaluationStepsSlot,
     kContextObjectSlot,  // Object whose creation context is the target Context
@@ -106,6 +106,7 @@ class ModuleWrap : public BaseObject {
   contextify::ContextifyContext* contextify_context_ = nullptr;
   bool synthetic_ = false;
   bool linked_ = false;
+  int module_hash_;
 };
 
 }  // namespace loader

diff --git a/src/node_contextify.cc b/src/node_contextify.cc
@@ -871,7 +871,11 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
                      "ContextifyScript::New");
     return;
   }
+
   contextify_script->script_.Reset(isolate, v8_script);
+  contextify_script->script_.SetWeak();
+  contextify_script->object()->SetInternalFieldForNodeCore(kUnboundScriptSlot,
+                                                           v8_script);
 
   std::unique_ptr<ScriptCompiler::CachedData> new_cached_data;
   if (produce_cached_data) {

diff --git a/src/node_contextify.h b/src/node_contextify.h
@@ -149,6 +149,11 @@ class ContextifyContext : public BaseObject {
 
 class ContextifyScript : public BaseObject {
  public:
+  enum InternalFields {
+    kUnboundScriptSlot = BaseObject::kInternalFieldCount,
+    kInternalFieldCount
+  };
+
   SET_NO_MEMORY_INFO()
   SET_MEMORY_INFO_NAME(ContextifyScript)
   SET_SELF_SIZE(ContextifyScript)

diff --git a/test/es-module/test-vm-contextified-script-leak.js b/test/es-module/test-vm-contextified-script-leak.js
@@ -0,0 +1,19 @@
+// Flags: --max-old-space-size=16 --trace-gc
+'use strict';
+
+// This tests that vm.Script with dynamic import callback does not leak.
+// See: https://github.com/nodejs/node/issues/33439
+require('../common');
+const vm = require('vm');
+let count = 0;
+
+function main() {
+  // Try to reach the maximum old space size.
+  new vm.Script(`"${Math.random().toString().repeat(512)}";`, {
+    async importModuleDynamically() {},
+  });
+  if (count++ < 2 * 1024) {
+    setTimeout(main, 1);
+  }
+}
+main();
diff --git a/test/es-module/test-vm-source-text-module-leak.js b/test/es-module/test-vm-source-text-module-leak.js
@@ -0,0 +1,23 @@
+// Flags: --experimental-vm-modules --max-old-space-size=16 --trace-gc
+'use strict';
+
+// This tests that vm.SourceTextModule() does not leak.
+// See: https://github.com/nodejs/node/issues/33439
+require('../common');
+
+const vm = require('vm');
+let count = 0;
+async function createModule() {
+  // Try to reach the maximum old space size.
+  const m = new vm.SourceTextModule(`
+  const bar = new Array(512).fill("----");
+  export { bar };
+`);
+  await m.link(() => {});
+  await m.evaluate();
+  if (count++ < 4096) {
+    setTimeout(createModule, 1);
+  }
+  return m;
+}
+createModule();