vm: TypeError thrown instead of ReferenceError when a Proxy is the vm context

domenic commented 3 months ago

Version

v22.7.0

Platform

Microsoft Windows NT 10.0.22631.0 x64

Subsystem

vm

What steps will reproduce the bug?

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

const context = vm.createContext({
  __proto__: new Proxy({}, {
    get(target, property, receiver) {
      return Reflect.get(target, property, receiver);
    }
  })
});
vm.runInContext("thisFunctionDoesNotExist()", context);

Output:

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

Always.

What is the expected behavior? Why is that the expected behavior?

If I instead do

const context = vm.createContext({
  __proto__: new Proxy({}, {})
});
vm.runInContext("thisFunctionDoesNotExist()", context);

or simply

const context = vm.createContext({});
vm.runInContext("thisFunctionDoesNotExist()", context);

then I get the expected output:

$ node test.js
evalmachine.<anonymous>:1
thisFunctionDoesNotExist()
^

ReferenceError: thisFunctionDoesNotExist is not defined
    at evalmachine.<anonymous>:1:1
    at Script.runInContext (node:vm:148:12)
    at Object.runInContext (node:vm:300:6)

What do you see instead?

evalmachine.<anonymous>:1
thisFunctionDoesNotExist()
^

TypeError: thisFunctionDoesNotExist is not a function
    at evalmachine.<anonymous>:1:1
    at Script.runInContext (node:vm:148:12)
    at Object.runInContext (node:vm:300:6)
    at Object.<anonymous> (C:\Users\d\OneDrive - domenic.me\Code\GitHub\jsdom\jsdom\test.js:31:4)
    at Module._compile (node:internal/modules/cjs/loader:1546:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1691:10)
    at Module.load (node:internal/modules/cjs/loader:1317:32)
    at Module._load (node:internal/modules/cjs/loader:1127:12)
    at TracingChannel.traceSync (node:diagnostics_channel:315:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:217:24)

Node.js v22.7.0

Additional information

I am not 100% sure this is a vm bug. It may be a fundamental limitation of Proxy and the complicated ECMAScript spec rules governing ReferenceError vs. TypeError. But I am pretty sure this is a vm limitation instead:

Browsers manage to give a ReferenceError here
Browsers have something very similar to proxies (the WindowProperties object) as part of their global object
I can't see anything in the spec for WindowProperties that cannot be emulated by a Proxy.

This is blocking jsdom from passing the web platform test window-runtime-error.html once I implement the WindowProperties object (https://github.com/jsdom/jsdom/pull/3765).

targos commented 3 months ago

Two observations:

It is not a regression (or not a recent one). I went back to v12 to reproduce.
Quick printf debugging in node_contextify.cc shows that while both cases with a Proxy call ContextifyContext::PropertyGetterCallback exactly 4 times (and no other intereptor), the problematic case actually intercepts 2 of the calls, vs 1 for the good case (with an empty proxy handler)

legendecas commented 2 months ago

This may be a V8 issue that when the global object has an interceptor it will not throw ReferenceError when the property does not exist: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/ic/ic.cc;l=454-481;drc=ee096094e94cdabe44749b7a3c89bdbb4082a527 (When an object has an interceptor, LookupIterator::IsFound() is always true`, and does not perform an existence check).

When the prototype of vm global object or sandbox object is a proxy, the PropertyGetterCallback will always return Intercepted::kYes without existence check.

An alternative would be performing an existence check on every PropertyGetterCallback.

nodejs / node