8000 module: fix the leak in SourceTextModule and ContextifySript · nodejs/node@b0ce78a · GitHub
[go: up one dir, main page]
Skip to content

Commit b0ce78a

Browse files
joyeecheungruyadorno
joyeecheung
authored and
ruyadorno
committed
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. Backport-PR-URL: #49874 PR-URL: #48510 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>
1 parent 4e578f8 commit b0ce78a

File tree

6 files changed

+59
-4
lines changed

6 files changed

+59
-4
lines changed

‎src/module_wrap.cc‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,10 @@ ModuleWrap::ModuleWrap(Environment* env,
5555
Local<String> url,
5656
Local<Object> context_object,
5757
Local<Value> synthetic_evaluation_step)
58-
: BaseObject(env, object), module_(env->isolate(), module) {
58+
: BaseObject(env, object),
59+
module_(env->isolate(), module),
60+
module_hash_(module->GetIdentityHash()) {
61+
object->SetInternalFieldForNodeCore(kModuleSlot, module);
5962
object->SetInternalField(kURLSlot, url);
6063
object->SetInternalField(kSyntheticEvaluationStepsSlot,
6164
synthetic_evaluation_step);
@@ -65,12 +68,12 @@ ModuleWrap::ModuleWrap(Environment* env,
6568
synthetic_ = true;
6669
}
6770
MakeWeak();
71+
module_.SetWeak();
6872
}
6973

7074
ModuleWrap::~ModuleWrap() {
7175
HandleScope scope(env()->isolate());
72-
Local<Module> module = module_.Get(env()->isolate());
73-
auto range = env()->hash_to_module_map.equal_range(module->GetIdentityHash());
76+
auto range = env()->hash_to_module_map.equal_range(module_hash_);
7477
for (auto it = range.first; it != range.second; ++it) {
7578
if (it->second == this) {
7679
env()->hash_to_module_map.erase(it);

‎src/module_wrap.h‎

Lines changed: 2 additions & 1 deletion
10000
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ enum HostDefinedOptions : int {
3333
class ModuleWrap : public BaseObject {
3434
public:
3535
enum InternalFields {
36-
kModuleWrapBaseField = BaseObject::kInternalFieldCount,
36+
kModuleSlot = BaseObject::kInternalFieldCount,
3737
kURLSlot,
3838
kSyntheticEvaluationStepsSlot,
3939
kContextObjectSlot, // Object whose creation context is the target Context
@@ -106,6 +106,7 @@ class ModuleWrap : public BaseObject {
106106
contextify::ContextifyContext* contextify_context_ = nullptr;
107107
bool synthetic_ = false;
108108
bool linked_ = false;
109+
int module_hash_;
109110
};
110111

111112
} // namespace loader

‎src/node_contextify.cc‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -855,7 +855,11 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
855855
"ContextifyScript::New");
856856
return;
857857
}
858+
858859
contextify_script->script_.Reset(isolate, v8_script);
860+
contextify_script->script_.SetWeak();
861+
contextify_script->object()->SetInternalFieldForNodeCore(kUnboundScriptSlot,
862+
v8_script);
859863

860864
std::unique_ptr<ScriptCompiler::CachedData> new_cached_data;
861865
if (produce_cached_data) {

‎src/node_contextify.h‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,11 @@ class ContextifyContext : public BaseObject {
128128

129129
class ContextifyScript : public BaseObject {
130130
public:
131+
enum InternalFields {
132+
kUnboundScriptSlot = BaseObject::kInternalFieldCount,
133+
kInternalFieldCount
134+
};
135+
131136
SET_NO_MEMORY_INFO()
132137
SET_MEMORY_INFO_NAME(ContextifyScript)
133138
SET_SELF_SIZE(ContextifyScript)

‎test/es-module/test-vm-contextified-script-leak.js‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
// Flags: --max-old-space-size=16 --trace-gc
2+
'use strict';
3+
4+
// This tests that vm.Script with dynamic import callback does not leak.
5+
// See: https://github.com/nodejs/node/issues/33439
6+
require('../common');
7+
const vm = require('vm');
8+
let count = 0;
9+
10+
function main() {
11+
// Try to reach the maximum old space size.
12+
new vm.Script(`"${Math.random().toString().repeat(512)}";`, {
13+
async importModuleDynamically() {},
14+
});
15+
if (count++ < 2 * 1024) {
16+
setTimeout(main, 1);
17+
}
18+
}
19+
main();

‎test/es-module/test-vm-source-text-module-leak.js‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
// Flags: --experimental-vm-modules --max-old-space-size=16 --trace-gc
2+
'use strict';
3+
4+
// This tests that vm.SourceTextModule() does not leak.
5+
// See: https://github.com/nodejs/node/issues/33439
6+
require('../common');
7+
8+
const vm = require('vm');
9+
let count = 0;
10+
async function createModule() {
11+
// Try to reach the maximum old space size.
12+
const m = new vm.SourceTextModule(`
13+
const bar = new Array(512).fill("----");
14+
export { bar };
15+
`);
16+
await m.link(() => {});
17+
await m.evaluate();
18+
if (count++ < 4096) {
19+
setTimeout(createModule, 1);
20+
}
21+
return m;
22+
}
23+
createModule();

0 commit comments

Comments
 (0)
0