Skip to content

test: check contextify contextual store behavior in strict mode#62571

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
Renegade334:test-vm-contextual-store
Apr 18, 2026
Merged

test: check contextify contextual store behavior in strict mode#62571
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
Renegade334:test-vm-contextual-store

Conversation

@Renegade334

Copy link
Copy Markdown
Member

Previously, this behaviour was tested indirectly by the REPL tests. However, REPL now uses DONT_CONTEXTIFY for its VM context, so the interceptor behaviour is no longer covered.

Refs: #61898 (comment)
Refs: #62371

Previously, this behavior was tested indirectly by the REPL tests.
However, REPL now uses DONT_CONTEXTIFY for its VM context.
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Apr 3, 2026
@Renegade334 Renegade334 requested review from legendecas and targos April 3, 2026 09:37
@Renegade334 Renegade334 added the vm Issues and PRs related to the vm subsystem. label Apr 3, 2026
@targos

targos commented Apr 3, 2026

Copy link
Copy Markdown
Member

Is this going to block the V8 update if it lands before it?

@codecov

codecov Bot commented Apr 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.72%. Comparing base (b411f90) to head (0913a15).
⚠️ Report is 122 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #62571      +/-   ##
==========================================
+ Coverage   89.71%   89.72%   +0.01%     
==========================================
  Files         695      695              
  Lines      214434   214434              
  Branches    41062    41067       +5     
==========================================
+ Hits       192371   192397      +26     
+ Misses      14124    14116       -8     
+ Partials     7939     7921      -18     

see 36 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@legendecas

Copy link
Copy Markdown
Member

Yes, we should wait until https://crrev.com/c/7718821 been cherry-picked to #61898 before landing this.

@legendecas legendecas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Test LGTM.

@legendecas legendecas added the blocked PRs that are blocked by other issues or PRs. label Apr 3, 2026
@Renegade334 Renegade334 removed the blocked PRs that are blocked by other issues or PRs. label Apr 16, 2026
@Renegade334

Copy link
Copy Markdown
Member Author

Unblocking as #61898 now passes these tests.

@Renegade334 Renegade334 added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 17, 2026
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 17, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/62571
✔  Done loading data for nodejs/node/pull/62571
----------------------------------- PR info ------------------------------------
Title      test: check contextify contextual store behavior in strict mode (#62571)
Author     René <[email protected]> (@Renegade334)
Branch     Renegade334:test-vm-contextual-store -> nodejs:main
Labels     vm, test, needs-ci
Commits    1
 - test: check contextify contextual store behavior in strict mode
Committers 1
 - Renegade334 <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/62571
Refs: https://github.com/nodejs/node/pull/62371
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/62571
Refs: https://github.com/nodejs/node/pull/62371
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 03 Apr 2026 09:36:43 GMT
   ✔  Approvals: 2
   ✔  - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/62571#pullrequestreview-4055848620
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/62571#pullrequestreview-4126603200
   ✔  Last GitHub CI successful
   ✘  No Jenkins CI runs detected
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/24552484328

@Renegade334 Renegade334 added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Apr 17, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 17, 2026
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@Renegade334 Renegade334 added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 18, 2026
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 18, 2026
@nodejs-github-bot nodejs-github-bot merged commit 64b3e83 into nodejs:main Apr 18, 2026
92 checks passed
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 64b3e83

aduh95 pushed a commit that referenced this pull request May 5, 2026
Previously, this behavior was tested indirectly by the REPL tests.
However, REPL now uses DONT_CONTEXTIFY for its VM context.

PR-URL: #62571
Refs: #62371
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
aduh95 pushed a commit that referenced this pull request May 7, 2026
Previously, this behavior was tested indirectly by the REPL tests.
However, REPL now uses DONT_CONTEXTIFY for its VM context.

PR-URL: #62571
Refs: #62371
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
araujogui pushed a commit to araujogui/node that referenced this pull request May 26, 2026
Previously, this behavior was tested indirectly by the REPL tests.
However, REPL now uses DONT_CONTEXTIFY for its VM context.

PR-URL: nodejs#62571
Refs: nodejs#62371
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
jkleinsc added a commit to electron/electron that referenced this pull request Jun 4, 2026
test-vm-global-contextual-store was added upstream in nodejs/node#62571. It
asserts that an undeclared strict-mode contextual store (`"use strict"; z = 42`)
throws a ReferenceError. Chromium's V8 removed
v8::PropertyCallbackInfo<T>::This(), so Electron's contextify setter cannot
distinguish a contextual store from an explicit `globalThis.x = v` store; it
keeps the original behavior (writing to the sandbox) so explicit global stores
in vm modules keep working (test-vm-module-referrer-realm). Guard the
strict-mode ReferenceError assertion in the new test under Electron.

Ref: nodejs/node#62571
Ref: nodejs/node#60616
Co-Authored-By: Claude Opus 4.8 <[email protected]>
jkleinsc added a commit to electron/electron that referenced this pull request Jun 4, 2026
test-vm-global-contextual-store was added upstream in nodejs/node#62571. It
asserts that an undeclared strict-mode contextual store (`"use strict"; z = 42`)
throws a ReferenceError. Chromium's V8 removed
v8::PropertyCallbackInfo<T>::This(), so Electron's contextify setter cannot
distinguish a contextual store from an explicit `globalThis.x = v` store; it
keeps the original behavior (writing to the sandbox) so explicit global stores
in vm modules keep working (test-vm-module-referrer-realm). Guard the
strict-mode ReferenceError assertion in the new test under Electron.

Ref: nodejs/node#62571
Ref: nodejs/node#60616
Co-Authored-By: Claude Opus 4.8 <[email protected]>
jkleinsc added a commit to electron/electron that referenced this pull request Jun 5, 2026
This new upstream test asserts that a strict-mode store to an undeclared
global in a vm context throws ReferenceError. Chromium's V8 contextify
global-proxy interceptor instead creates the property, so the assertion
is fundamentally incompatible with Electron's V8 and cannot be guarded
to pass.

Ref: nodejs/node#62571

Co-Authored-By: Claude Opus 4.8 <[email protected]>
jkleinsc added a commit to electron/electron that referenced this pull request Jun 8, 2026
* chore: bump node in DEPS to v24.16.0

* chore: remove upstreamed patch

Node.js restored fs patchability in the ESM loader upstream, making
the fix_lazyload_fs_in_esm_loaders_to_apply_asar_patches.patch
obsolete (the patch's exact change is now in lib/internal/modules/
esm/{load,resolve,translators}.js).

Ref: nodejs/node#62835

Co-Authored-By: Claude <[email protected]>
(cherry picked from commit 72638db)

* fix(patch): re-add experimental_fetch member after upstream cleanup

Upstream removed the experimental_fetch field from EnvironmentOptions,
but Electron's patch still registers --experimental-fetch as a CLI
option bound to that field. Re-add the member so the option compiles.

Ref: nodejs/node#62759

Co-Authored-By: Claude <[email protected]>
(cherry picked from commit 6fd9370)

* fix(patch): cast const away when freeing uv_cpu_info_t.model

libuv 1.52.1 typed uv_cpu_info_t.model as const char*, but uv__free
takes void*. Electron builds with -Werror,-Wincompatible-pointer-types-
discards-qualifiers, so add a cast. The memory is allocated via strdup
so the cast is safe.

Ref: nodejs/node#61829

Co-Authored-By: Claude <[email protected]>
(cherry picked from commit 90d1009)

* fix(patch): silence sign-compare warning in sessionVarintGetSafe
Cast int nBuf to size_t when comparing with sizeof(aCopy) so the
bundled sqlite3 amalgamation compiles under -Werror,-Wsign-compare.

Ref: nodejs/node#62699

Co-Authored-By: Claude <[email protected]>

* test: move root package.json aside in node spec runner

third_party/electron_node lives under Chromium's src/, whose package.json
("type": "module") is always an ancestor of the Node.js test tree. Upstream
assumes no package.json sits above the tests, so that ancestor changes how
test files and fixtures resolve their module type: it disables module-syntax
detection (breaking test-compile-cache-typescript-esm) and emits
MODULE_TYPELESS_PACKAGE_JSON warnings that break tests asserting clean stderr
(test-esm-detect-ambiguous, test-esm-import-meta-main-eval,
test-output-coverage-with-mock).

Move src/package.json aside for the duration of the run so the environment
matches upstream exactly, then restore it. The original is kept in a sibling
backup file so an interrupted/killed run self-heals on the next invocation
rather than leaving src/package.json missing.

Ref: Unable to locate reference

Co-Authored-By: Claude <[email protected]>
(cherry picked from commit 29abd0c)

* fix(patch): mark test-macos-app-sandbox as flaky

The test copies the Electron binary into a standalone .app bundle and
code-signs it; under parallel suite runs this races with dyld resolving
the Electron Framework rpath and intermittently aborts (SIGABRT). It
passes reliably when run alone. Mark it flaky so flakes don't fail CI.

Ref: Unable to locate reference

Co-Authored-By: Claude <[email protected]>
(cherry picked from commit 3853529)

* test: disable test-tls-set-default-ca-certificates-extra-override

setDefaultCACertificates() round-trips the default CA set through
BoringSSL's X509_STORE, which dedups a duplicate-subject root (DigiCert
Global Root CA) that OpenSSL keeps. The set therefore loses one cert on
re-add (149 -> 148), so the test's assertEqualCerts round-trip check
fails under Electron's BoringSSL. The sibling -recovery test is disabled
for the same reason.

Ref: nodejs/node#58822

Co-Authored-By: Claude <[email protected]>
(cherry picked from commit 3dbc596)

* fix(patch): mark test-runner watch tests as flaky

test-run-watch-repeatedly, test-run-watch-run-duration and
test-run-watch-without-file race under parallel suite load: the watcher
fires an extra re-run before the assertion, so the expected single-run
output ("tests 1") arrives with accumulated subtests. All three pass in
isolation.

Ref: nodejs/node#44898

Co-Authored-By: Claude <[email protected]>
(cherry picked from commit 0f9c327)

* chore(patches): update libuv const-cast patch management

Combine the Windows libuv cpu_info const-cast update into the existing
chore_cast_const_away_when_freeing_uv_cpu_info_t_model.patch and keep
a single patch-management commit for the final exported patch series.

Co-Authored-By: Claude <[email protected]>
(cherry picked from commit c590614)

* fix(patch): add UV_LOOP_INTERRUPT_ON_IO_CHANGE option

Ref: libuv/libuv#3308

Co-Authored-By: Claude <[email protected]>

* chore: update patches (trivial only)

Co-Authored-By: Claude <[email protected]>

* chore: update patches

Co-Authored-By: Claude <[email protected]>

* fix(patch): adapt GetIsolate removal for the inspector NetworkAgent

Adapts api_remove_deprecated_getisolate.patch for the inspector NetworkAgent:
adapt the GetIsolate removal to the NetworkAgent member helpers, qualify the
v8 Maybe helpers in network_agent, and drop duplicate inspector helper
definitions.

Ref: nodejs/node#62162

Co-Authored-By: Claude Opus 4.7 <[email protected]>

* fix(patch): guard vm contextual-store strict assertion for Chromium's V8

test-vm-global-contextual-store was added upstream in nodejs/node#62571. It
asserts that an undeclared strict-mode contextual store (`"use strict"; z = 42`)
throws a ReferenceError. Chromium's V8 removed
v8::PropertyCallbackInfo<T>::This(), so Electron's contextify setter cannot
distinguish a contextual store from an explicit `globalThis.x = v` store; it
keeps the original behavior (writing to the sandbox) so explicit global stores
in vm modules keep working (test-vm-module-referrer-realm). Guard the
strict-mode ReferenceError assertion in the new test under Electron.

Ref: nodejs/node#62571
Ref: nodejs/node#60616
Co-Authored-By: Claude Opus 4.8 <[email protected]>

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: John Kleinschmidt <[email protected]>
Co-authored-by: Claude <[email protected]>
jkleinsc added a commit to electron/electron that referenced this pull request Jun 9, 2026
* chore: bump node in DEPS to v24.16.0

* chore: remove upstreamed patch

Node.js restored fs patchability in the ESM loader upstream, making
the fix_lazyload_fs_in_esm_loaders_to_apply_asar_patches.patch
obsolete (the patch's exact change is now in lib/internal/modules/
esm/{load,resolve,translators}.js).

Ref: nodejs/node#62835

Co-Authored-By: Claude <[email protected]>
(cherry picked from commit 72638db)
(cherry picked from commit 0409547)

* fix(patch): re-add experimental_fetch member after upstream cleanup

Upstream removed the experimental_fetch field from EnvironmentOptions,
but Electron's patch still registers --experimental-fetch as a CLI
option bound to that field. Re-add the member so the option compiles.

Ref: nodejs/node#62759

Co-Authored-By: Claude <[email protected]>
(cherry picked from commit 6fd9370)
(cherry picked from commit 46b28d2)

* fix(patch): cast const away when freeing uv_cpu_info_t.model

libuv 1.52.1 typed uv_cpu_info_t.model as const char*, but uv__free
takes void*. Electron builds with -Werror,-Wincompatible-pointer-types-
discards-qualifiers, so add a cast. The memory is allocated via strdup
so the cast is safe.

Ref: nodejs/node#61829

Co-Authored-By: Claude <[email protected]>
(cherry picked from commit 90d1009)
(cherry picked from commit 07827c6)

* fix(patch): silence sign-compare warning in sessionVarintGetSafe
Cast int nBuf to size_t when comparing with sizeof(aCopy) so the
bundled sqlite3 amalgamation compiles under -Werror,-Wsign-compare.

Ref: nodejs/node#62699

Co-Authored-By: Claude <[email protected]>
(cherry picked from commit 379d109)

* test: move root package.json aside in node spec runner

third_party/electron_node lives under Chromium's src/, whose package.json
("type": "module") is always an ancestor of the Node.js test tree. Upstream
assumes no package.json sits above the tests, so that ancestor changes how
test files and fixtures resolve their module type: it disables module-syntax
detection (breaking test-compile-cache-typescript-esm) and emits
MODULE_TYPELESS_PACKAGE_JSON warnings that break tests asserting clean stderr
(test-esm-detect-ambiguous, test-esm-import-meta-main-eval,
test-output-coverage-with-mock).

Move src/package.json aside for the duration of the run so the environment
matches upstream exactly, then restore it. The original is kept in a sibling
backup file so an interrupted/killed run self-heals on the next invocation
rather than leaving src/package.json missing.

Ref: Unable to locate reference

Co-Authored-By: Claude <[email protected]>
(cherry picked from commit 29abd0c)
(cherry picked from commit 3190c99)

* fix(patch): mark test-macos-app-sandbox as flaky

The test copies the Electron binary into a standalone .app bundle and
code-signs it; under parallel suite runs this races with dyld resolving
the Electron Framework rpath and intermittently aborts (SIGABRT). It
passes reliably when run alone. Mark it flaky so flakes don't fail CI.

Ref: Unable to locate reference

Co-Authored-By: Claude <[email protected]>
(cherry picked from commit 3853529)
(cherry picked from commit 2dfc1ae)

* test: disable test-tls-set-default-ca-certificates-extra-override

setDefaultCACertificates() round-trips the default CA set through
BoringSSL's X509_STORE, which dedups a duplicate-subject root (DigiCert
Global Root CA) that OpenSSL keeps. The set therefore loses one cert on
re-add (149 -> 148), so the test's assertEqualCerts round-trip check
fails under Electron's BoringSSL. The sibling -recovery test is disabled
for the same reason.

Ref: nodejs/node#58822

Co-Authored-By: Claude <[email protected]>
(cherry picked from commit 3dbc596)
(cherry picked from commit 44f4094)

* fix(patch): mark test-runner watch tests as flaky

test-run-watch-repeatedly, test-run-watch-run-duration and
test-run-watch-without-file race under parallel suite load: the watcher
fires an extra re-run before the assertion, so the expected single-run
output ("tests 1") arrives with accumulated subtests. All three pass in
isolation.

Ref: nodejs/node#44898

Co-Authored-By: Claude <[email protected]>
(cherry picked from commit 0f9c327)
(cherry picked from commit 792e1e5)

* chore(patches): update libuv const-cast patch management

Combine the Windows libuv cpu_info const-cast update into the existing
chore_cast_const_away_when_freeing_uv_cpu_info_t_model.patch and keep
a single patch-management commit for the final exported patch series.

Co-Authored-By: Claude <[email protected]>
(cherry picked from commit c590614)
(cherry picked from commit 0506b64)

* fix(patch): add UV_LOOP_INTERRUPT_ON_IO_CHANGE option

Ref: libuv/libuv#3308

Co-Authored-By: Claude <[email protected]>
(cherry picked from commit 31b7fb0)

* fix(patch): update GetIsolate removal for inspector NetworkAgent refactor

Upstream refactored the inspector network agent helpers into
NetworkAgent:: methods, shifting where context->GetIsolate() appeared
and reorganizing node_contextify's PropertyDefinerCallback. Re-applied
the deprecated context->GetIsolate() -> v8::Isolate::GetCurrent()
replacements to the new structure.

Ref: nodejs/node#62162

Co-Authored-By: Claude Opus 4.8 <[email protected]>

* fix: drop dangling .patches entries for non-existent patch files

The cherry-pick of the cast-const fix (343078bf01) erroneously added
four entries to patches/node/.patches that reference patch files which
were never part of the 41-x-y branch (externalpointertypetag, thinlto
symbol preservation, v8 code cache, and startup snapshot generation).
The missing files caused `e sync` to abort while reading patches.

Co-Authored-By: Claude Opus 4.8 <[email protected]>

* chore: update patches (trivial only)

Co-Authored-By: Claude Opus 4.8 <[email protected]>

* test: disable test-vm-global-contextual-store

This new upstream test asserts that a strict-mode store to an undeclared
global in a vm context throws ReferenceError. Chromium's V8 contextify
global-proxy interceptor instead creates the property, so the assertion
is fundamentally incompatible with Electron's V8 and cannot be guarded
to pass.

Ref: nodejs/node#62571

Co-Authored-By: Claude Opus 4.8 <[email protected]>

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: John Kleinschmidt <[email protected]>
Co-authored-by: Claude <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. vm Issues and PRs related to the vm subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants