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

Migrate SetAccessor uses to SetNativeDataProperty #163

Closed
wants to merge 2,850 commits into from

Conversation

verwaest
Copy link

@verwaest verwaest commented Nov 6, 2023

SetAccessor creates a deprecated type of "native data property", which has been replaced by SetNativeDataProperty. The old mechanism for installing such properties took in an AccessControl that's been deprecated (node used DEFAULT anyway).

LiviaMedeiros and others added 30 commits August 15, 2023 13:45
PR-URL: nodejs#49125
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
PR-URL: nodejs#49126
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Subsystems: blob, child_process, common, crypto, http, http2,
readline, repl, snapshot, trace_events

PR-URL: nodejs#49127
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
PR-URL: nodejs#49128
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
PR-URL: nodejs#49143
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Deokjin Kim <deokjin81.kim@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
We previously only return startup data for the first slot for
BaseObjects because we can already serialize all the necessary
information in one go, but slots that do not get special startup
data would be serialized verbatim which means that the pointer
addresses are going to be part of the snapshot blob, resulting
in indeterminism.

This patch updates the serialization routines and capture information
for both of the two slots - the first slot with type information
and memory management type (which we can use in the future for
cppgc-managed objects) and the second slot with data about the
object itself. This way the embeedder slots can be serialized
in a reproducible manner in the snapshot.

PR-URL: nodejs#48996
Refs: nodejs/build#3043
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Signed-off-by: Michal Biesek <michalbiesek@gmail.com>
PR-URL: nodejs#49106
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Deokjin Kim <deokjin81.kim@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Fix warning about dereferencing null env

Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: nodejs#48954
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
getDefaultEncoding() always returns 'buffer' in Node.js 20. It requires
some careful justification but the default encoding can be eliminated
from sig.js entirely.

In Sign.prototype.update, we can safely remove the conditional
assignment of getDefaultEncoding() to encoding. This is because
SignUpdate() in crypto_sig.cc internally calls node::crypto::Decode,
which returns UTF8 for falsy encoding values. In other words, with the
conditional assignment, StringBytes::Write() ultimately receives the
encoding BUFFER, and without the conditional assignment, it receives the
encoding UTF8. However, StringBytes::Write() treats both encodings
identically, so there is no need to deviate from the internal default
encoding UTF8.

In Sign.prototype.sign, we can also safely remove the conditional
assignment of getDefaultEncoding() to encoding. Whether encoding is
falsy or 'buffer' makes no difference.

In Verify.prototype.verify, we can also safely remove the conditional
assignment of getDefaultEncoding() to sigEncoding. This is because the
function passes the sigEncoding to getArrayBufferOrView(), which passes
it to Buffer.from(). If sigEncoding is 'buffer', getArrayBufferOrView()
instead passes 'utf8' to Buffer.from(). Because the default encoding of
Buffer.from() is 'utf8', passing a falsy encoding to
getArrayBufferOrView() instead of 'buffer' results in the same behavior.

Refs: nodejs#47182
PR-URL: nodejs#49145
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
This version avoids the additional access to the embedder slot
when we already have a reference to the realm.

PR-URL: nodejs#49007
Refs: nodejs#48836
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
This reduce the number of embedder slot accesses and also removes
the assumption in a few binding methods that the current realm is
the principal realm of the current environment (which is not true
for shadow realms).

PR-URL: nodejs#49007
Refs: nodejs#48836
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
These can be used to check the state and the output of a child
process launched with `spawnSync()`. They log additional information
about the child process when the check fails to facilitate debugging
test failures.

PR-URL: nodejs#49020
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
..and replace the similar code added for logging.

PR-URL: nodejs#49020
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
This (not particularly elegant) native addon tests the effect of
UV_THREADPOOL_SIZE on node-api. The test fails if Node.js allows more
than UV_THREADPOOL_SIZE async tasks to run concurrently, or if it limits
the number of concurrent async tasks to anything less than
UV_THREADPOOL_SIZE.

PR-URL: nodejs#49165
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
getDefaultEncoding() always returns 'buffer' in Node.js 20. It requires
some careful justification but the default encoding can be eliminated
from hash.js entirely. The reasoning is almost identical with that in
nodejs#49145 so I won't repeat it here.

Refs: nodejs#47182
PR-URL: nodejs#49167
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
getDefaultEncoding() always returns 'buffer' in Node.js 20.

In diffiehellman.js, this value is always used as input to either
toBuf(), encode(), or getArrayBufferOrView(). All of these functions
treat any falsy encoding just like 'buffer', so we can safely remove the
calls to getDefaultEncoding().

Refs: nodejs#47182
PR-URL: nodejs#49169
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Previously we assume that the objects are GC'ed after one
global.gc() returns, which is not necessarily always the case. Use
gcUntil() to run GC multiple times if they are not GC'ed in the
first time around.

PR-URL: nodejs#49053
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
The C++ implementation can now be done entirely in JS using WeakRef.
Re-implement it in JS instead to simplify the code.

PR-URL: nodejs#49053
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
PR-URL: nodejs#49053
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
PR-URL: nodejs#49199
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
PR-URL: nodejs#49019
Fixes: nodejs#48995
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Previously the ContextifyContext holds a MicrotaskQueueWrap which in
turns holds a MicrotaskQueue in a shared pointer. The indirection is
actually unnecessary, we can directly hold the MicrotaskQueue via
a unique pointer in ContextifyContext, the lifetime would still
remain the same but the graph would be simpler, and this removes
the additional JS -> C++ to create the wrapper object.

PR-URL: nodejs#48982
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Add `process.sourceMapsEnabled` to detect
whether source-maps are enabled.

Fixes: nodejs#46304
PR-URL: nodejs#46391
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
PR-URL: nodejs#48890
Refs: https://github.com/orgs/nodejs/discussions/44975
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
PR-URL: nodejs#49215
Fixes: nodejs#49049
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
V8 now no longer supports serializing code cache in an isolate
with unfinalized read-only space. So guard the code cache regeneration
with the `is_building_snapshot()` flag. When the isolate is created
for snapshot generation, the code cache is going to be serialized
separately anyway, so there is no need to do it in the builtin loader.

PR-URL: nodejs#49108
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Fixes: nodejs#48112
Ref: nodejs#48208
PR-URL: nodejs#49184
Refs: nodejs#48208
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
PR-URL: nodejs#49186
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Support for a single comma separates list for allow-fs-* flags is
removed. Instead now multiple flags can be passed to allow multiple
paths.

Fixes: nodejs/security-wg#1039
PR-URL: nodejs#49047
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Refs: nodejs#32930
PR-URL: nodejs#49180
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
bmeurer and others added 28 commits September 12, 2023 12:34
# Conflicts:
#	test/es-module/es-module.status
# Conflicts:
#	test/parallel/parallel.status
# Conflicts:
#	test/parallel/parallel.status
The CL https://crrev.com/c/3799431 adds a new function to the
console object so we need to temporarily disable a repl test
that checks for the concrete shape of the console object.

# Conflicts:
#	test/parallel/test-repl.js
Skip test-debugger-break and test-debugger-run-after-quit-restart to
unblock https://crrev.com/c/4290476
…#155)

The failing test is a regression test for nodejs#39717, which is about a crash that occurs when the flag is disabled. Given that that will no longer be possible once the flag is gone, this seems safe to disable forever.
Re-enable tests temporarily disabled
in a8952fc
Co-authored-by: Rezvan Mahdavi Hezaveh <rezvan@chromium.org>
V8 will increase the maximum TypedArray size in
https://crrev.com/c/4872536; this CL adapts the tests to allow for that.

Tests that hard-code smaller sizes or are already tested in V8 are
removed; one test is adapted to not rely on a specific limit.
@verwaest verwaest closed this Nov 6, 2023
@verwaest verwaest deleted the 2023-11-06_fix_name branch November 6, 2023 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet