Repository navigation
vm: regression for "in" operator on global between 22.4.1 and 22.5.0; breaks jsdom #54436
Description
Activity
Can you strip your example to just the basics? Something very small that will reproduce this?
I've already done so. It's 30 lines with no external dependencies. The issue is specifically about class hierarchies, inherited methods, and subclassing so it's not like it can get any smaller. Maybe I could remove
enumerable: true, configurable: trueto save two lines if that'd help you out. Or rename the variables from descriptive names likeWindowandEventTargetinto generic ones likeSubclassandSuperclass.I've already done so. It's 30 lines with no external dependencies. The issue is specifically about class hierarchies, inherited methods, and subclassing so it's not like it can get any smaller. Maybe I could remove
enumerable: true, configurable: trueto save two lines if that'd help you out. Or rename the variables from descriptive names likeWindowandEventTargetinto generic ones likeSubclassandSuperclass.You are correct, I was trying to oversimplify the code, but that's not needed. Sorry for the mistake. When I get a chance I'll check this for
repro-exists.FWIW the only vm subsystem PR for that version is #53517
Maybe c0962dc @legendecas ?
I can confirm the issue. Will prepare a fix soon.
Reacted by Domenic Denicola- added a commit that references this issue
on Aug 19, 2024 I think the best course of this issue would be setting the prototype of the inner
globalThisin the vm context. The issue is caused by the class hierarchy with theglobalThis:Outer Context | Vm Context Object | Object ^ | ^ | prototype | | prototype EventTarget | Global ^ | ^ | prototype | | prototype Window | | ^ | | | prototype | | window (sandbox) <--- globalThis intercepts property accessIf I understand correctly, the expected class hierarchy should be:
Outer Context | Vm Context Object | Object ^ | ^ | prototype | | prototype EventTarget | EventTarget ^ | ^ | prototype | | prototype Window | Window ^ | ^ | prototype | | prototype window (sandbox) <--- globalThis intercepts property accessWith the fix in #53517, the inner
globalThiscorrectly queries the own properties of the sandbox object withObject.hasOwn, however, it can not distinguishinoperator withObject.hasOwnwith current V8 APIs.To fix the issue and preserving the correct
Object.hasOwnbehavior, we have two solutions:- Delegate
prototypelookup to the sandbox object, so thatinoperator will lookup the property on the outer prototype instead. However, this may unconditionally leaks vm outsideObject.prototypeproperties to the inner vm context. I believe this may already be the case forjsdomsince API objects likeEventTargetare inherited from the outsideObject. - Manually setting the inner
globalThisprototype to be theWindow.prototypein userland before (1) gets implemented. This works in Node.js versions before and afterv22.5.0. Moreover, inv22.5.0and later,Object.hasOwnwill return correct value.
Since
Object.hasOwnbehavior is not fixable in user land, I am wondering if it would make sense to you to set the inner global prototype like the following snippet:"use strict"; const vm = require("vm"); class EventTarget { addEventListener() {} } const windowConstructor = function () {}; Object.setPrototypeOf(windowConstructor, EventTarget); const windowPrototype = Object.create(EventTarget.prototype); function Window() { vm.createContext(this); this._globalProxy = vm.runInContext("this", this); // Set up the inner global prototype Object.setPrototypeOf(this._globalProxy, windowPrototype); const window = this; Object.defineProperty(this, "window", { get() { return window._globalProxy; }, enumerable: true, configurable: true }); } const window = new Window(); console.log(vm.runInContext(`"addEventListener" in window`, window));
In this way, the following snippet will output the expected results:
const jsdom = require('jsdom'); const vm = require('vm'); const { JSDOM } = jsdom; const dom = new JSDOM(``, { runScripts: 'outside-only', }); const vmContext = dom.getInternalVMContext(); // Uncomment the following line to see the difference. // vm.runInContext(`Object.setPrototypeOf(window, Window.prototype)`, vmContext); console.log(vm.runInContext(`window instanceof Object`, vmContext)); // => always `false` because the window was created outside the context console.log(vm.runInContext(`window instanceof EventTarget`, vmContext)); console.log(vm.runInContext(`'addEventListener' in window`, vmContext)); console.log(vm.runInContext(`Object.hasOwn(window, 'addEventListener')`, vmContext)); console.log(vm.runInContext(`Object.getOwnPropertyDescriptor(window, 'addEventListener')`, vmContext));
The output should be in Node.js
v22.5.0:false true true false undefined- Delegate
I can try such things, but from past experience anything other than our current (admittedly convoluted) setup has caused lots of tests to break. I would expect that anything that causes a regression like this to be reverted...
Given how many other things this appears to fix, I'm happy to close this. In fact, I would wonder if it's possible to back-port c0962dc to v20, so that I could land jsdom/jsdom#3765 without having to wait for v22 to become the earliest LTS...
I removed https://github.com/nodejs/node/labels/dont-land-on-v20.x on the original PR and it can be backported in the next release.
- added a commit that references this issue
on Sep 4, 2024 - added a commit that references this issue
on Sep 12, 2024
Version
v22.5.0
Platform
Subsystem
vm
What steps will reproduce the bug?
How often does it reproduce? Is there a required condition?
Every time
What is the expected behavior? Why is that the expected behavior?
Outputs true. (That occurs on v22.4.1.)
What do you see instead?
Outputs false
Additional information
No response