Skip to content

Commit 986498b

Browse files
joyeecheunglegendecas
authored andcommitted
vm: fix leak in vm.compileFunction when importModuleDynamically is used
Previously in the implementation there was a cycle that V8 could not detect: Strong global reference to CompiledFnEntry (JS wrapper) -> strong reference to callback setting (through the callbackMap key-value pair) -> importModuleDynamically (wrapper in internalCompileFunction()) -> Strong reference to the compiled function (through closure in internalCompileFunction()) The CompiledFnEntry only gets GC'ed when the compiled function is GC'ed. Since the compiled function is always reachable as described above, there is a leak. We only needed the first strong global reference because we didn't want the function to outlive the CompiledFnEntry. In this case it can be solved by using a private symbol instead of going with the global reference + destruction in the weak callback, which V8's GC is not going to understand. PR-URL: #46785 Fixes: #42080 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
1 parent 0597f1b commit 986498b

File tree

4 files changed

+20
-15
lines changed

4 files changed

+20
-15
lines changed

src/env_properties.h

+1
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
#define PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V) \
2121
V(arrow_message_private_symbol, "node:arrowMessage") \
2222
V(contextify_context_private_symbol, "node:contextify:context") \
23+
V(compiled_function_entry, "node:compiled_function_entry") \
2324
V(decorated_private_symbol, "node:decorated") \
2425
V(napi_type_tag, "node:napi:type_tag") \
2526
V(napi_wrapper, "node:napi:wrapper") \

src/node_contextify.cc

+5-12
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,6 @@ using v8::Uint32;
7777
using v8::UnboundScript;
7878
using v8::Value;
7979
using v8::WeakCallbackInfo;
80-
using v8::WeakCallbackType;
8180

8281
// The vm module executes code in a sandboxed environment with a different
8382
// global object than the rest of the code. This is achieved by applying
@@ -1263,8 +1262,7 @@ void ContextifyContext::CompileFunction(
12631262
context).ToLocal(&cache_key)) {
12641263
return;
12651264
}
1266-
CompiledFnEntry* entry = new CompiledFnEntry(env, cache_key, id, fn);
1267-
env->id_to_function_map.emplace(id, entry);
1265+
new CompiledFnEntry(env, cache_key, id, fn);
12681266

12691267
Local<Object> result = Object::New(isolate);
12701268
if (result->Set(parsing_context, env->function_string(), fn).IsNothing())
@@ -1296,23 +1294,18 @@ void ContextifyContext::CompileFunction(
12961294
args.GetReturnValue().Set(result);
12971295
}
12981296

1299-
void CompiledFnEntry::WeakCallback(
1300-
const WeakCallbackInfo<CompiledFnEntry>& data) {
1301-
CompiledFnEntry* entry = data.GetParameter();
1302-
delete entry;
1303-
}
1304-
13051297
CompiledFnEntry::CompiledFnEntry(Environment* env,
13061298
Local<Object> object,
13071299
uint32_t id,
13081300
Local<Function> fn)
1309-
: BaseObject(env, object), id_(id), fn_(env->isolate(), fn) {
1310-
fn_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter);
1301+
: BaseObject(env, object), id_(id) {
1302+
MakeWeak();
1303+
fn->SetPrivate(env->context(), env->compiled_function_entry(), object);
1304+
env->id_to_function_map.emplace(id, this);
13111305
}
13121306

13131307
CompiledFnEntry::~CompiledFnEntry() {
13141308
env()->id_to_function_map.erase(id_);
1315-
fn_.ClearWeak();
13161309
}
13171310

13181311
static void StartSigintWatchdog(const FunctionCallbackInfo<Value>& args) {

src/node_contextify.h

-3
Original file line numberDiff line numberDiff line change
@@ -194,9 +194,6 @@ class CompiledFnEntry final : public BaseObject {
194194

195195
private:
196196
uint32_t id_;
197-
v8::Global<v8::Function> fn_;
198-
199-
static void WeakCallback(const v8::WeakCallbackInfo<CompiledFnEntry>& data);
200197
};
201198

202199
v8::Maybe<bool> StoreCodeCacheResult(
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
'use strict';
2+
3+
// Flags: --max-old-space-size=10
4+
5+
require('../common');
6+
const vm = require('vm');
7+
8+
const code = `console.log("${'hello world '.repeat(1e5)}");`;
9+
10+
for (let i = 0; i < 10000; i++) {
11+
vm.compileFunction(code, [], {
12+
importModuleDynamically: () => {},
13+
});
14+
}

0 commit comments

Comments
 (0)