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

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

Closed
wants to merge 22 commits into from

Conversation

joyeecheung and others added 12 commits December 1, 2023 20:51
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 <[email protected]>
    Commit-Queue: Marja Hölttä <[email protected]>
    Reviewed-by: Jakob Linke <[email protected]>
    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ä <[email protected]>
    Commit-Queue: 王澳 <[email protected]>
    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 <[email protected]>
    Commit-Queue: Marja Hölttä <[email protected]>
    Reviewed-by: Marja Hölttä <[email protected]>
    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 <[email protected]>
    Reviewed-by: Marja Hölttä <[email protected]>
    Commit-Queue: Marja Hölttä <[email protected]>
    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 <[email protected]>
    Reviewed-by: Simon Zünd <[email protected]>
    Commit-Queue: Shu-yu Guo <[email protected]>
    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 <[email protected]>
    Commit-Queue: Shu-yu Guo <[email protected]>
    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 <[email protected]>
    Reviewed-by: Camillo Bruni <[email protected]>
    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 <[email protected]>
    Reviewed-by: Adam Klein <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#87180}

Refs: v8/v8@71ff688
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 <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
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 <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

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

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. v8 engine Issues and PRs related to the V8 dependency. labels Dec 1, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

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]>
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]>
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]>
PR-URL: nodejs#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]>
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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
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 <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
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 <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
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 <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 15, 2024
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 <[email protected]>
    Reviewed-by: Marja Hölttä <[email protected]>
    Commit-Queue: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#83507}

Refs: v8/v8@94e8282
PR-URL: #51004
Reviewed-By: Chengzhong Wu <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 15, 2024
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 <[email protected]>
    Reviewed-by: Simon Zünd <[email protected]>
    Commit-Queue: Shu-yu Guo <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#83518}

Refs: v8/v8@3dd9576
PR-URL: #51004
Reviewed-By: Chengzhong Wu <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 15, 2024
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 <[email protected]>
    Commit-Queue: Shu-yu Guo <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#83632}

Refs: v8/v8@1fada6b
PR-URL: #51004
Reviewed-By: Chengzhong Wu <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 15, 2024
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 <[email protected]>
    Reviewed-by: Camillo Bruni <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#84128}

Refs: v8/v8@705e374
PR-URL: #51004
Reviewed-By: Chengzhong Wu <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 15, 2024
PR-URL: #49855
Backport-PR-URL: #51004
Fixes: #49848
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 15, 2024
In preparation of https://chromium-review.googlesource.com/c/v8/v8/+/4707972
which changes the return value to v8::Data.

PR-URL: #48943
Backport-PR-URL: #51004
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 15, 2024
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: #49874
Backport-PR-URL: #51004
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 15, 2024
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: #49391
Backport-PR-URL: #51004
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
richardlau pushed a commit that referenced this pull request 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 pull request 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 pull request 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]>
richardlau pushed a commit that referenced this pull request Mar 15, 2024
PR-URL: #49671
Backport-PR-URL: #51004
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]>
richardlau pushed a commit that referenced this pull request Mar 15, 2024
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
Backport-PR-URL: #51004
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]>
richardlau pushed a commit that referenced this pull request Mar 15, 2024
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
Backport-PR-URL: #51004
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]>
richardlau pushed a commit that referenced this pull request Mar 15, 2024
This makes it possile to hit the in-isolate compilation cache when
host-defined options are not necessary.

PR-URL: #49950
Backport-PR-URL: #51004
Refs: #35375
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 15, 2024
Set a default host-defined option for vm.compileFunction so that
it's consistent with vm.Script.

PR-URL: #50137
Backport-PR-URL: #51004
Refs: #35375
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 15, 2024
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: #50137
Backport-PR-URL: #51004
Refs: #35375
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 15, 2024
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: #50137
Backport-PR-URL: #51004
Refs: #35375
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@richardlau
Copy link
Member

Landed in 902d8b3...fe66e9d.

@richardlau richardlau closed this Mar 15, 2024
@SimenB
Copy link
Member

SimenB commented Mar 15, 2024

🚀♥️

@richardlau richardlau mentioned this pull request Mar 20, 2024
jimsynz pushed a commit to jimsynz/cinder-space that referenced this pull request Mar 28, 2024
This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [node](https://nodejs.org) ([source](https://github.com/nodejs/node)) | minor | `18.19.1` -> `18.20.0` |

---

### Release Notes

<details>
<summary>nodejs/node (node)</summary>

### [`v18.20.0`](https://github.com/nodejs/node/releases/tag/v18.20.0): 2024-03-26, Version 18.20.0 &#x27;Hydrogen&#x27; (LTS), @&#8203;richardlau

[Compare Source](nodejs/node@v18.19.1...v18.20.0)

##### Notable Changes

##### Added support for import attributes

Support has been added for import attributes, to replace the old import
assertions syntax. This will aid migration by making the new syntax available
across all currently supported Node.js release lines.

This adds the `with` keyword which should be used in place of the previous
`assert` keyword, which will be removed in a future semver-major Node.js
release.

For example,

```console
import "foo" assert { ... }
```

should be replaced with

```console
import "foo" with { ... }
```

For more details, see

-   [#&#8203;50134](nodejs/node#50134)
-   [#&#8203;51622](nodejs/node#51622)

Contributed by Nicolò Ribaudo in [#&#8203;51136](nodejs/node#51136)
and Antoine du Hamel in [#&#8203;50140](nodejs/node#50140).

##### Doc deprecation for `dirent.path`

Please use newly added `dirent.parentPath` instead.

Contributed by Antoine du Hamel in [#&#8203;50976](nodejs/node#50976)
and [#&#8203;51020](nodejs/node#51020).

##### Experimental node-api feature flags

Introduces an experimental feature to segregate finalizers that affect GC state.
A new type called `node_api_nogc_env` has been introduced as the const version
of `napi_env` and `node_api_nogc_finalize` as a variant of `napi_finalize` that
accepts a `node_api_nogc_env` as its first argument.

This feature can be turned off by defining
`NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT`.

Contributed by Gabriel Schulhof in [#&#8203;50060](nodejs/node#50060).

##### Root certificates updated to NSS 3.98

Certificates added:

-   Telekom Security TLS ECC Root 2020
-   Telekom Security TLS RSA Root 2023

Certificates removed:

-   Security Communication Root CA

##### Updated dependencies

-   ada updated to 2.7.6.
-   base64 updated to 0.5.2.
-   c-ares updated to 1.27.0.
-   corepack updated to 0.25.2.
-   ICU updated to 74.2. Includes CLDR 44.1 and Unicode 15.1.
-   npm updated to 10.5.0. Fixes a regression in signals not being passed onto child processes.
-   simdutf8 updated to 4.0.8.
-   Timezone updated to 2024a.
-   zlib updated to 1.3.0.1-motley-40e35a7.

##### vm: fix V8 compilation cache support for vm.Script

Previously repeated compilation of the same source code using `vm.Script`
stopped hitting the V8 compilation cache after v16.x when support for
`importModuleDynamically` was added to `vm.Script`, resulting in a performance
regression that blocked users (in particular Jest users) from upgrading from
v16.x.

The recent fixes allow the compilation cache to be hit again
for `vm.Script` when `--experimental-vm-modules` is not used even in the
presence of the `importModuleDynamically` option, so that users affected by the
performance regression can now upgrade. Ongoing work is also being done to
enable compilation cache support for `vm.CompileFunction`.

Contributed by Joyee Cheung in [#&#8203;49950](nodejs/node#49950)
and [#&#8203;50137](nodejs/node#50137).

##### Commits

-   \[[`c70383b8d4`](nodejs/node@c70383b8d4)] - **build**: support Python 3.12 (Shi Pujin) [#&#8203;50209](nodejs/node#50209)
-   \[[`4b960c3a4a`](nodejs/node@4b960c3a4a)] - **build**: fix incorrect g++ warning message (Richard Lau) [#&#8203;51695](nodejs/node#51695)
-   \[[`8fdea67694`](nodejs/node@8fdea67694)] - **crypto**: update root certificates to NSS 3.98 (Node.js GitHub Bot) [#&#8203;51794](nodejs/node#51794)
-   \[[`812b126dd9`](nodejs/node@812b126dd9)] - **deps**: V8: cherry-pick [`d90d453`](nodejs/node@d90d4533b053) (Michaël Zasso) [#&#8203;50077](nodejs/node#50077)
-   \[[`9ab8c3db87`](nodejs/node@9ab8c3db87)] - **deps**: update c-ares to 1.27.0 (Node.js GitHub Bot) [#&#8203;51846](nodejs/node#51846)
-   \[[`c688680387`](nodejs/node@c688680387)] - **deps**: update c-ares to 1.26.0 (Node.js GitHub Bot) [#&#8203;51582](nodejs/node#51582)
-   \[[`9498ac8a47`](nodejs/node@9498ac8a47)] - **deps**: compile c-ares with C11 support (Michaël Zasso) [#&#8203;51410](nodejs/node#51410)
-   \[[`8fb743642f`](nodejs/node@8fb743642f)] - **deps**: update c-ares to 1.25.0 (Node.js GitHub Bot) [#&#8203;51385](nodejs/node#51385)
-   \[[`7bea2d7c12`](nodejs/node@7bea2d7c12)] - **deps**: update zlib to 1.3.0.1-motley-40e35a7 (Node.js GitHub Bot) [#&#8203;51274](nodejs/node#51274)
-   \[[`57a38c8f75`](nodejs/node@57a38c8f75)] - **deps**: update zlib to 1.3.0.1-motley-dd5fc13 (Node.js GitHub Bot) [#&#8203;51105](nodejs/node#51105)
-   \[[`b0ca084a6b`](nodejs/node@b0ca084a6b)] - **deps**: update zlib to 1.3-22124f5 (Node.js GitHub Bot) [#&#8203;50910](nodejs/node#50910)
-   \[[`4b43823f37`](nodejs/node@4b43823f37)] - **deps**: update zlib to 1.2.13.1-motley-5daffc7 (Node.js GitHub Bot) [#&#8203;50803](nodejs/node#50803)
-   \[[`f0da591812`](nodejs/node@f0da591812)] - **deps**: update zlib to 1.2.13.1-motley-dfc48fc (Node.js GitHub Bot) [#&#8203;50456](nodejs/node#50456)
-   \[[`16d28a883a`](nodejs/node@16d28a883a)] - **deps**: update base64 to 0.5.2 (Node.js GitHub Bot) [#&#8203;51455](nodejs/node#51455)
-   \[[`13a9e81cb6`](nodejs/node@13a9e81cb6)] - **deps**: update base64 to 0.5.1 (Node.js GitHub Bot) [#&#8203;50629](nodejs/node#50629)
-   \[[`b4502d3ac5`](nodejs/node@b4502d3ac5)] - **deps**: update simdutf to 4.0.8 (Node.js GitHub Bot) [#&#8203;51000](nodejs/node#51000)
-   \[[`183cf8a74a`](nodejs/node@183cf8a74a)] - **deps**: update simdutf to 4.0.4 (Node.js GitHub Bot) [#&#8203;50772](nodejs/node#50772)
-   \[[`11ba8593ea`](nodejs/node@11ba8593ea)] - **deps**: update ada to 2.7.6 (Node.js GitHub Bot) [#&#8203;51542](nodejs/node#51542)
-   \[[`73a946d55c`](nodejs/node@73a946d55c)] - **deps**: update ada to 2.7.5 (Node.js GitHub Bot) [#&#8203;51542](nodejs/node#51542)
-   \[[`cc434c1a39`](nodejs/node@cc434c1a39)] - **deps**: update ada to 2.7.4 (Node.js GitHub Bot) [#&#8203;50815](nodejs/node#50815)
-   \[[`3a3808a6ae`](nodejs/node@3a3808a6ae)] - **deps**: upgrade npm to 10.5.0 (npm team) [#&#8203;51913](nodejs/node#51913)
-   \[[`c8876d765c`](nodejs/node@c8876d765c)] - **deps**: upgrade npm to 10.3.0 (npm team) [#&#8203;51431](nodejs/node#51431)
-   \[[`5aec3af460`](nodejs/node@5aec3af460)] - **deps**: update corepack to 0.25.2 (Node.js GitHub Bot) [#&#8203;51810](nodejs/node#51810)
-   \[[`a593985326`](nodejs/node@a593985326)] - **deps**: update corepack to 0.24.1 (Node.js GitHub Bot) [#&#8203;51459](nodejs/node#51459)
-   \[[`d1a9237bf5`](nodejs/node@d1a9237bf5)] - **deps**: update corepack to 0.24.0 (Node.js GitHub Bot) [#&#8203;51318](nodejs/node#51318)
-   \[[`adac0c7a63`](nodejs/node@adac0c7a63)] - **deps**: update corepack to 0.23.0 (Node.js GitHub Bot) [#&#8203;50563](nodejs/node#50563)
-   \[[`4a6f83e32a`](nodejs/node@4a6f83e32a)] - **deps**: escape Python strings correctly (Michaël Zasso) [#&#8203;50695](nodejs/node#50695)
-   \[[`c13969e52a`](nodejs/node@c13969e52a)] - **deps**: V8: cherry-pick [`ea996ad`](nodejs/node@ea996ad04a68) (Nicolò Ribaudo) [#&#8203;51136](nodejs/node#51136)
-   \[[`6fbf0ba5c3`](nodejs/node@6fbf0ba5c3)] - **deps**: V8: cherry-pick [`a0fd320`](nodejs/node@a0fd3209dda8) (Nicolò Ribaudo) [#&#8203;51136](nodejs/node#51136)
-   \[[`68fd7516e1`](nodejs/node@68fd7516e1)] - **deps**: update timezone to 2024a (Michaël Zasso) [#&#8203;51723](nodejs/node#51723)
-   \[[`f9b229ebe1`](nodejs/node@f9b229ebe1)] - **deps**: update icu to 74.2 (Michaël Zasso) [#&#8203;51723](nodejs/node#51723)
-   \[[`90c73d2eb4`](nodejs/node@90c73d2eb4)] - **deps**: update timezone to 2023d (Node.js GitHub Bot) [#&#8203;51461](nodejs/node#51461)
-   \[[`2a2bf57028`](nodejs/node@2a2bf57028)] - **deps**: update icu to 74.1 (Node.js GitHub Bot) [#&#8203;50515](nodejs/node#50515)
-   \[[`425e011e52`](nodejs/node@425e011e52)] - **deps**: add v8::Object::SetInternalFieldForNodeCore() (Joyee Cheung) [#&#8203;49874](nodejs/node#49874)
-   \[[`58c70344a2`](nodejs/node@58c70344a2)] - **deps**: V8: cherry-pick [`705e374`](nodejs/node@705e374124ae) (Joyee Cheung) [#&#8203;51004](nodejs/node#51004)
-   \[[`b0e88899e1`](nodejs/node@b0e88899e1)] - **deps**: V8: cherry-pick [`1fada6b`](nodejs/node@1fada6b36f8d) (Joyee Cheung) [#&#8203;51004](nodejs/node#51004)
-   \[[`d87a810b81`](nodejs/node@d87a810b81)] - **deps**: V8: cherry-pick [`3dd9576`](nodejs/node@3dd9576ce336) (Joyee Cheung) [#&#8203;51004](nodejs/node#51004)
-   \[[`6d50966876`](nodejs/node@6d50966876)] - **deps**: V8: cherry-pick [`94e8282`](nodejs/node@94e8282325a1) (Joyee Cheung) [#&#8203;51004](nodejs/node#51004)
-   \[[`fafbacdfec`](nodejs/node@fafbacdfec)] - **deps**: V8: cherry-pick [`9a98f96`](nodejs/node@9a98f96b6d68) (Joyee Cheung) [#&#8203;51004](nodejs/node#51004)
-   \[[`d4a530ed8d`](nodejs/node@d4a530ed8d)] - **deps**: V8: cherry-pick [`7f5daed`](nodejs/node@7f5daed62d47) (Joyee Cheung) [#&#8203;51004](nodejs/node#51004)
-   \[[`1ce901b164`](nodejs/node@1ce901b164)] - **deps**: V8: cherry-pick [`c400af4`](nodejs/node@c400af48b5ef) (Joyee Cheung) [#&#8203;51004](nodejs/node#51004)
-   \[[`f232064f35`](nodejs/node@f232064f35)] - **doc**: fix historical experimental fetch flag (Kenrick) [#&#8203;51506](nodejs/node#51506)
-   \[[`194ff6a40f`](nodejs/node@194ff6a40f)] - **(SEMVER-MINOR)** **doc**: add deprecation notice to `dirent.path` (Antoine du Hamel) [#&#8203;50976](nodejs/node#50976)
-   \[[`0f09267dc6`](nodejs/node@0f09267dc6)] - **(SEMVER-MINOR)** **doc**: deprecate `dirent.path` (Antoine du Hamel) [#&#8203;50976](nodejs/node#50976)
-   \[[`8bfb8f5b2f`](nodejs/node@8bfb8f5b2f)] - **doc,crypto**: further clarify RSA_PKCS1\_PADDING support (Tobias Nießen) [#&#8203;51799](nodejs/node#51799)
-   \[[`c7baf7b274`](nodejs/node@c7baf7b274)] - **doc,crypto**: add changelog and note about disabled RSA_PKCS1\_PADDING (Filip Skokan) [#&#8203;51782](nodejs/node#51782)
-   \[[`a193be3dc2`](nodejs/node@a193be3dc2)] - **esm**: use import attributes instead of import assertions (Antoine du Hamel) [#&#8203;50140](nodejs/node#50140)
-   \[[`26e8f7793e`](nodejs/node@26e8f7793e)] - **(SEMVER-MINOR)** **fs**: introduce `dirent.parentPath` (Antoine du Hamel) [#&#8203;50976](nodejs/node#50976)
-   \[[`5b5e5192f7`](nodejs/node@5b5e5192f7)] - **lib**: fix compileFunction throws range error for negative numbers (Jithil P Ponnan) [#&#8203;49855](nodejs/node#49855)
-   \[[`7552de6806`](nodejs/node@7552de6806)] - **module**: fix the leak in SourceTextModule and ContextifySript (Joyee Cheung) [#&#8203;48510](nodejs/node#48510)
-   \[[`2e05cf1c60`](nodejs/node@2e05cf1c60)] - **module**: fix leak of vm.SyntheticModule (Joyee Cheung) [#&#8203;48510](nodejs/node#48510)
-   \[[`a86a2e14a3`](nodejs/node@a86a2e14a3)] - **module**: use symbol in WeakMap to manage host defined options (Joyee Cheung) [#&#8203;48510](nodejs/node#48510)
-   \[[`32906ddcac`](nodejs/node@32906ddcac)] - **node-api**: segregate nogc APIs from rest via type system (Gabriel Schulhof) [#&#8203;50060](nodejs/node#50060)
-   \[[`1aa71c26ff`](nodejs/node@1aa71c26ff)] - **node-api**: factor out common code into macros (Gabriel Schulhof) [#&#8203;50664](nodejs/node#50664)
-   \[[`3d0b233f52`](nodejs/node@3d0b233f52)] - **node-api**: introduce experimental feature flags (Gabriel Schulhof) [#&#8203;50991](nodejs/node#50991)
-   \[[`96514a8b9f`](nodejs/node@96514a8b9f)] - **src**: iterate on import attributes array correctly (Michaël Zasso) [#&#8203;50703](nodejs/node#50703)
-   \[[`2c2892bf88`](nodejs/node@2c2892bf88)] - **src**: set ModuleWrap internal fields only once (Joyee Cheung) [#&#8203;49391](nodejs/node#49391)
-   \[[`ff334cb774`](nodejs/node@ff334cb774)] - **src**: cast v8::Object::GetInternalField() return value to v8::Value (Joyee Cheung) [#&#8203;48943](nodejs/node#48943)
-   \[[`270b519971`](nodejs/node@270b519971)] - **stream**: do not defer construction by one microtick (Matteo Collina) [#&#8203;52005](nodejs/node#52005)
-   \[[`95d7a75084`](nodejs/node@95d7a75084)] - **test**: fix dns test case failures after c-ares update to 1.21.0+ (Brad House) [#&#8203;50743](nodejs/node#50743)
-   \[[`cd613e5167`](nodejs/node@cd613e5167)] - **test**: handle relative https redirect (Richard Lau) [#&#8203;51121](nodejs/node#51121)
-   \[[`40f10eafcf`](nodejs/node@40f10eafcf)] - **test**: fix `internet/test-inspector-help-page` (Richard Lau) [#&#8203;51693](nodejs/node#51693)
-   \[[`5e426511b1`](nodejs/node@5e426511b1)] - **test**: deflake test-vm-contextified-script-leak (Joyee Cheung) [#&#8203;49710](nodejs/node#49710)
-   \[[`0b156c6d28`](nodejs/node@0b156c6d28)] - **test**: use checkIfCollectable in vm leak tests (Joyee Cheung) [#&#8203;49671](nodejs/node#49671)
-   \[[`1586c11b3c`](nodejs/node@1586c11b3c)] - **test**: add checkIfCollectable to test/common/gc.js (Joyee Cheung) [#&#8203;49671](nodejs/node#49671)
-   \[[`902d8b3d4b`](nodejs/node@902d8b3d4b)] - **test**: fix flaky http-chunk-extensions-limit test (Ethan Arrowood) [#&#8203;51943](nodejs/node#51943)
-   \[[`1743d2bdc1`](nodejs/node@1743d2bdc1)] - **test**: test surrogate pair filenames on windows (Mert Can Altın) [#&#8203;51800](nodejs/node#51800)
-   \[[`1c1a7ec22d`](nodejs/node@1c1a7ec22d)] - **test**: increase platform timeout zlib-brotli-16gb (Rafael Gonzaga) [#&#8203;51792](nodejs/node#51792)
-   \[[`931d02fe3e`](nodejs/node@931d02fe3e)] - **test, v8**: fix wrong import attributes test (Nicolò Ribaudo) [#&#8203;52184](nodejs/node#52184)
-   \[[`d9ea6c1f8d`](nodejs/node@d9ea6c1f8d)] - **tls**: fix order of setting cipher before setting cert and key (Kumar Rishav) [#&#8203;50186](nodejs/node#50186)
-   \[[`3184befa2e`](nodejs/node@3184befa2e)] - **tools**: fix update-icu.sh (Michaël Zasso) [#&#8203;51723](nodejs/node#51723)
-   \[[`06646e11be`](nodejs/node@06646e11be)] - **(SEMVER-MINOR)** **vm**: use import attributes instead of import assertions (Antoine du Hamel) [#&#8203;50141](nodejs/node#50141)
-   \[[`fe66e9d06e`](nodejs/node@fe66e9d06e)] - **vm**: reject in importModuleDynamically without --experimental-vm-modules (Joyee Cheung) [#&#8203;50137](nodejs/node#50137)
-   \[[`052e095c6b`](nodejs/node@052e095c6b)] - **vm**: use internal versions of compileFunction and Script (Joyee Cheung) [#&#8203;50137](nodejs/node#50137)
-   \[[`9f7899ed0a`](nodejs/node@9f7899ed0a)] - **vm**: unify host-defined option generation in vm.compileFunction (Joyee Cheung) [#&#8203;50137](nodejs/node#50137)
-   \[[`6291c107d0`](nodejs/node@6291c107d0)] - **vm**: use default HDO when importModuleDynamically is not set (Joyee Cheung) [#&#8203;49950](nodejs/node#49950)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIwLjAuMC1zZW1hbnRpYy1yZWxlYXNlIiwidXBkYXRlZEluVmVyIjoiMC4wLjAtc2VtYW50aWMtcmVsZWFzZSIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Reviewed-on: https://harton.dev/cinder/cinder-space/pulls/25
Co-authored-by: Renovate Bot <[email protected]>
Co-committed-by: Renovate Bot <[email protected]>
TiKevin83 added a commit to TiKevin83/docker-android that referenced this pull request Jun 12, 2024
Updating to Node 18.20 will fix a memory leak regression that has been affecting Jest for a while, see here for references:

jestjs/jest#11956

nodejs/node#51004
TiKevin83 added a commit to TiKevin83/docker-android that referenced this pull request Jun 19, 2024
Updating to Node 18.20 will fix a memory leak regression that has been affecting Jest for a while, see here for references:

jestjs/jest#11956

nodejs/node#51004
cortinico pushed a commit to react-native-community/docker-android that referenced this pull request Jun 19, 2024
Updating to Node 18.20 will fix a memory leak regression that has been affecting Jest for a while, see here for references:

jestjs/jest#11956

nodejs/node#51004
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants