[v20.x] backport vm-related memory fixes

[joyeecheung]

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 #49491 and #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 #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
v8/v8@b60a03d
and
v8/v8@0aa622e
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
#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
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]

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.

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]

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
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]

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

PR-URL: #49671
Reviewed-By: James M Snell [email protected]
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Yagiz Nizipli [email protected]
Reviewed-By: Michaël Zasso [email protected]

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.

PR-URL: #49671
Reviewed-By: James M Snell [email protected]
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Yagiz Nizipli [email protected]
Reviewed-By: Michaël Zasso [email protected]

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: #49710
Refs: nodejs/reliability#669
Reviewed-By: Franziska Hinkelmann [email protected]
Reviewed-By: Michaël Zasso [email protected]
Reviewed-By: Luigi Pinca [email protected]
Reviewed-By: Rich Trott [email protected]

[nodejs-github-bot]

Review requested:

• @nodejs/gyp
• @nodejs/loaders
• @nodejs/v8-update
• @nodejs/vm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/54245/

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/5586/

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/5586/

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/5586/

[mcollina]

lgtm

[github-actions]

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @mcollina.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment.

[addaleax]

Honestly, to me this seems very much fine to land without breaking the ABI version.

It's a minor ABI breakage, however, in that the V8 patches mostly just changes the type casts and the class hierarchy of a few V8 classes, the type cast changes would only incur check failures in debug mode

… if they fail, right? I think that’s fine because it only happens when casting to a value that isn’t actually of the expected type.

while the breakages introduced by class hierarchy change is implementation-defined, they are both POD types with no instance members, but theoretically this could still break the ABI.

I don’t think even a class layout change would break the ABI here? Users are not expected to subclass these types, I think it’s fair to call that unsupported, and the rest is all pointer accesses through Local<...> anyway.

If regardless of that the concern about ABI breakage is still too big, you could always remove the inheritance of Script and UnboundScript from Data and add static methods to convert between Local<Script> and Local<Data>, using V8’s own internal conversion methods, right? Then the classes would look exactly the same to consumers.

[joyeecheung]

you could always remove the inheritance of Script and UnboundScript from Data and add static methods to convert between Local<Script> and Local, using V8’s own internal conversion methods, right?

That sounds like a better idea. Actually on that note, I wonder if we can just add some new SetInternalField overloads specifically for v8::Module and v8::UnboundScript, now that could involve some pretty dumb copy-pasting but if we aren't going to update V8 on v20.x much it's probably okay anyway, and that probably reduces the likelihood of breakages to zero

[addaleax]

Agreed, in this form this is definitely not an ABI or API breakage concern 🙂

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/5587/

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/5587/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/54256/

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/5587/

[ruyadorno]

Landed in e49a573...be211ef