GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Fix a segfault by ensuring TLS Sockets are closed if the underlying stream closes by pimterry · Pull Request #49327 · nodejs/node

Conversation

pimterry commented Aug 25, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

This fixes a potential HTTP/2 segfault (#49307, #48567) caused by underlying TLS behaviour, and likely fixes various other apparently related issues (#46094, #35695).

I found this while investigating #48519, which this doesn't directly fix, but definitely relates to, as the JS Stream Sockets there have all sorts of issues directly caused by TLS interactions after the JSS is closed.

The underlying problem here is that TLS sockets which are created by passing an existing socket (either tlsServer.emit('connection', socket) or tls.createConnection({ ..., socket })) do not listen to whether the underlying socket was closed elsewhere. This is easy to demo:

const tlsOptions = {
  key: // Any key, e.g. fixtures.readKey('agent1-key.pem'),
  cert: // Any cert, e.g. fixtures.readKey('agent1-cert.pem')
};

const net = require('net');
const tls = require('tls');

const server = tls.createServer(tlsOptions, () => {
    console.log('Got TLS connection');
});

server.listen(0, () => {
    const socket = net.createConnection({
        port: server.address().port
    });

    socket.on('connect', () => {
        const tlsSocket = tls.connect({
            socket,
            rejectUnauthorized: false
        });

        tlsSocket.on('secureConnect', () => {
            console.log('TLS client connected');

            setImmediate(() => {
                socket.destroy();

                setTimeout(() => {
                    console.log('Is the socket alive?', !socket.destroyed);
                    console.log('Is TLS readable & writable?', tlsSocket.writable, tlsSocket.readable);
                }, 100);
            });
        });
    });
});

This prints:

TLS client connected
Got TLS connection
Is the socket alive? false
Is TLS readable & writable? true true

With this change, the last line is instead false false.

Clearly the TLS socket should not be readable or writeable 100ms after it's definitely disconnected. This happens even if something is reading data - the data just stops, and the affected TLS socket (the client in this case) never closes.

These TLSSockets do seem to shut down correctly if the remote side closes the connection, but in any other scenario, the TLS connection will remain happily active like this even when the underlying socket is gone, until something more serious interacts with its internals, which then explode (either segfaults in normal cases, or the various finishWrite errors in the issues above in JS Stream Socket cases).

The first segfault is definitely caused by this case, and I've added a new test for that.

I'm fairly confident this is the underlying issue for the other cases too - note that both cases use the manual socket handling pattern (with HTTP/2, but implicitly on top of TLS) - but neither has a reliable repro to confirm that.

This change notably significantly modifies an existing TLS test too. Unfortunately with this change in place, that test no longer works at all, as it tries to mess with internals to trigger a race condition that crashed in some Node v7 versions, and those internals are now cleaned up before they can be messed with. This test has been simplified to a more general TLS shutdown test instead.

There is some history to this - very similar code did used to exist!

It was added here
It was then reverted here on the basis (discussed here) that it was now unnecessary, due to other changes.
That caused issues, which were fixed for the StreamWrap case only here
That was reverted as an unnecessary special case here

@addaleax and @ronag will likely be interested in this, since they were involved at various stages of that.

I think there's a good case here that something explicitly handling this is necessary. Somehow, TLS sockets need to be informed if the underlying socket is closed. Note that this change as here is not specific to StreamWraps, unless some past changes. AFAICT this is required here for all manually constructed TLS cases - the new test here just passes a net.Socket directly and it still crashes on all current Node versions.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ywave620 reacted with thumbs up emoji

bricss reacted with eyes emoji

All reactions

👍 1 reaction
👀 1 reaction

nodejs-github-bot added needs-ci

PRs that need a full CI run.

tls Issues and PRs related to the tls subsystem. labels

Aug 25, 2023


          tls: ensure TLS Sockets are closed if the underlying wrap closes

06fc079

This fixes a potential segfault, among various other likely-related
issues, which all occur because TLSSockets were not informed if their
underlying stream was closed in many cases.

This also significantly modifies an existing TLS test. With this change
in place, that test no longer works, as it tries to mess with internals
to trigger a race, and those internals are now cleaned up earlier. This
test has been simplified to a more general TLS shutdown test.

pimterry force-pushed the fix-tls-shutdown branch from e068064 to 06fc079 Compare

August 25, 2023 16:20

aduh95 commented Aug 26, 2023

Copy link

Copy Markdown

Contributor

parallel/test-http2-socket-close is failing on ASan and macOS CIs.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Tweak new H2 shutdown test towards reliable ordering & client shutdown

87ceb7e

Currently the test fails only in a couple of environments (Mac & ASan)
reliably, even though it passes reliably in all others. The failures are
due to emitted errors on the client H2 stream (not the focus of this
test). I'm hoping here that that's due to a timing different in those
environments, and scheduling the shutdown like this instead will
guarantee the order so that doesn't happen any more.

pimterry commented Aug 28, 2023

Copy link

Copy Markdown

Member Author

parallel/test-http2-socket-close is failing on ASan and macOS CIs.

Thanks @aduh95! It seems this fails because the new segfault test throws a connection reset error on the client H2 stream.

That's not a huge problem: it's the server that segfaults without this change, so the test is really interested in server behaviour here, not the client, and a handleable connection error event is far better than a segfault anyway. That said, it's weird that it's different for Mac & ASAN compared to the other cases, I wonder if it's just a timing issue?

I've pushed a change to the tests, to see whether it's a timing problem, which moves the shutdown logic into timers started after the response is definitely fully received. Hopefully that will ensure the client session handles this more cleanly. The test still segfaults on Node 20.5.1 and passes reliably with this change (on my Linux machine).

If this test still doesn't pass in the same cases, I think just ignoring client errors for this case is fine - it's an unusual shutdown regardless, and really we just want to check the server doesn't crash, so I don't want to get distracted too much by independent client issues (even if they might be worth investigating elsewhere later).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

pimterry commented Aug 28, 2023

Copy link

Copy Markdown

Member Author

I've pushed a change to the tests, to see whether it's a timing problem, which moves the shutdown logic into timers started after the response is definitely fully received.

Seems it was indeed a timing issue - all tests are now passing everywhere 👍

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Aug 28, 2023

Uncaught exception in socket code: "Cannot read properties of null (reading 'finishWrite')" #46094

Closed

Fix JS Stream Socket finishShutdown crash #49400

Closed

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Aug 29, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Aug 29, 2023

nodejs-github-bot commented Aug 29, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mcollina approved these changes

Aug 29, 2023

View reviewed changes

mcollina left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

github-actions Bot mentioned this pull request

Aug 30, 2023

CI Reliability 2023-08-30 nodejs/reliability#650

Open

16 tasks

pimterry commented Aug 30, 2023

Copy link

Copy Markdown

Member Author

I can see two CI failures here:

win-v2019-arm64 which fails due to a 60 minute timeout cross-compiling to ARM64
node-test-commit-osx on osx 10.15 which fails with "write error: No space left on device"

Both seem very unlikely to be related to this change, so I'm going to assume this is all good and done, and those will presumably pass later on (maybe once that node's disk space is cleaned up?).

Thanks for the review @mcollina! I think this is ready to merge but let me know if there's anything else I should do to make that happen.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Aug 30, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mcollina added commit-queue

PRs queued for automated landing through the Commit Queue.

author ready PRs with CI started, the required approvals, and no outstanding review comments. and removed needs-ci PRs that need a full CI run. labels

Aug 30, 2023

nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels

Aug 30, 2023

nodejs-github-bot commented Aug 30, 2023

Copy link

Copy Markdown

Collaborator

Commit Queue failed

- Loading data for nodejs/node/pull/49327
✔  Done loading data for nodejs/node/pull/49327
----------------------------------- PR info ------------------------------------
Title      Fix a segfault by ensuring TLS Sockets are closed if the underlying stream closes (#49327)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     pimterry:fix-tls-shutdown -> nodejs:main
Labels     tls, author ready
Commits    2
 - tls: ensure TLS Sockets are closed if the underlying wrap closes
 - Tweak new H2 shutdown test towards reliable ordering & client shutdown
Committers 1
 - Tim Perry 
PR-URL: https://github.com/nodejs/node/pull/49327
Reviewed-By: Matteo Collina 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/49327
Reviewed-By: Matteo Collina 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 25 Aug 2023 16:11:30 GMT
   ✔  Approvals: 1
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/49327#pullrequestreview-1600235070
   ✘  This PR needs to wait 41 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-08-30T19:32:01Z: https://ci.nodejs.org/job/node-test-pull-request/53650/
- Querying data for job/node-test-pull-request/53650/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/6030922076

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot mentioned this pull request

Aug 31, 2023

CI Reliability 2023-08-31 nodejs/reliability#651

Open

18 tasks

mcollina added commit-queue

PRs queued for automated landing through the Commit Queue.

commit-queue-squash PRs the Commit Queue should land as one squashed commit. and removed commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. labels

Aug 31, 2023

nodejs-github-bot commented Aug 31, 2023

Copy link

Copy Markdown

Collaborator

Commit Queue failed

- Loading data for nodejs/node/pull/49327
✔  Done loading data for nodejs/node/pull/49327
----------------------------------- PR info ------------------------------------
Title      Fix a segfault by ensuring TLS Sockets are closed if the underlying stream closes (#49327)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     pimterry:fix-tls-shutdown -> nodejs:main
Labels     tls, author ready, commit-queue-squash
Commits    2
 - tls: ensure TLS Sockets are closed if the underlying wrap closes
 - Tweak new H2 shutdown test towards reliable ordering & client shutdown
Committers 1
 - Tim Perry 
PR-URL: https://github.com/nodejs/node/pull/49327
Reviewed-By: Matteo Collina 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/49327
Reviewed-By: Matteo Collina 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 25 Aug 2023 16:11:30 GMT
   ✔  Approvals: 1
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/49327#pullrequestreview-1600235070
   ✘  This PR needs to wait 27 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-08-30T23:03:30Z: https://ci.nodejs.org/job/node-test-pull-request/53650/
- Querying data for job/node-test-pull-request/53650/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/6037572193

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

debadree25 added the commit-queue PRs queued for automated landing through the Commit Queue. label

Sep 1, 2023

nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label

Sep 1, 2023

nodejs-github-bot merged commit 048e0be into nodejs:main

Sep 1, 2023

nodejs-github-bot commented Sep 1, 2023

Copy link

Copy Markdown

Collaborator

Landed in 048e0be

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

pimterry mentioned this pull request

Sep 1, 2023

Reproducible Segfault in HTTP2 module #48567

Closed

This was referenced Sep 2, 2023

CI Reliability 2023-09-02 nodejs/reliability#653

Open

CI Reliability 2023-09-03 nodejs/reliability#654

Open

lpinca commented Sep 9, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

@pimterry the parallel/test-tls-socket-close.js test is flaky now. An assertion fails

=== release test-tls-socket-close ===                   
Path: parallel/test-tls-socket-close
node:assert:125
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

false !== true

    at Immediate._onImmediate (/Users/luigi/code/node/test/parallel/test-tls-socket-close.js:47:16)
    at process.processImmediate (node:internal/timers:478:21) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: false,
  expected: true,
  operator: 'strictEqual'
}

Node.js v21.0.0-pre
Command: out/Release/node /Users/luigi/code/node/test/parallel/test-tls-socket-close.js

I can reproduce the issue on macOS by running

python3 tools/test.py -J --repeat=1000 test/parallel/test-tls-socket-close.js

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lpinca commented Sep 9, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

The following patch seems to fix the issue

diff --git a/test/parallel/test-tls-socket-close.js b/test/parallel/test-tls-socket-close.js
index 667b291309..70af760d53 100644
--- a/test/parallel/test-tls-socket-close.js
+++ b/test/parallel/test-tls-socket-close.js
@@ -44,9 +44,9 @@ function connectClient(server) {
 
       setImmediate(() => {
         assert.strictEqual(netSocket.destroyed, true);
-        assert.strictEqual(clientTlsSocket.destroyed, true);
 
         setImmediate(() => {
+          assert.strictEqual(clientTlsSocket.destroyed, true);
           assert.strictEqual(serverTlsSocket.destroyed, true);
 
           tlsServer.close();

but I'm not sure if it invalidates some assumptions.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

pimterry commented Sep 9, 2023

Copy link

Copy Markdown

Member Author

Ah, sorry about that. I think that patch absolutely makes sense - there's no specific reason that it should be exactly one setImmediate to get to the destroyed state on the remote socket, it just needs to settle there eventually.

I assume you're happy to PR that patch, but let me know if you'd prefer me to do it.

lpinca reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lpinca added a commit to lpinca/node that referenced this pull request

Sep 9, 2023


          test: deflake test-tls-socket-close

03a2d48

Move the check for the destroyed state of the remote socket to the inner
`setImmediate()`.

Refs: nodejs#49327 (comment)

lpinca mentioned this pull request

Sep 9, 2023

test: deflake test-tls-socket-close #49575

Merged

lpinca commented Sep 9, 2023

Copy link

Copy Markdown

Member

Done, thanks for the fast reply.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

UlisesGascon pushed a commit that referenced this pull request

Sep 10, 2023


          tls: ensure TLS Sockets are closed if the underlying wrap closes

8949cc7

This fixes a potential segfault, among various other likely-related
issues, which all occur because TLSSockets were not informed if their
underlying stream was closed in many cases.

This also significantly modifies an existing TLS test. With this change
in place, that test no longer works, as it tries to mess with internals
to trigger a race, and those internals are now cleaned up earlier. This
test has been simplified to a more general TLS shutdown test.

PR-URL: #49327
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>

UlisesGascon mentioned this pull request

Sep 10, 2023

v20.7.0 proposal #49592

Merged

nodejs-github-bot pushed a commit that referenced this pull request

Sep 13, 2023


          test: deflake test-tls-socket-close

ccf46ba

Move the check for the destroyed state of the remote socket to the inner
`setImmediate()`.

Refs: #49327 (comment)
PR-URL: #49575
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>

ruyadorno pushed a commit that referenced this pull request

Sep 28, 2023


          test: deflake test-tls-socket-close

185d9b5

Move the check for the destroyed state of the remote socket to the inner
`setImmediate()`.

Refs: #49327 (comment)
PR-URL: #49575
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>

codebytere added a commit to electron/electron that referenced this pull request

Nov 6, 2023


          tls: ensure TLS Sockets are closed if the underlying wrap closes

27c228d

nodejs/node#49327

codebytere added a commit to electron/electron that referenced this pull request

Nov 14, 2023


          tls: ensure TLS Sockets are closed if the underlying wrap closes

57e6248

nodejs/node#49327

codebytere added a commit to electron/electron that referenced this pull request

Nov 15, 2023


          tls: ensure TLS Sockets are closed if the underlying wrap closes

7e776ce

nodejs/node#49327

codebytere added a commit to electron/electron that referenced this pull request

Nov 16, 2023


          tls: ensure TLS Sockets are closed if the underlying wrap closes

5f881ea

nodejs/node#49327

codebytere added a commit to electron/electron that referenced this pull request

Nov 21, 2023


          tls: ensure TLS Sockets are closed if the underlying wrap closes

e645d89

nodejs/node#49327

codebytere added a commit to electron/electron that referenced this pull request

Nov 22, 2023


          tls: ensure TLS Sockets are closed if the underlying wrap closes

26fa4b2

nodejs/node#49327

codebytere added a commit to electron/electron that referenced this pull request

Nov 28, 2023


          tls: ensure TLS Sockets are closed if the underlying wrap closes

7a0c9b2

nodejs/node#49327

codebytere added a commit to electron/electron that referenced this pull request

Nov 29, 2023


          tls: ensure TLS Sockets are closed if the underlying wrap closes

a80becc

nodejs/node#49327

jkleinsc pushed a commit to electron/electron that referenced this pull request

Nov 30, 2023


          chore: upgrade to Node.js v20 (#40545)

9c94fd7

* chore: upgrade to Node.js v20

* src: allow embedders to override NODE_MODULE_VERSION

nodejs/node#49279

* src: fix missing trailing ,

nodejs/node#46909

* src,tools: initialize cppgc

nodejs/node#45704

* tools: allow passing absolute path of config.gypi in js2c

nodejs/node#49162

* tools: port js2c.py to C++

nodejs/node#46997

* doc,lib: disambiguate the old term, NativeModule

nodejs/node#45673

* chore: fixup Node.js BSSL tests

* nodejs/node#49492
* nodejs/node#44498

* deps: upgrade to libuv 1.45.0

nodejs/node#48078

* deps: update V8 to 10.7

nodejs/node#44741

* test: use gcUntil() in test-v8-serialize-leak

nodejs/node#49168

* module: make CJS load from ESM loader

nodejs/node#47999

* src: make BuiltinLoader threadsafe and non-global

nodejs/node#45942

* chore: address changes to CJS/ESM loading

* module: make CJS load from ESM loader (nodejs/node#47999)
* lib: improve esm resolve performance (nodejs/node#46652)

* bootstrap: optimize modules loaded in the built-in snapshot

nodejs/node#45849

* test: mark test-runner-output as flaky

nodejs/node#49854

* lib: lazy-load deps in modules/run_main.js

nodejs/node#45849

* url: use private properties for brand check

nodejs/node#46904

* test: refactor `test-node-output-errors`

nodejs/node#48992

* assert: deprecate callTracker

nodejs/node#47740

* src: cast v8::Object::GetInternalField() return value to v8::Value

nodejs/node#48943

* test: adapt test-v8-stats for V8 update

nodejs/node#45230

* tls: ensure TLS Sockets are closed if the underlying wrap closes

nodejs/node#49327

* test: deflake test-tls-socket-close

nodejs/node#49575

* net: fix crash due to simultaneous close/shutdown on JS Stream Sockets

nodejs/node#49400

* net: use asserts in JS Socket Stream to catch races in future

nodejs/node#49400

* lib: fix BroadcastChannel initialization location

nodejs/node#46864

* src: create BaseObject with node::Realm

nodejs/node#44348

* src: implement DataQueue and non-memory resident Blob

nodejs/node#45258

* sea: add support for V8 bytecode-only caching

nodejs/node#48191

* chore: fixup patch indices

* gyp: put filenames in variables

nodejs/node#46965

* build: modify js2c.py into GN executable

* fix: (WIP) handle string replacement of fs -> original-fs

* [v20.x] backport vm-related memory fixes

nodejs/node#49874

* src: make BuiltinLoader threadsafe and non-global

nodejs/node#45942

* src: avoid copying string in fs_permission

nodejs/node#47746

* look upon my works ye mighty

and dispair

* chore: patch cleanup

* [api] Remove AllCan Read/Write

https://chromium-review.googlesource.com/c/v8/v8/+/5006387

* fix: missing include for NODE_EXTERN

* chore: fixup patch indices

* fix: fail properly when js2c fails in Node.js

* build: fix js2c root_gen_dir

* fix: lib/fs.js -> lib/original-fs.js

* build: fix original-fs file xforms

* fixup! module: make CJS load from ESM loader

* build: get rid of CppHeap for now

* build: add patch to prevent extra fs lookup on esm load

* build: greatly simplify js2c modifications

Moves our original-fs modifications back into a super simple python script action, wires up the output of that action into our call to js2c

* chore: update to handle moved internal/modules/helpers file

* test: update @types/node test

* feat: enable preventing cppgc heap creation

* feat: optionally prevent calling V8::EnableWebAssemblyTrapHandler

* fix: no cppgc initialization in the renderer

* gyp: put filenames in variables

nodejs/node#46965

* test: disable single executable tests

* fix: nan tests failing on node headers missing file

* tls,http2: send fatal alert on ALPN mismatch

nodejs/node#44031

* test: disable snapshot tests

* nodejs/node#47887
* nodejs/node#49684
* nodejs/node#44193

* build: use deps/v8 for v8/tools

Node.js hard depends on these in their builtins

* test: fix edge snapshot stack traces

nodejs/node#49659

* build: remove js2c //base dep

* build: use electron_js2c_toolchain to build node_js2c

* fix: don't create SafeSet outside packageResolve

Fixes failure in parallel/test-require-delete-array-iterator:

=== release test-require-delete-array-iterator ===
Path: parallel/test-require-delete-array-iterator
node:internal/per_context/primordials:426
    constructor(i) { super(i); } // eslint-disable-line no-useless-constructor
                     ^

TypeError: object is not iterable (cannot read property Symbol(Symbol.iterator))
    at new Set (<anonymous>)
    at new SafeSet (node:internal/per_context/primordials:426:22)

* fix: failing crashReporter tests on Linux

These were failing because our change from node::InitializeNodeWithArgs to
node::InitializeOncePerProcess meant that we now inadvertently called
PlatformInit, which reset signal handling. This meant that our intentional
crash function ElectronBindings::Crash no longer worked and the renderer process
no longer crashed when process.crash() was called. We don't want to use Node.js'
default signal handling in the renderer process, so we disable it by passing
kNoDefaultSignalHandling to node::InitializeOncePerProcess.

* build: only create cppgc heap on non-32 bit platforms

* chore: clean up util:CompileAndCall

* src: fix compatility with upcoming V8 12.1 APIs

nodejs/node#50709

* fix: use thread_local BuiltinLoader

* chore: fixup v8 patch indices

---------

Co-authored-by: Keeley Hammond <[email protected]>
Co-authored-by: Samuel Attard <[email protected]>

MrHuangJser pushed a commit to MrHuangJser/electron that referenced this pull request

Dec 11, 2023


          chore: upgrade to Node.js v20 (electron#40545)

34651d1

* chore: upgrade to Node.js v20

* src: allow embedders to override NODE_MODULE_VERSION

nodejs/node#49279

* src: fix missing trailing ,

nodejs/node#46909

* src,tools: initialize cppgc

nodejs/node#45704

* tools: allow passing absolute path of config.gypi in js2c

nodejs/node#49162

* tools: port js2c.py to C++

nodejs/node#46997

* doc,lib: disambiguate the old term, NativeModule

nodejs/node#45673

* chore: fixup Node.js BSSL tests

* nodejs/node#49492
* nodejs/node#44498

* deps: upgrade to libuv 1.45.0

nodejs/node#48078

* deps: update V8 to 10.7

nodejs/node#44741

* test: use gcUntil() in test-v8-serialize-leak

nodejs/node#49168

* module: make CJS load from ESM loader

nodejs/node#47999

* src: make BuiltinLoader threadsafe and non-global

nodejs/node#45942

* chore: address changes to CJS/ESM loading

* module: make CJS load from ESM loader (nodejs/node#47999)
* lib: improve esm resolve performance (nodejs/node#46652)

* bootstrap: optimize modules loaded in the built-in snapshot

nodejs/node#45849

* test: mark test-runner-output as flaky

nodejs/node#49854

* lib: lazy-load deps in modules/run_main.js

nodejs/node#45849

* url: use private properties for brand check

nodejs/node#46904

* test: refactor `test-node-output-errors`

nodejs/node#48992

* assert: deprecate callTracker

nodejs/node#47740

* src: cast v8::Object::GetInternalField() return value to v8::Value

nodejs/node#48943

* test: adapt test-v8-stats for V8 update

nodejs/node#45230

* tls: ensure TLS Sockets are closed if the underlying wrap closes

nodejs/node#49327

* test: deflake test-tls-socket-close

nodejs/node#49575

* net: fix crash due to simultaneous close/shutdown on JS Stream Sockets

nodejs/node#49400

* net: use asserts in JS Socket Stream to catch races in future

nodejs/node#49400

* lib: fix BroadcastChannel initialization location

nodejs/node#46864

* src: create BaseObject with node::Realm

nodejs/node#44348

* src: implement DataQueue and non-memory resident Blob

nodejs/node#45258

* sea: add support for V8 bytecode-only caching

nodejs/node#48191

* chore: fixup patch indices

* gyp: put filenames in variables

nodejs/node#46965

* build: modify js2c.py into GN executable

* fix: (WIP) handle string replacement of fs -> original-fs

* [v20.x] backport vm-related memory fixes

nodejs/node#49874

* src: make BuiltinLoader threadsafe and non-global

nodejs/node#45942

* src: avoid copying string in fs_permission

nodejs/node#47746

* look upon my works ye mighty

and dispair

* chore: patch cleanup

* [api] Remove AllCan Read/Write

https://chromium-review.googlesource.com/c/v8/v8/+/5006387

* fix: missing include for NODE_EXTERN

* chore: fixup patch indices

* fix: fail properly when js2c fails in Node.js

* build: fix js2c root_gen_dir

* fix: lib/fs.js -> lib/original-fs.js

* build: fix original-fs file xforms

* fixup! module: make CJS load from ESM loader

* build: get rid of CppHeap for now

* build: add patch to prevent extra fs lookup on esm load

* build: greatly simplify js2c modifications

Moves our original-fs modifications back into a super simple python script action, wires up the output of that action into our call to js2c

* chore: update to handle moved internal/modules/helpers file

* test: update @types/node test

* feat: enable preventing cppgc heap creation

* feat: optionally prevent calling V8::EnableWebAssemblyTrapHandler

* fix: no cppgc initialization in the renderer

* gyp: put filenames in variables

nodejs/node#46965

* test: disable single executable tests

* fix: nan tests failing on node headers missing file

* tls,http2: send fatal alert on ALPN mismatch

nodejs/node#44031

* test: disable snapshot tests

* nodejs/node#47887
* nodejs/node#49684
* nodejs/node#44193

* build: use deps/v8 for v8/tools

Node.js hard depends on these in their builtins

* test: fix edge snapshot stack traces

nodejs/node#49659

* build: remove js2c //base dep

* build: use electron_js2c_toolchain to build node_js2c

* fix: don't create SafeSet outside packageResolve

Fixes failure in parallel/test-require-delete-array-iterator:

=== release test-require-delete-array-iterator ===
Path: parallel/test-require-delete-array-iterator
node:internal/per_context/primordials:426
    constructor(i) { super(i); } // eslint-disable-line no-useless-constructor
                     ^

TypeError: object is not iterable (cannot read property Symbol(Symbol.iterator))
    at new Set (<anonymous>)
    at new SafeSet (node:internal/per_context/primordials:426:22)

* fix: failing crashReporter tests on Linux

These were failing because our change from node::InitializeNodeWithArgs to
node::InitializeOncePerProcess meant that we now inadvertently called
PlatformInit, which reset signal handling. This meant that our intentional
crash function ElectronBindings::Crash no longer worked and the renderer process
no longer crashed when process.crash() was called. We don't want to use Node.js'
default signal handling in the renderer process, so we disable it by passing
kNoDefaultSignalHandling to node::InitializeOncePerProcess.

* build: only create cppgc heap on non-32 bit platforms

* chore: clean up util:CompileAndCall

* src: fix compatility with upcoming V8 12.1 APIs

nodejs/node#50709

* fix: use thread_local BuiltinLoader

* chore: fixup v8 patch indices

---------

Co-authored-by: Keeley Hammond <[email protected]>
Co-authored-by: Samuel Attard <[email protected]>

dglittle mentioned this pull request

Jan 7, 2026

Assertion failure crash in TLSWrap::DoWrite with zombie HTTP/2 session (close event not propagated from OS-level CLOSED socket) #61304

Closed

Reviewers

mcollina

mcollina approved these changes

+1 more reviewer

debadree25

debadree25 approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready

PRs with CI started, the required approvals, and no outstanding review comments.

commit-queue-squash

PRs the Commit Queue should land as one squashed commit.

tls

Issues and PRs related to the tls subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

pimterry commented Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 commented Aug 26, 2023

Uh oh!

pimterry commented Aug 28, 2023

Uh oh!

pimterry commented Aug 28, 2023

Uh oh!

nodejs-github-bot commented Aug 29, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

pimterry commented Aug 30, 2023

Uh oh!

nodejs-github-bot commented Aug 30, 2023

Uh oh!

nodejs-github-bot commented Aug 30, 2023

Uh oh!

nodejs-github-bot commented Aug 31, 2023

Uh oh!

nodejs-github-bot commented Sep 1, 2023

Uh oh!

lpinca commented Sep 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpinca commented Sep 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pimterry commented Sep 9, 2023

Uh oh!

lpinca commented Sep 9, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pimterry commented Aug 25, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

lpinca commented Sep 9, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

lpinca commented Sep 9, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.