[18.x] vm: backport vm-related fixes #51004

Original commit message: [symbol-as-weakmap-key] Implement Symbol as WeakMap Keys Allow non-registered symbols as keys in weakmap and weakset. Allow non-registered symbols as target and unregisterToken in WeakRef and FinalizationRegistry. Bug: v8:12947 Change-Id: Ieb63bda66e3cc378879ac651e23300b71caed627 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3865056 Reviewed-by: Dominik Inführ <dinfuehr@chromium.org> Commit-Queue: Marja Hölttä <marja@chromium.org> Reviewed-by: Jakob Linke <jgruber@chromium.org> Cr-Commit-Position: refs/heads/main@{#83313} Refs: v8/v8@c400af4

Original commit message: [symbol-as-weakmap-key] Add tests to check weak collection size ... after gc. This CL also adds a runtime test function GetWeakCollectionSize to get the weak collection size. Bug: v8:12947 Change-Id: I4aff39165a54b63b3d690bfea71c2a439da01d00 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3905071 Reviewed-by: Marja Hölttä <marja@chromium.org> Commit-Queue: 王澳 <wangao.james@bytedance.com> Cr-Commit-Position: refs/heads/main@{#83464} Refs: v8/v8@7f5daed

Original commit message: [symbol-as-weakmap-key] Stage the feature Bug: v8:12947 Change-Id: I0a151a6b301ee93675cc9f87a4fa24cb1be76462 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3928061 Auto-Submit: Shu-yu Guo <syg@chromium.org> Commit-Queue: Marja Hölttä <marja@chromium.org> Reviewed-by: Marja Hölttä <marja@chromium.org> Cr-Commit-Position: refs/heads/main@{#83483} Refs: v8/v8@9a98f96

Original commit message: [symbol-as-weakmap-key] Fix DCHECKs and add CanBeHeldWeakly There are a few DCHECKs that weren't updated to allow for Symbols as weak collection keys. This CL updates those DCHECKs and also does the following refactors for clarity: - Add Object::CanBeHeldWeakly - Rename GotoIfCannotBeWeakKey -> GotoIfCannotBeHeldWeakly to align with spec AO name Bug: chromium:1370400, chromium:1370402, v8:12947 Change-Id: I380840c8377497feae97e3fca37555dae0dcc255 Fixed: chromium:1370400, chromium:1370402 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3928150 Auto-Submit: Shu-yu Guo <syg@chromium.org> Reviewed-by: Marja Hölttä <marja@chromium.org> Commit-Queue: Marja Hölttä <marja@chromium.org> Cr-Commit-Position: refs/heads/main@{#83507} Refs: v8/v8@94e8282

Original commit message: [inspector] Support Symbols in EntryPreview The Symbols-as-WeakMap-keys proposal allows non-Symbol.for Symbol values in weak collections, which means it can show in EntryPreviews. Also apparently Symbols in regular Maps and Sets were also unsupported. Bug: v8:13350, v8:12947 Change-Id: Ib10476fa2f3c7f59af67933f0bf61640be1bbd97 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3930037 Reviewed-by: Benedikt Meurer <bmeurer@chromium.org> Reviewed-by: Simon Zünd <szuend@chromium.org> Commit-Queue: Shu-yu Guo <syg@chromium.org> Cr-Commit-Position: refs/heads/main@{#83518} Refs: v8/v8@3dd9576

Original commit message: [symbol-as-weakmap-key] Fix DCHECKs when clearing JS weakrefs Bug: chromium:1372500, v8:12947 Fixed: chromium:1372500 Change-Id: Id6330de5886e4ea72544b307c358e2190ea47d9c Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3942586 Reviewed-by: Anton Bikineev <bikineev@chromium.org> Commit-Queue: Shu-yu Guo <syg@chromium.org> Cr-Commit-Position: refs/heads/main@{#83632} Refs: v8/v8@1fada6b

Original commit message: [symbol-as-weakmap-key] Ship the proposal I2S with 3 LGTMs: https://groups.google.com/a/chromium.org/g/blink-dev/c/E6pDZP_TiBA/m/ZcXLwiz8AAAJ Bug: v8:12947 Change-Id: Ibce4abc8b0610afb2041d44cc9ed136db8b62c0d Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4004610 Commit-Queue: Shu-yu Guo <syg@chromium.org> Reviewed-by: Camillo Bruni <cbruni@chromium.org> Cr-Commit-Position: refs/heads/main@{#84128} Refs: v8/v8@705e374

Original commit message: [symbol-as-weakmap-key] Remove --harmony-symbol-as-weakmap-key The proposal has shipped since Nov 2022. Bug: v8:12947 Change-Id: Ibb2e9cf1d22e4b754ec7625ae38175fc96007255 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4451066 Commit-Queue: Shu-yu Guo <syg@chromium.org> Reviewed-by: Adam Klein <adamk@chromium.org> Cr-Commit-Position: refs/heads/main@{#87180} Refs: v8/v8@71ff688

PR-URL: nodejs#49855 Fixes: nodejs#49848 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

In preparation of https://chromium-review.googlesource.com/c/v8/v8/+/4707972 which changes the return value to v8::Data. PR-URL: nodejs#48943 Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

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 v18.x. PR-URL: nodejs#49874 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

There is no need to initialize the internal fields to undefined and then initialize them to something else in the caller. Simply pass the internal fields into the constructor to initialize them just once. PR-URL: nodejs#49391 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

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 <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

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 <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

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 <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

PR-URL: nodejs#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>

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: nodejs#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>

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: nodejs#49710 Refs: nodejs/reliability#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>

This makes it possile to hit the in-isolate compilation cache when host-defined options are not necessary. PR-URL: nodejs#49950 Refs: nodejs#35375 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

Set a default host-defined option for vm.compileFunction so that it's consistent with vm.Script. PR-URL: nodejs#50137 Refs: nodejs#35375 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

Instead of using the public versions of the vm APIs internally, use the internal versions so that we can skip unnecessary argument validation. The public versions would need special care to the generation of host-defined options to hit the isolate compilation cache when imporModuleDynamically isn't used, while internally it's almost always used, so this allows us to handle the host-defined options separately. PR-URL: nodejs#50137 Refs: nodejs#35375 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

Users cannot access any API that can be used to return a module or module namespace in this callback without --experimental-vm-modules anyway, so this would eventually lead to a rejection. This patch rejects in this case with our own error message and use a constant host-defined option for the rejection, so that scripts with the same source can still be compiled using the compilation cache if no `import()` is actually called in the script. PR-URL: nodejs#50137 Refs: nodejs#35375 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[18.x] vm: backport vm-related fixes #51004

[18.x] vm: backport vm-related fixes #51004

Commits on Dec 1, 2023