Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v19.8.0] module_wrap.cc:599: Assertion (it) != (env->id_to_function_map.end())' failed. #47096

Closed
liuxingbaoyu opened this issue Mar 14, 2023 · 26 comments · Fixed by #47101 or CSE210-G13/my-roommates#93
Labels
confirmed-bug Issues with confirmed bugs. module Issues and PRs related to the module subsystem.

Comments

@liuxingbaoyu
Copy link
Contributor

liuxingbaoyu commented Mar 14, 2023

edit @bnoordhuis: fixed in v19.8.1

Version

v19.8.0

Platform

all

Subsystem

No response

What steps will reproduce the bug?

Sorry haven't found a minimal repro.
I'll update here if I do.
Currently, it can be reproduced through the following steps.
git clone https://github.com/babel/babel
make bootstrap
yarn jest

This is still not minimal, but I can't find where it crashes.
https://github.com/liuxingbaoyu/node-bug

How often does it reproduce? Is there a required condition?

always

What is the expected behavior? Why is that the expected behavior?

No response

What do you see instead?

2023-03-14T19:29:13.0710897Z Administrator:  C:\Windows\system32\cmd.exe [5240]: c:\ws\src\module_wrap.cc:599: Assertion `(it) != (env->id_to_function_map.end())' failed.
2023-03-14T19:29:13.3555749Z  1: 00007FF66C7722FF node_api_throw_syntax_error+180191
2023-03-14T19:29:13.3556967Z  2: 00007FF66C6F69D6 v8::internal::MicrotaskQueue::GetMicrotasksScopeDepth+61942
2023-03-14T19:29:13.3558529Z  3: 00007FF66C6F6DB2 v8::internal::MicrotaskQueue::GetMicrotasksScopeDepth+62930
2023-03-14T19:29:13.3559858Z  4: 00007FF66C737520 v8::internal::ReusableUnoptimizedCompileState::ast_raw_string_zone+9696
2023-03-14T19:29:13.3561012Z  5: 00007FF66D0F53AD v8::internal::Isolate::RunHostImportModuleDynamicallyCallback+605
2023-03-14T19:29:13.3562005Z  6: 00007FF66CD5AFD4 v8::internal::Runtime::SetObjectProperty+2548
2023-03-14T19:29:13.3562926Z  7: 00007FF66D2DFC0C v8::internal::SetupIsolateDelegate::SetupHeap+607212
2023-03-14T19:29:13.3563798Z  8: 00007FF66D38D0A5 v8::internal::SetupIsolateDelegate::SetupHeap+1316997
2023-03-14T19:29:13.3564699Z  9: 00007FF66D257330 v8::internal::SetupIsolateDelegate::SetupHeap+47888
2023-03-14T19:29:13.3565674Z 10: 00007FF66D257330 v8::internal::SetupIsolateDelegate::SetupHeap+47888
2023-03-14T19:29:13.3566794Z 11: 00007FF66D257330 v8::internal::SetupIsolateDelegate::SetupHeap+47888
2023-03-14T19:29:13.3588911Z 12: 00007FF66D28EE58 v8::internal::SetupIsolateDelegate::SetupHeap+276024
2023-03-14T19:29:13.3594449Z 13: 00007FF66D338985 v8::internal::SetupIsolateDelegate::SetupHeap+971109
2023-03-14T19:29:13.3595158Z 14: 00007FF66D27ECE0 v8::internal::SetupIsolateDelegate::SetupHeap+210112
2023-03-14T19:29:13.3595968Z 15: 00007FF66D2559AB v8::internal::SetupIsolateDelegate::SetupHeap+41355
2023-03-14T19:29:13.3603881Z 16: 00007FF66D105CA0 v8::internal::Execution::CallWasm+1664
2023-03-14T19:29:13.3608875Z 17: 00007FF66D105DBB v8::internal::Execution::CallWasm+1947
2023-03-14T19:29:13.3611841Z 18: 00007FF66D106B6A v8::internal::Execution::TryCallScript+346
2023-03-14T19:29:13.3612868Z 19: 00007FF66D0DE6E2 v8::internal::MicrotaskQueue::RunMicrotasks+370
2023-03-14T19:29:13.3613780Z 20: 00007FF66D0DE4AA v8::internal::MicrotaskQueue::PerformCheckpointInternal+74
2023-03-14T19:29:13.3614380Z 21: 00007FF66C7A680E node::CallbackScope::~CallbackScope+414
2023-03-14T19:29:13.3614959Z 22: 00007FF66C7A6CB0 node::CallbackScope::~CallbackScope+1600
2023-03-14T19:29:13.3615592Z 23: 00007FF66C79E378 v8::internal::compiler::Operator::EffectOutputCount+248
2023-03-14T19:29:13.3616279Z 24: 00007FF66C6EFCAB v8::internal::MicrotaskQueue::GetMicrotasksScopeDepth+33995
2023-03-14T19:29:13.3616899Z 25: 00007FF66C6E241F v8::base::CPU::has_fpu+50495
2023-03-14T19:29:13.3617533Z 26: 00007FF66C6F1775 v8::internal::MicrotaskQueue::GetMicrotasksScopeDepth+40853
2023-03-14T19:29:13.3618080Z 27: 00007FF66C7D79E7 uv_timer_stop+1207
2023-03-14T19:29:13.3618550Z 28: 00007FF66C7D3E5B uv_update_time+491
2023-03-14T19:29:13.3619020Z 29: 00007FF66C7D39A2 uv_run+1266
2023-03-14T19:29:13.3619472Z 30: 00007FF66C7A5F95 node::SpinEventLoop+389
2023-03-14T19:29:13.3620296Z 31: 00007FF66C6AD3C8 cppgc::internal::Marker::conservative_visitor+51768
2023-03-14T19:29:13.3621201Z 32: 00007FF66C732A4E node::InitializeOncePerProcess+2990
2023-03-14T19:29:13.3621804Z 33: 00007FF66C734D0E node::Start+3566
2023-03-14T19:29:13.3622418Z 34: 00007FF66C733F50 node::Start+48
2023-03-14T19:29:13.3622911Z 35: 00007FF66C53E44C AES_cbc_encrypt+150140
2023-03-14T19:29:13.3623418Z 36: 00007FF66D8A13D4 inflateValidate+19028
2023-03-14T19:29:13.3623974Z 37: 00007FF8CC524DE0 BaseThreadInitThunk+16
2023-03-14T19:29:13.3624501Z 38: 00007FF8CD89E40B RtlUserThreadStart+43
2023-03-14T19:29:14.5277623Z ##[error]Process completed with exit code 1.

Additional information

This is a regression bug.
Might be related to #46785.

@liuxingbaoyu liuxingbaoyu changed the title [v19.8.0] module_wrap.cc:599: Assertion `(it) != (env->id_to_function_map.end())' failed. [v19.8.0] module_wrap.cc:599: Assertion (it) != (env->id_to_function_map.end())' failed.` Mar 14, 2023
@liuxingbaoyu liuxingbaoyu changed the title [v19.8.0] module_wrap.cc:599: Assertion (it) != (env->id_to_function_map.end())' failed.` [v19.8.0] module_wrap.cc:599: Assertion (it) != (env->id_to_function_map.end())' failed. Mar 14, 2023
@JLHwung
Copy link
Contributor

JLHwung commented Mar 14, 2023

For reference here is the error stack on darwin-arm64:

/tmp/.nvm/versions/node/v19.8.0/bin/node[36950]: ../src/module_wrap.cc:599:MaybeLocal<v8::Promise> node::loader::ImportModuleDynamically(Local<v8::Context>, Local<v8::Data>, Local<v8::Value>, Local<v8::String>, Local<v8::FixedArray>): Assertion `(it) != (env->id_to_function_map.end())' failed.
 1: 0x102500e00 node::Abort() [/tmp/.nvm/versions/node/v19.8.0/bin/node]
 2: 0x102500bcc node::PrintCaughtException(v8::Isolate*, v8::Local<v8::Context>, v8::TryCatch const&) [/tmp/.nvm/versions/node/v19.8.0/bin/node]
 3: 0x1024c932c node::loader::ImportModuleDynamically(v8::Local<v8::Context>, v8::Local<v8::Data>, v8::Local<v8::Value>, v8::Local<v8::String>, v8::Local<v8::FixedArray>) [/tmp/.nvm/versions/node/v19.8.0/bin/node]
 4: 0x1027b092c v8::internal::Isolate::RunHostImportModuleDynamicallyCallback(v8::internal::MaybeHandle<v8::internal::Script>, v8::internal::Handle<v8::internal::Object>, v8::internal::MaybeHandle<v8::internal::Object>) [/tmp/.nvm/versions/node/v19.8.0/bin/node]
 5: 0x102bb7604 v8::internal::Runtime_DynamicImportCall(int, unsigned long*, v8::internal::Isolate*) [/tmp/.nvm/versions/node/v19.8.0/bin/node]
 6: 0x102f43524 Builtins_CEntry_Return1_DontSaveFPRegs_ArgvInRegister_NoBuiltinExit [/tmp/.nvm/versions/node/v19.8.0/bin/node]
 7: 0x102fe849c Builtins_CallRuntimeHandler [/tmp/.nvm/versions/node/v19.8.0/bin/node]
 8: 0x102ec0064 Builtins_InterpreterEntryTrampoline [/tmp/.nvm/versions/node/v19.8.0/bin/node]
 9: 0x102ec0064 Builtins_InterpreterEntryTrampoline [/tmp/.nvm/versions/node/v19.8.0/bin/node]
10: 0x102ec0064 Builtins_InterpreterEntryTrampoline [/tmp/.nvm/versions/node/v19.8.0/bin/node]
11: 0x102ef68b4 Builtins_AsyncFunctionAwaitResolveClosure [/tmp/.nvm/versions/node/v19.8.0/bin/node]
12: 0x102f96e38 Builtins_PromiseFulfillReactionJob [/tmp/.nvm/versions/node/v19.8.0/bin/node]
13: 0x102ee6834 Builtins_RunMicrotasks [/tmp/.nvm/versions/node/v19.8.0/bin/node]
14: 0x102ebe3c4 Builtins_JSRunMicrotasksEntry [/tmp/.nvm/versions/node/v19.8.0/bin/node]
15: 0x1027939a0 v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) [/tmp/.nvm/versions/node/v19.8.0/bin/node]
16: 0x102793e90 v8::internal::(anonymous namespace)::InvokeWithTryCatch(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) [/tmp/.nvm/versions/node/v19.8.0/bin/node]
17: 0x10279406c v8::internal::Execution::TryRunMicrotasks(v8::internal::Isolate*, v8::internal::MicrotaskQueue*) [/tmp/.nvm/versions/node/v19.8.0/bin/node]
18: 0x1027bbfe4 v8::internal::MicrotaskQueue::RunMicrotasks(v8::internal::Isolate*) [/tmp/.nvm/versions/node/v19.8.0/bin/node]
19: 0x1027bc880 v8::internal::MicrotaskQueue::PerformCheckpoint(v8::Isolate*) [/tmp/.nvm/versions/node/v19.8.0/bin/node]
20: 0x1026c65ec v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, unsigned long*, int) [/tmp/.nvm/versions/node/v19.8.0/bin/node]
21: 0x1026c5d5c v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [/tmp/.nvm/versions/node/v19.8.0/bin/node]
22: 0x102f433ec Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_BuiltinExit [/tmp/.nvm/versions/node/v19.8.0/bin/node]
23: 0x107b53568 
24: 0x102ebe4f0 Builtins_JSEntryTrampoline [/tmp/.nvm/versions/node/v19.8.0/bin/node]
25: 0x102ebe184 Builtins_JSEntry [/tmp/.nvm/versions/node/v19.8.0/bin/node]
26: 0x1027939d0 v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) [/tmp/.nvm/versions/node/v19.8.0/bin/node]
27: 0x102792f28 v8::internal::Execution::Call(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*) [/tmp/.nvm/versions/node/v19.8.0/bin/node]
28: 0x102676a2c v8::Function::Call(v8::Local<v8::Context>, v8::Local<v8::Value>, int, v8::Local<v8::Value>*) [/tmp/.nvm/versions/node/v19.8.0/bin/node]
29: 0x102448d0c node::InternalCallbackScope::Close() [/tmp/.nvm/versions/node/v19.8.0/bin/node]
30: 0x102448fd8 node::InternalMakeCallback(node::Environment*, v8::Local<v8::Object>, v8::Local<v8::Object>, v8::Local<v8::Function>, int, v8::Local<v8::Value>*, node::async_context) [/tmp/.nvm/versions/node/v19.8.0/bin/node]
31: 0x10245ef9c node::AsyncWrap::MakeCallback(v8::Local<v8::Function>, int, v8::Local<v8::Value>*) [/tmp/.nvm/versions/node/v19.8.0/bin/node]
32: 0x1025ba658 node::StreamBase::CallJSOnreadMethod(long, v8::Local<v8::ArrayBuffer>, unsigned long, node::StreamBase::StreamBaseJSChecks) [/tmp/.nvm/versions/node/v19.8.0/bin/node]
33: 0x1025bbe94 node::EmitToJSStreamListener::OnStreamRead(long, uv_buf_t const&) [/tmp/.nvm/versions/node/v19.8.0/bin/node]
34: 0x1025bff40 node::LibuvStreamWrap::OnUvRead(long, uv_buf_t const*) [/tmp/.nvm/versions/node/v19.8.0/bin/node]
35: 0x1025c06dc node::LibuvStreamWrap::ReadStart()::$_1::__invoke(uv_stream_s*, long, uv_buf_t const*) [/tmp/.nvm/versions/node/v19.8.0/bin/node]
36: 0x102ea8244 uv__stream_io [/tmp/.nvm/versions/node/v19.8.0/bin/node]
37: 0x102eb035c uv__io_poll [/tmp/.nvm/versions/node/v19.8.0/bin/node]
38: 0x102e9e088 uv_run [/tmp/.nvm/versions/node/v19.8.0/bin/node]
39: 0x1024496f4 node::SpinEventLoopInternal(node::Environment*) [/tmp/.nvm/versions/node/v19.8.0/bin/node]
40: 0x10253dee4 node::NodeMainInstance::Run() [/tmp/.nvm/versions/node/v19.8.0/bin/node]
41: 0x1024ce260 node::LoadSnapshotDataAndRun(node::SnapshotData const**, node::InitializationResultImpl const*) [/tmp/.nvm/versions/node/v19.8.0/bin/node]
42: 0x1024ce4c8 node::Start(int, char**) [/tmp/.nvm/versions/node/v19.8.0/bin/node]
43: 0x19bbbbe50 start [/usr/lib/dyld]

@azianmike
Copy link

We have a similar issue but a slightly different stack trace. Might be related?

Mar 14 02:53:16 PM  /opt/render/project/nodes/node-19.8.0/bin/node[78]: ../src/module_wrap.cc:599:v8::MaybeLocal<v8::Promise> node::loader::ImportModuleDynamically(v8::Local<v8::Context>, v8::Local<v8::Data>, v8::Local<v8::Value>, v8::Local<v8::String>, v8::Local<v8::FixedArray>): Assertion `(it) != (env->id_to_function_map.end())' failed.
Mar 14 02:53:16 PM   1: 0xbf9680 node::Abort() [/opt/render/project/nodes/node-19.8.0/bin/node]
Mar 14 02:53:16 PM   2: 0xbf96fe  [/opt/render/project/nodes/node-19.8.0/bin/node]
Mar 14 02:53:16 PM   3: 0xbaf577  [/opt/render/project/nodes/node-19.8.0/bin/node]
Mar 14 02:53:16 PM   4: 0xf5966f v8::internal::Isolate::RunHostImportModuleDynamicallyCallback(v8::internal::MaybeHandle<v8::internal::Script>, v8::internal::Handle<v8::internal::Object>, v8::internal::MaybeHandle<v8::internal::Object>) [/opt/render/project/nodes/node-19.8.0/bin/node]
Mar 14 02:53:16 PM   5: 0x13c2ffb v8::internal::Runtime_DynamicImportCall(int, unsigned long*, v8::internal::Isolate*) [/opt/render/project/nodes/node-19.8.0/bin/node]
Mar 14 02:53:16 PM   6: 0x18425f4  [/opt/render/project/nodes/node-19.8.0/bin/node]
Mar 14 02:53:16 PM  Aborted
Mar 14 02:53:16 PM  error Command failed with exit code 134.

@azianmike
Copy link

Note: We hard-pinned our node version to 19.7 (without any other code changes) and that fixed our issue. Obviously would love to see if the issue can be fixed so we can upgrade to 19.8

@justinvp
Copy link

justinvp commented Mar 15, 2023

We're consistently hitting the following on 19.8.0:

/opt/hostedtoolcache/node/19.8.0/x64/bin/node[18122]: ../src/module_wrap.cc:599:v8::MaybeLocal<v8::Promise> node::loader::ImportModuleDynamically(v8::Local<v8::Context>, v8::Local<v8::Data>, v8::Local<v8::Value>, v8::Local<v8::String>, v8::Local<v8::FixedArray>): Assertion `(it) != (env->id_to_function_map.end())' failed.
 1: 0xbf9680 node::Abort() [/opt/hostedtoolcache/node/19.8.0/x64/bin/node]
 2: 0xbf96fe  [/opt/hostedtoolcache/node/19.8.0/x64/bin/node]
 3: 0xbaf577  [/opt/hostedtoolcache/node/19.8.0/x64/bin/node]
 4: 0xf5966f v8::internal::Isolate::RunHostImportModuleDynamicallyCallback(v8::internal::MaybeHandle<v8::internal::Script>, v8::internal::Handle<v8::internal::Object>, v8::internal::MaybeHandle<v8::internal::Object>) [/opt/hostedtoolcache/node/19.8.0/x64/bin/node]
 5: 0x13c2ffb v8::internal::Runtime_DynamicImportCall(int, unsigned long*, v8::internal::Isolate*) [/opt/hostedtoolcache/node/19.8.0/x64/bin/node]
 6: 0x18425f4  [/opt/hostedtoolcache/node/19.8.0/x64/bin/node]

Previous runs on 19.7.0 were not hitting this.

@samuelbailey123
Copy link

I also have this problem.

zloirock added a commit to zloirock/core-js that referenced this issue Mar 15, 2023
@APiligrim
Copy link

APiligrim commented Mar 15, 2023

I have this problem as well 😢 My deployment is failing on : /nodes/node-19.8.0/. Downgraded to an older version and it worked.

@targos
Copy link
Member

targos commented Mar 15, 2023

I confirm that reverting #46785 fixes the crash.

/cc @joyeecheung @nodejs/cpp-reviewers

@targos targos added confirmed-bug Issues with confirmed bugs. module Issues and PRs related to the module subsystem. v19.x and removed v19.x labels Mar 15, 2023
@antonimarek
Copy link

I have this problem as well cry My deployment is failing on : /nodes/node-19.8.0/. Downgraded to an older version and it worked.

For me downgrading to 19.7 also fixed the issue

@ErCollao
Copy link

We're also having the issue, and downgrading to 19.7 fixed it.

@ekwoka
Copy link

ekwoka commented Mar 15, 2023

This is happening on shopify-cli

When installing with homebrew, we can't force shopify-cli to use a different version, nor can we easily install an older version on homebrew.

@Arkham
Copy link

Arkham commented Mar 15, 2023

This is happening on shopify-cli

When installing with homebrew, we can't force shopify-cli to use a different version, nor can we easily install an older version on homebrew.

@ekwoka you can run brew edit shopify-cli and replace depends_on "node" with depends_on "node@18" as a quick workaround.

@ekwoka
Copy link

ekwoka commented Mar 15, 2023

Ah, interesting! I was trying to edit node to get it to use 19.7 but it always would use 19.8 no matter what I tried.

@bnoordhuis bnoordhuis pinned this issue Mar 15, 2023
@NaveenaTsLivearea
Copy link

We're also having the issue, and downgrading to 19.7 fixed it.

@ErCollao chan you share command to downgrade node version?

joyeecheung added a commit to joyeecheung/node that referenced this issue Sep 26, 2023
Previously when managing the importModuleDynamically callback of
vm.compileFunction(), we use an ID number as the host defined option
and maintain a per-Environment ID -> CompiledFnEntry map to retain
the top-level referrer function returned by vm.compileFunction() in
order to pass it back to the callback, but it would leak because with
how we used v8::Persistent to maintain this reference, V8 would not
be able to understand the cycle and would just think that the
CompiledFnEntry was supposed to live forever. We made an attempt
to make that reference known to V8 by making the CompiledFnEntry weak
and using a private symbol to make CompiledFnEntry strongly
references the top-level referrer function in
nodejs#46785, but that turned out to be
unsound, because the there's no guarantee that the top-level function
must be alive while import() can still be initiated from that
function, since V8 could discard the top-level function and only keep
inner functions alive, so relying on the top-level function to keep
the CompiledFnEntry alive could result in use-after-free which caused
a revert of that fix.

With this patch we use a symbol in the host defined options instead of
a number, because with the stage-3 symbol-as-weakmap-keys proposal
we could directly use that symbol to keep the referrer alive using a
WeakMap. As a bonus this also keeps the other kinds of referrers
alive as long as import() can still be initiated from that
Script/Module, so this also fixes the long-standing crash caused by
vm.Script being GC'ed too early when its importModuleDynamically
callback still needs it.

PR-URL: nodejs#48510
Refs: nodejs#44211
Refs: nodejs#42080
Refs: nodejs#47096
Refs: nodejs#43205
Refs: nodejs#38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Sep 26, 2023
Previously we maintain a strong persistent reference to the
ModuleWrap to retrieve the ID-to-ModuleWrap mapping from
the HostImportModuleDynamicallyCallback using the number ID
stored in the host-defined options. As a result the ModuleWrap
would be kept alive until the Environment is shut down, which
would be a leak for user code. With the new symbol-based
host-defined option we can just get the ModuleWrap from the
JS-land WeakMap so there's now no need to maintain this
strong reference. This would at least fix the leak for
vm.SyntheticModule. vm.SourceTextModule is still leaking
due to the strong persistent reference to the v8::Module.

PR-URL: nodejs#48510
Refs: nodejs#44211
Refs: nodejs#42080
Refs: nodejs#47096
Refs: nodejs#43205
Refs: nodejs#38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Sep 26, 2023
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: nodejs#48510
Refs: nodejs#44211
Refs: nodejs#42080
Refs: nodejs#47096
Refs: nodejs#43205
Refs: nodejs#38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Sep 26, 2023
Previously when managing the importModuleDynamically callback of
vm.compileFunction(), we use an ID number as the host defined option
and maintain a per-Environment ID -> CompiledFnEntry map to retain
the top-level referrer function returned by vm.compileFunction() in
order to pass it back to the callback, but it would leak because with
how we used v8::Persistent to maintain this reference, V8 would not
be able to understand the cycle and would just think that the
CompiledFnEntry was supposed to live forever. We made an attempt
to make that reference known to V8 by making the CompiledFnEntry weak
and using a private symbol to make CompiledFnEntry strongly
references the top-level referrer function in
nodejs#46785, but that turned out to be
unsound, because the there's no guarantee that the top-level function
must be alive while import() can still be initiated from that
function, since V8 could discard the top-level function and only keep
inner functions alive, so relying on the top-level function to keep
the CompiledFnEntry alive could result in use-after-free which caused
a revert of that fix.

With this patch we use a symbol in the host defined options instead of
a number, because with the stage-3 symbol-as-weakmap-keys proposal
we could directly use that symbol to keep the referrer alive using a
WeakMap. As a bonus this also keeps the other kinds of referrers
alive as long as import() can still be initiated from that
Script/Module, so this also fixes the long-standing crash caused by
vm.Script being GC'ed too early when its importModuleDynamically
callback still needs it.

PR-URL: nodejs#48510
Refs: nodejs#44211
Refs: nodejs#42080
Refs: nodejs#47096
Refs: nodejs#43205
Refs: nodejs#38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Sep 26, 2023
Previously we maintain a strong persistent reference to the
ModuleWrap to retrieve the ID-to-ModuleWrap mapping from
the HostImportModuleDynamicallyCallback using the number ID
stored in the host-defined options. As a result the ModuleWrap
would be kept alive until the Environment is shut down, which
would be a leak for user code. With the new symbol-based
host-defined option we can just get the ModuleWrap from the
JS-land WeakMap so there's now no need to maintain this
strong reference. This would at least fix the leak for
vm.SyntheticModule. vm.SourceTextModule is still leaking
due to the strong persistent reference to the v8::Module.

PR-URL: nodejs#48510
Refs: nodejs#44211
Refs: nodejs#42080
Refs: nodejs#47096
Refs: nodejs#43205
Refs: nodejs#38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Sep 26, 2023
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: nodejs#48510
Refs: nodejs#44211
Refs: nodejs#42080
Refs: nodejs#47096
Refs: nodejs#43205
Refs: nodejs#38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Sep 26, 2023
Previously when managing the importModuleDynamically callback of
vm.compileFunction(), we use an ID number as the host defined option
and maintain a per-Environment ID -> CompiledFnEntry map to retain
the top-level referrer function returned by vm.compileFunction() in
order to pass it back to the callback, but it would leak because with
how we used v8::Persistent to maintain this reference, V8 would not
be able to understand the cycle and would just think that the
CompiledFnEntry was supposed to live forever. We made an attempt
to make that reference known to V8 by making the CompiledFnEntry weak
and using a private symbol to make CompiledFnEntry strongly
references the top-level referrer function in
nodejs#46785, but that turned out to be
unsound, because the there's no guarantee that the top-level function
must be alive while import() can still be initiated from that
function, since V8 could discard the top-level function and only keep
inner functions alive, so relying on the top-level function to keep
the CompiledFnEntry alive could result in use-after-free which caused
a revert of that fix.

With this patch we use a symbol in the host defined options instead of
a number, because with the stage-3 symbol-as-weakmap-keys proposal
we could directly use that symbol to keep the referrer alive using a
WeakMap. As a bonus this also keeps the other kinds of referrers
alive as long as import() can still be initiated from that
Script/Module, so this also fixes the long-standing crash caused by
vm.Script being GC'ed too early when its importModuleDynamically
callback still needs it.

PR-URL: nodejs#48510
Refs: nodejs#44211
Refs: nodejs#42080
Refs: nodejs#47096
Refs: nodejs#43205
Refs: nodejs#38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Sep 26, 2023
Previously we maintain a strong persistent reference to the
ModuleWrap to retrieve the ID-to-ModuleWrap mapping from
the HostImportModuleDynamicallyCallback using the number ID
stored in the host-defined options. As a result the ModuleWrap
would be kept alive until the Environment is shut down, which
would be a leak for user code. With the new symbol-based
host-defined option we can just get the ModuleWrap from the
JS-land WeakMap so there's now no need to maintain this
strong reference. This would at least fix the leak for
vm.SyntheticModule. vm.SourceTextModule is still leaking
due to the strong persistent reference to the v8::Module.

PR-URL: nodejs#48510
Refs: nodejs#44211
Refs: nodejs#42080
Refs: nodejs#47096
Refs: nodejs#43205
Refs: nodejs#38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Sep 26, 2023
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: nodejs#48510
Refs: nodejs#44211
Refs: nodejs#42080
Refs: nodejs#47096
Refs: nodejs#43205
Refs: nodejs#38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Sep 28, 2023
Previously when managing the importModuleDynamically callback of
vm.compileFunction(), we use an ID number as the host defined option
and maintain a per-Environment ID -> CompiledFnEntry map to retain
the top-level referrer function returned by vm.compileFunction() in
order to pass it back to the callback, but it would leak because with
how we used v8::Persistent to maintain this reference, V8 would not
be able to understand the cycle and would just think that the
CompiledFnEntry was supposed to live forever. We made an attempt
to make that reference known to V8 by making the CompiledFnEntry weak
and using a private symbol to make CompiledFnEntry strongly
references the top-level referrer function in
nodejs#46785, but that turned out to be
unsound, because the there's no guarantee that the top-level function
must be alive while import() can still be initiated from that
function, since V8 could discard the top-level function and only keep
inner functions alive, so relying on the top-level function to keep
the CompiledFnEntry alive could result in use-after-free which caused
a revert of that fix.

With this patch we use a symbol in the host defined options instead of
a number, because with the stage-3 symbol-as-weakmap-keys proposal
we could directly use that symbol to keep the referrer alive using a
WeakMap. As a bonus this also keeps the other kinds of referrers
alive as long as import() can still be initiated from that
Script/Module, so this also fixes the long-standing crash caused by
vm.Script being GC'ed too early when its importModuleDynamically
callback still needs it.

PR-URL: nodejs#48510
Refs: nodejs#44211
Refs: nodejs#42080
Refs: nodejs#47096
Refs: nodejs#43205
Refs: nodejs#38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Sep 28, 2023
Previously we maintain a strong persistent reference to the
ModuleWrap to retrieve the ID-to-ModuleWrap mapping from
the HostImportModuleDynamicallyCallback using the number ID
stored in the host-defined options. As a result the ModuleWrap
would be kept alive until the Environment is shut down, which
would be a leak for user code. With the new symbol-based
host-defined option we can just get the ModuleWrap from the
JS-land WeakMap so there's now no need to maintain this
strong reference. This would at least fix the leak for
vm.SyntheticModule. vm.SourceTextModule is still leaking
due to the strong persistent reference to the v8::Module.

PR-URL: nodejs#48510
Refs: nodejs#44211
Refs: nodejs#42080
Refs: nodejs#47096
Refs: nodejs#43205
Refs: nodejs#38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Sep 28, 2023
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: nodejs#48510
Refs: nodejs#44211
Refs: nodejs#42080
Refs: nodejs#47096
Refs: nodejs#43205
Refs: nodejs#38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
ruyadorno pushed a commit that referenced this issue Sep 28, 2023
Previously when managing the importModuleDynamically callback of
vm.compileFunction(), we use an ID number as the host defined option
and maintain a per-Environment ID -> CompiledFnEntry map to retain
the top-level referrer function returned by vm.compileFunction() in
order to pass it back to the callback, but it would leak because with
how we used v8::Persistent to maintain this reference, V8 would not
be able to understand the cycle and would just think that the
CompiledFnEntry was supposed to live forever. We made an attempt
to make that reference known to V8 by making the CompiledFnEntry weak
and using a private symbol to make CompiledFnEntry strongly
references the top-level referrer function in
#46785, but that turned out to be
unsound, because the there's no guarantee that the top-level function
must be alive while import() can still be initiated from that
function, since V8 could discard the top-level function and only keep
inner functions alive, so relying on the top-level function to keep
the CompiledFnEntry alive could result in use-after-free which caused
a revert of that fix.

With this patch we use a symbol in the host defined options instead of
a number, because with the stage-3 symbol-as-weakmap-keys proposal
we could directly use that symbol to keep the referrer alive using a
WeakMap. As a bonus this also keeps the other kinds of referrers
alive as long as import() can still be initiated from that
Script/Module, so this also fixes the long-standing crash caused by
vm.Script being GC'ed too early when its importModuleDynamically
callback still needs it.

Backport-PR-URL: #49874
PR-URL: #48510
Refs: #44211
Refs: #42080
Refs: #47096
Refs: #43205
Refs: #38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
ruyadorno pushed a commit that referenced this issue Sep 28, 2023
Previously we maintain a strong persistent reference to the
ModuleWrap to retrieve the ID-to-ModuleWrap mapping from
the HostImportModuleDynamicallyCallback using the number ID
stored in the host-defined options. As a result the ModuleWrap
would be kept alive until the Environment is shut down, which
would be a leak for user code. With the new symbol-based
host-defined option we can just get the ModuleWrap from the
JS-land WeakMap so there's now no need to maintain this
strong reference. This would at least fix the leak for
vm.SyntheticModule. vm.SourceTextModule is still leaking
due to the strong persistent reference to the v8::Module.

Backport-PR-URL: #49874
PR-URL: #48510
Refs: #44211
Refs: #42080
Refs: #47096
Refs: #43205
Refs: #38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
ruyadorno pushed a commit that referenced this issue Sep 28, 2023
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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
alexfernandez pushed a commit to alexfernandez/node that referenced this issue Nov 1, 2023
Previously when managing the importModuleDynamically callback of
vm.compileFunction(), we use an ID number as the host defined option
and maintain a per-Environment ID -> CompiledFnEntry map to retain
the top-level referrer function returned by vm.compileFunction() in
order to pass it back to the callback, but it would leak because with
how we used v8::Persistent to maintain this reference, V8 would not
be able to understand the cycle and would just think that the
CompiledFnEntry was supposed to live forever. We made an attempt
to make that reference known to V8 by making the CompiledFnEntry weak
and using a private symbol to make CompiledFnEntry strongly
references the top-level referrer function in
nodejs#46785, but that turned out to be
unsound, because the there's no guarantee that the top-level function
must be alive while import() can still be initiated from that
function, since V8 could discard the top-level function and only keep
inner functions alive, so relying on the top-level function to keep
the CompiledFnEntry alive could result in use-after-free which caused
a revert of that fix.

With this patch we use a symbol in the host defined options instead of
a number, because with the stage-3 symbol-as-weakmap-keys proposal
we could directly use that symbol to keep the referrer alive using a
WeakMap. As a bonus this also keeps the other kinds of referrers
alive as long as import() can still be initiated from that
Script/Module, so this also fixes the long-standing crash caused by
vm.Script being GC'ed too early when its importModuleDynamically
callback still needs it.

PR-URL: nodejs#48510
Refs: nodejs#44211
Refs: nodejs#42080
Refs: nodejs#47096
Refs: nodejs#43205
Refs: nodejs#38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
alexfernandez pushed a commit to alexfernandez/node that referenced this issue Nov 1, 2023
Previously we maintain a strong persistent reference to the
ModuleWrap to retrieve the ID-to-ModuleWrap mapping from
the HostImportModuleDynamicallyCallback using the number ID
stored in the host-defined options. As a result the ModuleWrap
would be kept alive until the Environment is shut down, which
would be a leak for user code. With the new symbol-based
host-defined option we can just get the ModuleWrap from the
JS-land WeakMap so there's now no need to maintain this
strong reference. This would at least fix the leak for
vm.SyntheticModule. vm.SourceTextModule is still leaking
due to the strong persistent reference to the v8::Module.

PR-URL: nodejs#48510
Refs: nodejs#44211
Refs: nodejs#42080
Refs: nodejs#47096
Refs: nodejs#43205
Refs: nodejs#38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
alexfernandez pushed a commit to alexfernandez/node that referenced this issue Nov 1, 2023
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: nodejs#48510
Refs: nodejs#44211
Refs: nodejs#42080
Refs: nodejs#47096
Refs: nodejs#43205
Refs: nodejs#38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Nov 25, 2023
Previously when managing the importModuleDynamically callback of
vm.compileFunction(), we use an ID number as the host defined option
and maintain a per-Environment ID -> CompiledFnEntry map to retain
the top-level referrer function returned by vm.compileFunction() in
order to pass it back to the callback, but it would leak because with
how we used v8::Persistent to maintain this reference, V8 would not
be able to understand the cycle and would just think that the
CompiledFnEntry was supposed to live forever. We made an attempt
to make that reference known to V8 by making the CompiledFnEntry weak
and using a private symbol to make CompiledFnEntry strongly
references the top-level referrer function in
nodejs#46785, but that turned out to be
unsound, because the there's no guarantee that the top-level function
must be alive while import() can still be initiated from that
function, since V8 could discard the top-level function and only keep
inner functions alive, so relying on the top-level function to keep
the CompiledFnEntry alive could result in use-after-free which caused
a revert of that fix.

With this patch we use a symbol in the host defined options instead of
a number, because with the stage-3 symbol-as-weakmap-keys proposal
we could directly use that symbol to keep the referrer alive using a
WeakMap. As a bonus this also keeps the other kinds of referrers
alive as long as import() can still be initiated from that
Script/Module, so this also fixes the long-standing crash caused by
vm.Script being GC'ed too early when its importModuleDynamically
callback still needs it.

PR-URL: nodejs#48510
Refs: nodejs#44211
Refs: nodejs#42080
Refs: nodejs#47096
Refs: nodejs#43205
Refs: nodejs#38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Nov 25, 2023
Previously we maintain a strong persistent reference to the
ModuleWrap to retrieve the ID-to-ModuleWrap mapping from
the HostImportModuleDynamicallyCallback using the number ID
stored in the host-defined options. As a result the ModuleWrap
would be kept alive until the Environment is shut down, which
would be a leak for user code. With the new symbol-based
host-defined option we can just get the ModuleWrap from the
JS-land WeakMap so there's now no need to maintain this
strong reference. This would at least fix the leak for
vm.SyntheticModule. vm.SourceTextModule is still leaking
due to the strong persistent reference to the v8::Module.

PR-URL: nodejs#48510
Refs: nodejs#44211
Refs: nodejs#42080
Refs: nodejs#47096
Refs: nodejs#43205
Refs: nodejs#38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Nov 25, 2023
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: nodejs#48510
Refs: nodejs#44211
Refs: nodejs#42080
Refs: nodejs#47096
Refs: nodejs#43205
Refs: nodejs#38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Dec 1, 2023
Previously when managing the importModuleDynamically callback of
vm.compileFunction(), we use an ID number as the host defined option
and maintain a per-Environment ID -> CompiledFnEntry map to retain
the top-level referrer function returned by vm.compileFunction() in
order to pass it back to the callback, but it would leak because with
how we used v8::Persistent to maintain this reference, V8 would not
be able to understand the cycle and would just think that the
CompiledFnEntry was supposed to live forever. We made an attempt
to make that reference known to V8 by making the CompiledFnEntry weak
and using a private symbol to make CompiledFnEntry strongly
references the top-level referrer function in
nodejs#46785, but that turned out to be
unsound, because the there's no guarantee that the top-level function
must be alive while import() can still be initiated from that
function, since V8 could discard the top-level function and only keep
inner functions alive, so relying on the top-level function to keep
the CompiledFnEntry alive could result in use-after-free which caused
a revert of that fix.

With this patch we use a symbol in the host defined options instead of
a number, because with the stage-3 symbol-as-weakmap-keys proposal
we could directly use that symbol to keep the referrer alive using a
WeakMap. As a bonus this also keeps the other kinds of referrers
alive as long as import() can still be initiated from that
Script/Module, so this also fixes the long-standing crash caused by
vm.Script being GC'ed too early when its importModuleDynamically
callback still needs it.

PR-URL: nodejs#48510
Refs: nodejs#44211
Refs: nodejs#42080
Refs: nodejs#47096
Refs: nodejs#43205
Refs: nodejs#38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Dec 1, 2023
Previously we maintain a strong persistent reference to the
ModuleWrap to retrieve the ID-to-ModuleWrap mapping from
the HostImportModuleDynamicallyCallback using the number ID
stored in the host-defined options. As a result the ModuleWrap
would be kept alive until the Environment is shut down, which
would be a leak for user code. With the new symbol-based
host-defined option we can just get the ModuleWrap from the
JS-land WeakMap so there's now no need to maintain this
strong reference. This would at least fix the leak for
vm.SyntheticModule. vm.SourceTextModule is still leaking
due to the strong persistent reference to the v8::Module.

PR-URL: nodejs#48510
Refs: nodejs#44211
Refs: nodejs#42080
Refs: nodejs#47096
Refs: nodejs#43205
Refs: nodejs#38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Dec 1, 2023
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: nodejs#48510
Refs: nodejs#44211
Refs: nodejs#42080
Refs: nodejs#47096
Refs: nodejs#43205
Refs: nodejs#38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Dec 1, 2023
Previously when managing the importModuleDynamically callback of
vm.compileFunction(), we use an ID number as the host defined option
and maintain a per-Environment ID -> CompiledFnEntry map to retain
the top-level referrer function returned by vm.compileFunction() in
order to pass it back to the callback, but it would leak because with
how we used v8::Persistent to maintain this reference, V8 would not
be able to understand the cycle and would just think that the
CompiledFnEntry was supposed to live forever. We made an attempt
to make that reference known to V8 by making the CompiledFnEntry weak
and using a private symbol to make CompiledFnEntry strongly
references the top-level referrer function in
nodejs#46785, but that turned out to be
unsound, because the there's no guarantee that the top-level function
must be alive while import() can still be initiated from that
function, since V8 could discard the top-level function and only keep
inner functions alive, so relying on the top-level function to keep
the CompiledFnEntry alive could result in use-after-free which caused
a revert of that fix.

With this patch we use a symbol in the host defined options instead of
a number, because with the stage-3 symbol-as-weakmap-keys proposal
we could directly use that symbol to keep the referrer alive using a
WeakMap. As a bonus this also keeps the other kinds of referrers
alive as long as import() can still be initiated from that
Script/Module, so this also fixes the long-standing crash caused by
vm.Script being GC'ed too early when its importModuleDynamically
callback still needs it.

PR-URL: nodejs#48510
Refs: nodejs#44211
Refs: nodejs#42080
Refs: nodejs#47096
Refs: nodejs#43205
Refs: nodejs#38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Dec 1, 2023
Previously we maintain a strong persistent reference to the
ModuleWrap to retrieve the ID-to-ModuleWrap mapping from
the HostImportModuleDynamicallyCallback using the number ID
stored in the host-defined options. As a result the ModuleWrap
would be kept alive until the Environment is shut down, which
would be a leak for user code. With the new symbol-based
host-defined option we can just get the ModuleWrap from the
JS-land WeakMap so there's now no need to maintain this
strong reference. This would at least fix the leak for
vm.SyntheticModule. vm.SourceTextModule is still leaking
due to the strong persistent reference to the v8::Module.

PR-URL: nodejs#48510
Refs: nodejs#44211
Refs: nodejs#42080
Refs: nodejs#47096
Refs: nodejs#43205
Refs: nodejs#38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this issue Dec 1, 2023
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: nodejs#48510
Refs: nodejs#44211
Refs: nodejs#42080
Refs: nodejs#47096
Refs: nodejs#43205
Refs: nodejs#38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
richardlau pushed a commit that referenced this issue Mar 15, 2024
Previously when managing the importModuleDynamically callback of
vm.compileFunction(), we use an ID number as the host defined option
and maintain a per-Environment ID -> CompiledFnEntry map to retain
the top-level referrer function returned by vm.compileFunction() in
order to pass it back to the callback, but it would leak because with
how we used v8::Persistent to maintain this reference, V8 would not
be able to understand the cycle and would just think that the
CompiledFnEntry was supposed to live forever. We made an attempt
to make that reference known to V8 by making the CompiledFnEntry weak
and using a private symbol to make CompiledFnEntry strongly
references the top-level referrer function in
#46785, but that turned out to be
unsound, because the there's no guarantee that the top-level function
must be alive while import() can still be initiated from that
function, since V8 could discard the top-level function and only keep
inner functions alive, so relying on the top-level function to keep
the CompiledFnEntry alive could result in use-after-free which caused
a revert of that fix.

With this patch we use a symbol in the host defined options instead of
a number, because with the stage-3 symbol-as-weakmap-keys proposal
we could directly use that symbol to keep the referrer alive using a
WeakMap. As a bonus this also keeps the other kinds of referrers
alive as long as import() can still be initiated from that
Script/Module, so this also fixes the long-standing crash caused by
vm.Script being GC'ed too early when its importModuleDynamically
callback still needs it.

PR-URL: #48510
Backport-PR-URL: #51004
Refs: #44211
Refs: #42080
Refs: #47096
Refs: #43205
Refs: #38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
richardlau pushed a commit that referenced this issue Mar 15, 2024
Previously we maintain a strong persistent reference to the
ModuleWrap to retrieve the ID-to-ModuleWrap mapping from
the HostImportModuleDynamicallyCallback using the number ID
stored in the host-defined options. As a result the ModuleWrap
would be kept alive until the Environment is shut down, which
would be a leak for user code. With the new symbol-based
host-defined option we can just get the ModuleWrap from the
JS-land WeakMap so there's now no need to maintain this
strong reference. This would at least fix the leak for
vm.SyntheticModule. vm.SourceTextModule is still leaking
due to the strong persistent reference to the v8::Module.

PR-URL: #48510
Backport-PR-URL: #51004
Refs: #44211
Refs: #42080
Refs: #47096
Refs: #43205
Refs: #38695
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
richardlau pushed a commit that referenced this issue Mar 15, 2024
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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. module Issues and PRs related to the module subsystem.
Projects
None yet