Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 7552de6

Browse filesBrowse files
joyeecheungrichardlau
joyeecheung
authored and
richardlau
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. 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>
1 parent 2e05cf1 commit 7552de6
Copy full SHA for 7552de6

File tree

Expand file treeCollapse file tree

6 files changed

+59
-4
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

6 files changed

+59
-4
lines changed
Open diff view settings
Collapse file

‎src/module_wrap.cc‎

Copy file name to clipboardExpand all lines: src/module_wrap.cc
+6-3Lines 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);
Collapse file

‎src/module_wrap.h‎

Copy file name to clipboardExpand all lines: src/module_wrap.h
+2-1Lines changed: 2 additions & 1 deletion
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
Collapse file

‎src/node_contextify.cc‎

Copy file name to clipboardExpand all lines: src/node_contextify.cc
+4Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -871,7 +871,11 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
871871
"ContextifyScript::New");
872872
return;
873873
}
874+
874875
contextify_script->script_.Reset(isolate, v8_script);
876+
contextify_script->script_.SetWeak();
877+
contextify_script->object()->SetInternalFieldForNodeCore(kUnboundScriptSlot,
878+
v8_script);
875879

876880
std::unique_ptr<ScriptCompiler::CachedData> new_cached_data;
877881
if (produce_cached_data) {
Collapse file

‎src/node_contextify.h‎

Copy file name to clipboardExpand all lines: src/node_contextify.h
+5Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,11 @@ class ContextifyContext : public BaseObject {
149149

150150
class ContextifyScript : public BaseObject {
151151
public:
152+
enum InternalFields {
153+
kUnboundScriptSlot = BaseObject::kInternalFieldCount,
154+
kInternalFieldCount
155+
};
156+
152157
SET_NO_MEMORY_INFO()
153158
SET_MEMORY_INFO_NAME(ContextifyScript)
154159
SET_SELF_SIZE(ContextifyScript)
Collapse file

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

Copy file name to clipboard
+19Lines 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();
Collapse file

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

Copy file name to clipboard
+23Lines 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)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.