vm: DONT_CONTEXTIFY when no contextObject set

[legendecas]

When contextObject is not passed in vm.createContext, do not
contextify by default. This reduces the chance that the script semantics
are broken by the interceptors.

It's getting harder to maintain a spec-compliant behavior in a
vm contextified context. Hopefully this could reduce the breakage where
the contextObject is not set, like a simple vm.createContext()
usage.

Refs: #61898 (comment)

[legendecas]

This assert.throws needs to be removed when #61898 lands.

[jasnell]

I assume that if this lands first, 61898 will CI will fail or...? Just want to make sure it's not easily missed.

[legendecas]

if this lands first, 61898 will need updating this test as a known issue, as #61898 (comment).

[legendecas]

Or, we can land after #61898. This PR is trying to remediate #61898 (comment).

[joyeecheung]

I think this is technically semver-major e.g. it affects proxy/accessor behavior?

[legendecas]

I think this is technically semver-major e.g. it affects proxy/accessor behavior?

This only changes vm.createContext() with no arguments. The result should not be observable given that the user didn't pass in a custom context object.

And inside the vm, the context behavior is more aligned with spec, so this could be even a bug-fix for the case. I think this does not necessarily consist as a breaking change?

[joyeecheung]

I am thinking more about what happens when the manipulation happens in the script for the global proxy or it's passed out with other stuff installed, e.g.

> vm.runInNewContext('Object.freeze(globalThis)')
evalmachine.<anonymous>:1
Object.freeze(globalThis)
       ^

Uncaught TypeError: Cannot freeze
    at Object.freeze (<anonymous>)
    at evalmachine.<anonymous>:1:8
    at Script.runInContext (node:vm:149:12)
    at Script.runInNewContext (node:vm:154:17)
    at Object.runInNewContext (node:vm:310:38)
    at REPL2:1:4
    at ContextifyScript.runInThisContext (node:vm:137:12)
    at REPLServer.defaultEval (node:repl:600:22)
    at bound (node:domain:433:15)
    at REPLServer.runBound [as eval] (node:domain:444:12)

> vm.runInNewContext('Object.freeze(globalThis)', vm.constants.DONT_CONTEXTIFY)
{}

> Object.freeze(vm.runInNewContext('globalThis'))
Uncaught TypeError: Cannot freeze
    at Object.freeze (<anonymous>)

> Object.freeze(vm.runInNewContext('globalThis', vm.constants.DONT_CONTEXTIFY))
{}

[joyeecheung]

Actually this seems more easily observable than I thought..

> vm.createContext(vm.constants.DONT_CONTEXTIFY).Array
[Function: Array]
> vm.createContext().Array
undefined

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.71%. Comparing base (38647b3) to head (5b9187e).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #62459      +/-   ##
==========================================
- Coverage   89.71%   89.71%   -0.01%     
==========================================
  Files         676      678       +2     
  Lines      206751   207260     +509     
  Branches    39645    39755     +110     
==========================================
+ Hits       185482   185937     +455     
- Misses      13403    13449      +46     
- Partials     7866     7874       +8     

Files with missing lines	Coverage Δ
lib/vm.js	99.28% <100.00%> (ø)

... and 62 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]

Added semver-major.

[joyeecheung]

I realized the docs need some updates, e.g. the parts that talk about the differences when it's contexified or not https://nodejs.org/api/vm.html#scriptruninnewcontextcontextobject-options

[jasnell]

LGTM with the doc edit @joyeecheung is suggesting

[legendecas]

Taking @jasnell's comment on #62459 (comment) into consideration, I think given that #62371 has fixed CI for #61898, it would be easier to land this PR after the V8 upgrade.