Release `Buffer` and `ArrayBuffer` instances that were created through
our addon APIs and have finalizers attached to them only after V8 has
called the deleter callback passed to the `BackingStore`, instead of
relying on our own GC callback(s).
This fixes the following race condition:
1. Addon code allocates pointer P via `malloc`.
2. P is passed into `napi_create_external_buffer` with a finalization
callback which calls `free(P)`. P is inserted into V8’s global array
buffer table for tracking.
3. The finalization callback is executed on GC. P is freed and returned
to the allocator. P is not yet removed from V8’s global array
buffer table. (!)
4. Addon code attempts to allocate memory once again. The allocator
returns P, as it is now available.
5. P is passed into `napi_create_external_buffer`. P still has not been
removed from the v8 global array buffer table.
6. The world ends with `Check failed: result.second`.
Since our API contract is to call the finalizer on the JS thread on
which the `ArrayBuffer` was created, but V8 may call the `BackingStore`
deleter callback on another thread, fixing this requires posting
a task back to the JS thread.
Refs: https://github.com/nodejs/node/issues/32463#issuecomment-625877175
Fixes: https://github.com/nodejs/node/issues/32463
PR-URL: https://github.com/nodejs/node/pull/33321
Reviewed-By: James M Snell <jasnell@gmail.com>
In some situations, it can be useful to use threadsafe callbacks
on an `Environment` to perform cleanup operations that should run
even when the process would otherwise be ending.
PR-URL: https://github.com/nodejs/node/pull/33320
Reviewed-By: James M Snell <jasnell@gmail.com>
This is a manually edited and outdated list of builtin modules.
Instead, it is better to rely upon the officially documented way
to get a list of builtin modules.
As a side by fix this makes sure all exports are in one place. Thus,
it is easier to see what parts are actually exported and which are
not.
Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: https://github.com/nodejs/node/pull/33294
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Add regression tests for the case in which an async_hook is enabled
inside a Worker thread and `process.exit()` is called during the
async part of an async function.
This commit includes multiple tests that seem like they should all
crash in a similar way, but interestingly don’t. In particular, it’s
surprising that the presence of a statement after `process.exit()`
in a function has an effect on the kind of crash that’s being
exhibited (V8 DCHECK vs. assertion in our own code) and the
circumstances under which it crashes (e.g. the -1 and -2 tests
can be “fixed” by reverting 13c5a1629c, although they
should have the same behavior as the -3 and -4 tests).
PR-URL: https://github.com/nodejs/node/pull/33347
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Termination exceptions are similar to uncaught exceptions in that they
should clear the async id stack, because no ongoing async callbacks
will be brought to completion when execution terminates.
Previously, there was a check that made sure that that happened when
the termination occurred during the callback itself, but no such check
was in place for the case that the termination occurred during
microtasks started by them. This commit adds such a check, both for
microtasks and the `nextTick` queue. The latter addition doesn’t fix
a crash, but still makes sense conceptually.
The condition here is also flipped from applying only on Worker threads
to also applying on the main thread, and setting the `failed_` flag
rather than reading it. The former makes sense because the public C++
`Stop(env)` API can have the same effect as worker thread termination,
but on the main thread rather than a Worker thread.
PR-URL: https://github.com/nodejs/node/pull/33347
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
libuv does not expect concurrent operations on `uv_dir_t` instances,
and will gladly create memory leaks, corrupt data, or crash the
process.
This patch forbids that, and:
- Makes sure that concurrent async operations are run sequentially
- Throws an exception if sync operations are attempted during an
async operation
The assumption here is that a thrown exception is preferable to
a potential hard crash.
This fully fixes flakiness from `parallel/test-fs-opendir` when
run under ASAN.
PR-URL: https://github.com/nodejs/node/pull/33274
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
A call into JS can schedule another operation on the same `uv_dir_t`.
In particular, when the handle is closed from the callback for a
directory read operation, there previously was a race condition window:
1. A `dir.read()` operation is submitted to libuv
2. The read operation is finished by libuv, calling `AfterDirRead()`
3. We call into JS
4. JS calls dir.close()
5. libuv posts the close request to a thread in the pool
6. The close request runs, destroying the directory handle
7. `AfterDirRead()` is being exited.
Exiting the `FSReqAfterScope` in step 7 attempts to destroy the original
uv_fs_t` from step 1, which now points to an `uv_dir_t` that has
already been destroyed in step 5.
By forcing the `FSReqAfterScope` to clean up before we call into JS,
we can be sure that no other operations on the same `uv_dir_t` are
submitted concurrently.
This addresses issues observed when running with ASAN/valgrind.
PR-URL: https://github.com/nodejs/node/pull/33274
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This improves the autocompletion for require calls. It had multiple
small issues so far. Most important: it won't suggest completions for
require statements that are fully written out. Second, it'll detect
require calls that have whitespace behind the opening bracket. Third,
it makes sure node modules are detected as such instead of only
suggesting them as folders. Last, it adds suggestions for input that
starts with backticks.
Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: https://github.com/nodejs/node/pull/33282
Fixes: https://github.com/nodejs/node/issues/33238
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
This replaces the internally used hard coded Node.js core module
list with the actual internal existent modules. That way all modules
are automatically picked up instead of having to update the list
manually.
This currently only applies to the REPL and to the Node.js `eval`
functionality (User passed `-e` or `--eval` arguments to Node without
`-i` or `--interactive`).
Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: https://github.com/nodejs/node/pull/33282
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
This aligns the behavior with the one in the Firefox console.
It will visualize ReferenceErrors in case the input has no possible
completion and no buffered input. That way typos can already be
highlighted before being evaluated.
Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: https://github.com/nodejs/node/pull/33282
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
This aligns the REPL preview with the one used in the Chrome
DevTools console. It will now preview the output for the input
including the completion suffix as input. When pressing enter while
previewing such data, it will automatically insert the suffix
before evaluating the input. When pressing escape, that behavior
is deactivated until the input is changed.
Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: https://github.com/nodejs/node/pull/33282
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
This is separated out of the QUIC PR. It is not specific to QUIC but
provides a new base class that is used there as an abstraction of
the actual implementation. This is a purely internal implementation
detail that has no outward functional changes (so no need for tests)
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/33289
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
There's a typo that causes only the first socket to be logged
(i.e. when the warning is emitted).
In addition, server sockets aren't logged because `keylog` events
are not emitted on tls.Server, not the socket. This behaviour is
counterintuitive and has caused more bugs in the past, so make all
sockets (server or client) emit 'keylog'. tls.Server will just
re-emit these events.
Refs: https://github.com/nodejs/node/pull/30055
PR-URL: https://github.com/nodejs/node/pull/33366
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
- Print test results as soon as they are available, instead of
until after all the tests are complete. This helps us printing
tests whose completion callback is not called because of
failures.
- Run the scripts specified by `// META: script=` one by one
instead of concatenating them first for better error stack
traces.
- Print a status summary when the test process is about to exit.
This can be used as reference for updating the status file.
For example the stderr output of
`out/Release/node test/wpt/test-console.js` would be:
```
{
'idlharness.any.js': {
fail: {
expected: [
'assert_equals: operation has wrong .length expected 1 but got 0'
]
}
}
}
Ran 4/4 tests, 0 skipped, 3 passed, 1 expected failures, 0 unexpected failures
```
PR-URL: https://github.com/nodejs/node/pull/33297
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
This commit updates the interfaces to
https://github.com/web-platform-tests/wpt/tree/8ada332aea/interfaces
and updates the hr-time test status:
- `window-worker-timeOrigin.window.js` should be skipped because we
don't implement `Blob`
- `idlharness.any.js` should be skipped since the IDL parser needs
to be updated, but the parser update would also result in
an update of the test harness which in turn requires updates of
other tests. We need to fix the URL implementation first,
and then update the harness and all the tests.
PR-URL: https://github.com/nodejs/node/pull/33297
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Rich Trott