vm: regression for "in" operator on global between 22.4.1 and 22.5.0; breaks jsdom

[domenic]

Version

v22.5.0

Platform

Microsoft Windows NT 10.0.22631.0 x64

Subsystem

vm

What steps will reproduce the bug?

"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);

  Object.setPrototypeOf(this, 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));

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

[RedYetiDev]

Can you strip your example to just the basics? Something very small that will reproduce this?

[domenic]

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: true to save two lines if that'd help you out. Or rename the variables from descriptive names like Window and EventTarget into generic ones like Subclass and Superclass.

[RedYetiDev]

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: true to save two lines if that'd help you out. Or rename the variables from descriptive names like Window and EventTarget into generic ones like Subclass and Superclass.

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.

[RedYetiDev]

FWIW the only vm subsystem PR for that version is #53517

[marco-ippolito]

Maybe c0962dc @legendecas ?

[legendecas]

I can confirm the issue. Will prepare a fix soon.

[legendecas]

I think the best course of this issue would be setting the prototype of the inner globalThis in the vm context. The issue is caused by the class hierarchy with the globalThis:

Outer Context    |    Vm Context
Object           |    Object
  ^              |      ^
  | prototype    |      | prototype
EventTarget      |    Global
  ^              |      ^
  | prototype    |      | prototype
Window           |      |
  ^              |      |
  | prototype    |      |
window (sandbox) <--- globalThis
		intercepts property access

If 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 access

With the fix in #53517, the inner globalThis correctly queries the own properties of the sandbox object with Object.hasOwn, however, it can not distinguish in operator with Object.hasOwn with current V8 APIs.

To fix the issue and preserving the correct Object.hasOwn behavior, we have two solutions:

1. Delegate prototype lookup to the sandbox object, so that in operator will lookup the property on the outer prototype instead. However, this may unconditionally leaks vm outside Object.prototype properties to the inner vm context. I believe this may already be the case for jsdom since API objects like EventTarget are inherited from the outside Object.
2. Manually setting the inner globalThis prototype to be the Window.prototype in userland before (1) gets implemented. This works in Node.js versions before and after v22.5.0. Moreover, in v22.5.0 and later, Object.hasOwn will return correct value.

Since Object.hasOwn behavior 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

[domenic]

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...

[domenic]

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...

[legendecas]

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.