[v20.x] backport vm-related memory fixes (!49874) · Merge requests · Rodrigo Test / Test Group-nodejs / node

Rodrigo Muino Tomonari requested to merge github/fork/joyeecheung/backport-vm-changes into v20.x-staging Sep 26, 2023

This includes multiple PRs for the fixes and the tests related to vm memory issues.

To avoid breaking the ABI on v20.x, instead of backporting https://github.com/nodejs/node/pull/49491 and https://github.com/nodejs/node/pull/49419, we just introduce an completely new API v8::Object::SetInternalFieldForNodeCore() to our v20.x fork of V8 which can be used to add internal references to v8::UnboundScript and v8::Module, and use that instead to backport https://github.com/nodejs/node/pull/48510 - this should be fine if we add enough warning to tell embedders and addon authors not to use them, and we will not update the V8 fork in v20.x often so it is unlikely to create conflicts.

deps: add v8::Object::SetInternalFieldForNodeCore()

This is a non-ABI breaking solution for https://github.com/v8/v8/commit/b60a03df4cebafb4c92ee644d11617ad73889e5e and https://github.com/v8/v8/commit/0aa622e12893e9921c01a34ce9507b544e599c4a which are necessary for backporting vm-related memory fixes to v20.x.

module: use symbol in WeakMap to manage host defined options

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 https://github.com/nodejs/node/pull/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: https://github.com/nodejs/node/pull/48510 Refs: https://github.com/nodejs/node/issues/44211 Refs: https://github.com/nodejs/node/issues/42080 Refs: https://github.com/nodejs/node/issues/47096 Refs: https://github.com/nodejs/node/issues/43205 Refs: https://github.com/nodejs/node/issues/38695 Reviewed-By: Antoine du Hamel duhamelantoine1995@gmail.com Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com Reviewed-By: Stephen Belanger admin@stephenbelanger.com

module: fix leak of vm.SyntheticModule

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.

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.

test: add checkIfCollectable to test/common/gc.js

PR-URL: https://github.com/nodejs/node/pull/49671 Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com Reviewed-By: Yagiz Nizipli yagiz@nizipli.com Reviewed-By: Michaël Zasso targos@protonmail.com

test: use checkIfCollectable in vm leak tests

Previously we simply create a lot of the target objects and check if the process crash due to OOM. Due to how we use emphemeron GC to handle memory management, which is inefficient but necessary for correctness, the tests can produce false positives as the GC isn't efficient enough to catch up with a very fast heap growth.

This patch uses a new checkIfCollectable() utility to terminate the test early once we detect that any of the target object can actually be garbage collected. This should lower the chance of false positives. As a drive-by this also allows us to use setImmediate() to grow the heap even faster to make the tests run faster.

test: deflake test-vm-contextified-script-leak

Similar to the test-vm-source-text-module-leak fix, use a snapshot to force a thorough GC in order to prevent false positives.

PR-URL: https://github.com/nodejs/node/pull/49710 Refs: https://github.com/nodejs/reliability/issues/669 Reviewed-By: Franziska Hinkelmann franziska.hinkelmann@gmail.com Reviewed-By: Michaël Zasso targos@protonmail.com Reviewed-By: Luigi Pinca luigipinca@gmail.com Reviewed-By: Rich Trott rtrott@gmail.com

Admin message