Repository navigation
build: remove six obsolete Node.js patches - #53524
Merged
Merged
Conversation
Pass --no-js-source-phase-imports from NodeBindings and node_main instead of patching Node's default, and drop five patches whose upstream reason is gone (common.gypi defaults, test-runner env clobbering, libc++ errno deprecation, cppgc double-init guard, TLS external-memory accounting now fixed upstream).
codebytere
added
target/44-x-y
…-as-node fork() propagates execArgv, so the inserted flag accumulated in children (parallel/test-child-process-fork-exec-argv).
node_mksnapshot runs Node's argument processing, which now turns the V8 flag on by default; the builtins code cache it embeds is keyed on V8's flags hash and was rejected in the browser process, where Electron keeps the flag off (spec/api-js2c-code-cache-spec.ts).
…node-patches # Conflicts: # patches/node/build_perfetto-sdk.patch # patches/node/build_restore_clang_as_default_compiler_on_macos.patch # patches/node/build_restore_macos_deployment_target_to_13_0.patch # patches/node/v8_propagate_v8_cppgc_microtask_queue_flag_to_common_gypi.patch
MarshallOfSound
approved these changes
Sep 4, 2026
MarshallOfSound
removed
the
target/44-x-y
It failed on the external-memory DCHECK the dropped TLS patch caused.
codebytere
changed the title
codebytere
enabled auto-merge (squash)
September 4, 2026 20:28
|
No Release Notes |
Contributor
|
I have automatically backported this PR to "45-x-y", please check out #53549 |
trop
Bot
added
in-flight/45-x-y
merged/45-x-y
MarshallOfSound
added a commit
that referenced
this pull request
Oct 15, 2026
Upstream moved the SecureContext external-memory Increase() into Init() and guards the Decrease() on ctx_, so the leak this patch fixed is gone. The patch's removal of that guard now double-decrements when v24.22.0's TLSSocket closes a single-use context before the wrapper is destroyed, tripping V8's ExternalMemoryAccounter DCHECK in every TLS test. Already dropped on main in #53524.
MarshallOfSound
added a commit
that referenced
this pull request
Oct 15, 2026
Upstream moved the SecureContext external-memory Increase() into Init() and guards the Decrease() on ctx_, so the leak this patch fixed is gone. The patch's removal of that guard now double-decrements when v24.22.0's TLSSocket closes a single-use context before the wrapper is destroyed, tripping V8's ExternalMemoryAccounter DCHECK in every TLS test. Already dropped on main in #53524.
MarshallOfSound
added a commit
that referenced
this pull request
Oct 15, 2026
* chore: bump node in DEPS to v24.22.0 * fix(patch): enable the V8 sandbox in shared-cage builds Upstream now enables the V8 sandbox only for shared-cage builds. Keep Electron's behaviour of enabling it whenever pointer compression is on, matching Chromium's V8. The unofficial.gni x86 hunk landed upstream. Ref: nodejs/node#62237 Ref: nodejs/node#65491 * fix(patch): use names for asymmetric key algorithms Upstream dropped the BoringSSL guard in DhKeyGenTraits::Setup(), removed ReadASN1Element() from ncrypto, and no longer includes <openssl/evp.h> from env.h, so those hunks are no longer needed. Ref: nodejs/node#65966 Ref: nodejs/node#66108 Ref: nodejs/node#65484 * fix(patch): skip RSA-PSS keygen deprecation tests with BoringSSL The skip is now upstream. Ref: nodejs/node#65808 * fix(patch): detach cppgc wrappers from their Realm before it is freed Upstream's CppgcWrapperList::Cleanup() now pops nodes itself and the wrapper owns and frees its list node, so the local PopFront/delete hunk is dropped. Ref: nodejs/node#65778 * fix(patch): gate jspi flag on V8_ENABLE_WEBASSEMBLY Ref: nodejs/node#66082 * chore: remove upstreamed patches - nodejs/node#63985 - nodejs/node#65630 - nodejs/node#64453 - nodejs/node#65778 * chore: update patches (trivial only) * chore: update filenames.auto.gni * fix(patch): discover hashes from OpenSSL providers ncrypto's DigestCache::aliases() now has a function-local static, so ncrypto needs the no_exit_time_destructors config too. Ref: nodejs/node#65484 * fix(patch): read the peer certificate chain without consuming it sk_X509_num() returns size_t in BoringSSL. Ref: nodejs/node#65602 * fix(patch): fix shared buffer growability validation Ref: nodejs/node#65845 * chore: update patches (trivial only) * node#65820: src: fix null pointer call when running without a startup snapshot SnapshotData::AsEmbedderWrapper() was removed. Both call sites wrap the embedded snapshot, so use EmbedderSnapshotData::BuiltinSnapshotData(). Ref: nodejs/node#65820 * node#65407: src: apply IsolateSettings when using a snapshot node::CreateEnvironment() no longer installs the per-isolate message listener when it builds the environment from a snapshot, so stop compensating for that and let SetIsolateUpForNode() add it. Ref: nodejs/node#65407 * test: drop removed wasm rab integration flag from webidl buffer source test Chromium's V8 removed --experimental-wasm-rab-integration after shipping it; the new test-internal-webidl-buffer-source.js passes it. Ref: nodejs/node#65845 Ref: https://chromium-review.googlesource.com/c/v8/v8/+/7708297 * chore: remove obsolete TLS external-memory accounting patch Upstream moved the SecureContext external-memory Increase() into Init() and guards the Decrease() on ctx_, so the leak this patch fixed is gone. The patch's removal of that guard now double-decrements when v24.22.0's TLSSocket closes a single-use context before the wrapper is destroyed, tripping V8's ExternalMemoryAccounter DCHECK in every TLS test. Already dropped on main in #53524. * test: skip new rmSync permission error test when the Node suite runs as root test-fs-rmSync-special-char-additional-error.js is new in v24.22.0 and relies on a read-only directory blocking removal, which root bypasses; the Linux Node.js test job runs as root. Ref: nodejs/node#64453 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Sam Attard <[email protected]>
MarshallOfSound
added a commit
that referenced
this pull request
Oct 15, 2026
* chore: bump node in DEPS to v24.22.0 * fix(patch): enable the V8 sandbox in shared-cage builds Upstream now enables the V8 sandbox only for shared-cage builds. Keep Electron's behaviour of enabling it whenever pointer compression is on, matching Chromium's V8. The unofficial.gni x86 hunk landed upstream. Ref: nodejs/node#62237 Ref: nodejs/node#65491 * fix(patch): use names for asymmetric key algorithms Upstream dropped the BoringSSL guard in DhKeyGenTraits::Setup(), removed ReadASN1Element() from ncrypto, and no longer includes <openssl/evp.h> from env.h, so those hunks are no longer needed. Ref: nodejs/node#65966 Ref: nodejs/node#66108 Ref: nodejs/node#65484 * fix(patch): skip RSA-PSS keygen deprecation tests with BoringSSL The skip is now upstream. Ref: nodejs/node#65808 * fix(patch): detach cppgc wrappers from their Realm before it is freed Upstream's CppgcWrapperList::Cleanup() now pops nodes itself and the wrapper owns and frees its list node, so the local PopFront/delete hunk is dropped. Ref: nodejs/node#65778 * fix(patch): gate jspi flag on V8_ENABLE_WEBASSEMBLY Ref: nodejs/node#66082 * chore: remove upstreamed patches - nodejs/node#63985 - nodejs/node#65630 - nodejs/node#64453 - nodejs/node#65778 * chore: update patches (trivial only) * chore: update filenames.auto.gni * fix(patch): discover hashes from OpenSSL providers ncrypto's DigestCache::aliases() now has a function-local static, so ncrypto needs the no_exit_time_destructors config too. Ref: nodejs/node#65484 * fix(patch): read the peer certificate chain without consuming it sk_X509_num() returns size_t in BoringSSL. Ref: nodejs/node#65602 * chore: update patches (trivial only) * node#65820: src: fix null pointer call when running without a startup snapshot SnapshotData::AsEmbedderWrapper() was removed. Both call sites wrap the embedded snapshot, so use EmbedderSnapshotData::BuiltinSnapshotData(). Ref: nodejs/node#65820 * node#65407: src: apply IsolateSettings when using a snapshot node::CreateEnvironment() no longer installs the per-isolate message listener when it builds the environment from a snapshot, so stop compensating for that and let SetIsolateUpForNode() add it. Ref: nodejs/node#65407 * test: drop removed wasm rab integration flag from webidl buffer source test Chromium's V8 removed --experimental-wasm-rab-integration after shipping it; the new test-internal-webidl-buffer-source.js passes it. Ref: nodejs/node#65845 Ref: https://chromium-review.googlesource.com/c/v8/v8/+/7708297 * chore: remove obsolete TLS external-memory accounting patch Upstream moved the SecureContext external-memory Increase() into Init() and guards the Decrease() on ctx_, so the leak this patch fixed is gone. The patch's removal of that guard now double-decrements when v24.22.0's TLSSocket closes a single-use context before the wrapper is destroyed, tripping V8's ExternalMemoryAccounter DCHECK in every TLS test. Already dropped on main in #53524. * test: skip new rmSync permission error test when the Node suite runs as root test-fs-rmSync-special-char-additional-error.js is new in v24.22.0 and relies on a read-only directory blocking removal, which root bypasses; the Linux Node.js test job runs as root. Ref: nodejs/node#64453 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Sam Attard <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description of Change
Removes six Node.js patches whose reason for existing has gone away on v24.20.0. The one behavior any of them still carried - keeping V8's source phase imports off - now comes from Electron passing
--no-js-source-phase-importsto Node rather than from editing Node's default.fix_add_default_values_for_variables_in_common_gypi-llvm_versionandbuild_v8_with_gnare no longer read anywhere incommon.gypi, andenable_ltocomes from theconfig.gypiElectron ships (node.h already#errors without it).ci_ensure_node_tests_set_electron_run_as_node- upstream stopped clobberingenvin the test-runner fixtures this patched (nodejs/node@cb08703c804), so the rebased patch was re-adding the clobbering it was written to undo.fix_remove_deprecated_errno_constants- libc++ reverted the deprecation of theENODATA/ENOSR/ENOSTR/ETIMEmacros ([libc++] Undeprecate POSIX STREAM macros. llvm/llvm-project#88296); only thestd::errcenumerators stay deprecated, which Node and libuv don't use.os.constants.errnoanduvget the four entries back.fix_cppgc_initializing_twice-NodeBindingspasseskNoInitializeCppgc, and innode_main.ccNode's process init runs beforeV8::Initialize(), which itself skipscppgc::InitializeProcess()when already initialized. The added!cppgc::IsInitialized()guard could not change the outcome on either path.feat_disable_js_source_phase_imports_by_default- src: allow disabling JS source phase imports nodejs/node#60364 is in v24.20.0, so Node honors--no-js-source-phase-imports;NodeBindings::ParseNodeCliFlags()andnode_main.ccnow pass it (kept out ofprocess.execArgvin run-as-node sofork()doesn't accumulate it), andnode_mksnapshotis run with it so the builtins code cache it embeds keeps the same V8 flags hash as the browser isolate. Renderers need it because Blink sets that V8 flag before Node runs and V8 flags are frozen by then.fix_avoid_external_memory_leak_on_invalid_tls_protocol_versions- fixed upstream by tls: fix leak on invalid protocol method nodejs/node#60427 (v24.12.0), which guards theDecrease()inSecureContext::Reset()onctx_. Our patch removed that guard on rebase, so atls.createSecureContext()that throws inInit()decremented 1024 bytes of external memory that were never added: aamount_of_external_memory_ >= sizeDCHECK in testing builds (parallel/test-tls-no-sslv23, re-enabled here) and a skewed counter in release.Checklist
npm testpassesRelease Notes
Notes: none