vm module regression in v18: setting properties on global proxy breaks jsdom

[domenic]

Version

v18.1.0

Platform

Microsoft Windows NT 10.0.22000.0 x64

Subsystem

vm

What steps will reproduce the bug?

Put this code in vm-test.js:

"use strict";
const vm = require("vm");

const window = createWindow();

console.log(Object.getOwnPropertyDescriptor(window.globalProxy, "onhashchange"));
window.globalProxy.onhashchange = () => {};

console.log("success");

function createWindow() {
  const obj = {};
  vm.createContext(obj);
  Object.defineProperty(obj, "onhashchange", {
    get() { },
    set() {},
    configurable: true
  });

  obj.globalProxy = vm.runInContext("this", obj);

  return obj;
}

How often does it reproduce? Is there a required condition?

Always reproduces

What is the expected behavior?

The v17 behavior was relied upon by jsdom extensively. Using Node v17.9.0 you get:

$ node vm-test.js
{
  get: [Function: get],
  set: [Function: set],
  enumerable: false,
  configurable: true
}
success

What do you see instead?

Using Node v18.0.0 or v18.1.0 you get:

$ node vm-test.js
{
  get: [Function: get],
  set: [Function: set],
  enumerable: false,
  configurable: true
}
C:\Users\Domenic\Dropbox\GitHub\jsdom\jsdom\vm-test.js:7
window.globalProxy.onhashchange = () => {};
                                ^

TypeError: Cannot redefine property: onhashchange
    at Object.<anonymous> (C:\Users\Domenic\Dropbox\GitHub\jsdom\jsdom\vm-test.js:7:33)
    at Module._compile (node:internal/modules/cjs/loader:1105:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1159:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Module._load (node:internal/modules/cjs/loader:827:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:77:12)
    at node:internal/main/run_main_module:17:47

Node.js v18.1.0

Additional information

No response

[legendecas]

Bisected into history I found that #42657 introduced the breakage. Will try to dig out the related V8 changes.

[syg]

The problem seems to be with named interceptors in node_contextify.cc. What's happening is that the setter interceptor, even when setting the property on the backing sandbox object, does not signal that the assignment is in fact now intercepted. So V8 tries to redo the assignment, which uses define semantics when interceptors are present, and doesn't allow redefines to accessors. I don't really grok the interceptor stuff, and since it's special V8 embedder stuff, I'm not entirely surprised it's behaving very weirdly from a JS MOP perspective.

As a fix/workaround, reading https://github.com/nodejs/node/blob/master/src/node_contextify.cc#L461, ISTM the intention is that this interceptor intends to handle the assignment at that point. I.e., V8 should consider the assignment complete and not continue if execution reaches L461 of that setter interceptor. If that's the case, then args.GetReturnValue() should be set with a non-null value to short-circuit, as per the API.

cc @joyeecheung to see if the suggested fix above makes sense.

The behavior change is from https://bugs.chromium.org/p/chromium/issues/detail?id=1309225 (you might not be able to see this due to security sensitivity), where for a security fix it looks like setter interceptors behavior was perhaps inadvertently changed.

I'll try to follow up internally with V8 to understand the behavior of interceptors.

[targos]

#42963

@syg looks like it works!

@domenic I used your example as a test case.