From 2920904784aed61d00e11ceba57c615f6d234c1f Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 23 Jun 2023 18:32:57 +0200 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/48510 Refs: https://github.com/nodejs/node/issues/44211 Refs: https://github.com/nodejs/node/issues/42080 Refs: https://github.com/nodejs/node/issues/47096 Refs: https://github.com/nodejs/node/issues/43205 Refs: https://github.com/nodejs/node/issues/38695 Reviewed-By: Antoine du Hamel Reviewed-By: Benjamin Gruenbaum Reviewed-By: Stephen Belanger --- src/module_wrap.cc | 9 +++++--- src/module_wrap.h | 3 ++- src/node_contextify.cc | 4 ++++ src/node_contextify.h | 5 ++++ .../test-vm-contextified-script-leak.js | 19 +++++++++++++++ .../test-vm-source-text-module-leak.js | 23 +++++++++++++++++++ 6 files changed, 59 insertions(+), 4 deletions(-) create mode 100644 test/es-module/test-vm-contextified-script-leak.js create mode 100644 test/es-module/test-vm-source-text-module-leak.js diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 56699764ef38a7..a1b0f812391486 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -55,7 +55,10 @@ ModuleWrap::ModuleWrap(Environment* env, Local url, Local context_object, Local 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_.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 index b96d629f3e5ae4..6435bad40936fe 100644 --- 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 index 5ec80a7b6666cb..75208c7293863e 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -855,7 +855,11 @@ void ContextifyScript::New(const FunctionCallbackInfo& 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 new_cached_data; if (produce_cached_data) { diff --git a/src/node_contextify.h b/src/node_contextify.h index 1098a7745257ac..d1dddbf374d563 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -128,6 +128,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 new file mode 100644 index 00000000000000..7498b46ab80cfa --- /dev/null +++ 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 new file mode 100644 index 00000000000000..bf7f70c670e34c --- /dev/null +++ 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();