Skip to content

Commit

Permalink
module: fix the leak in SourceTextModule and ContextifySript
Browse files Browse the repository at this point in the history
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: #48510
Backport-PR-URL: #51004
Refs: #44211
Refs: #42080
Refs: #47096
Refs: #43205
Refs: #38695
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
  • Loading branch information
joyeecheung authored and richardlau committed Mar 15, 2024
1 parent 2e05cf1 commit 7552de6
Show file tree
Hide file tree
Showing 6 changed files with 59 additions and 4 deletions.
9 changes: 6 additions & 3 deletions src/module_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion src/module_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -106,6 +106,7 @@ class ModuleWrap : public BaseObject {
contextify::ContextifyContext* contextify_context_ = nullptr;
bool synthetic_ = false;
bool linked_ = false;
int module_hash_;
};

} // namespace loader
Expand Down
4 changes: 4 additions & 0 deletions src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
5 changes: 5 additions & 0 deletions src/node_contextify.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
19 changes: 19 additions & 0 deletions test/es-module/test-vm-contextified-script-leak.js
Original file line number Diff line number Diff line change
@@ -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();
23 changes: 23 additions & 0 deletions test/es-module/test-vm-source-text-module-leak.js
Original file line number Diff line number Diff line change
@@ -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();

0 comments on commit 7552de6

Please sign in to comment.