src: implement query callbacks for vm

[TimothyGu]

Fixes: #17480
Fixes: #17481

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

@TimothyGu build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/588/pipeline

[addaleax]

Looks good!

[TimothyGu]

/cc @nodejs/v8

[TimothyGu]

Also added a section in the VM docs on how to use Proxy with vm correctly.

CI: https://ci.nodejs.org/job/node-test-pull-request/16547/

[jdalton]

I was just trying to use a proxy as a context yesterday and re-running into issues.
Perfect fix timing 👏

[jdalton]

@TimothyGu As someone not super familiar with the internals of this (me) can you explain a bit about how this is fixing the linked to issues.

[jdalton]

@TimothyGu I noticed in the linked to issues in the opening summary that V8 work was needed. A patch for V8 was merged but then reverted for perf reasons. Was the perf issue eventually resolved and the appropriate V8 changed landed or is this PR working around V8?

[addaleax]

Gentle ping @TimothyGu ? I know it’s only been four days, so no hurry …

CI: https://ci.nodejs.org/job/node-test-pull-request/16707/

[TimothyGu]

@jdalton Take the in operator (#17480). V8 uses a few mechanisms in order to determine if an object contains the given property (or in ECMA-262 speak, "[[HasProperty]]"). For ordinary properties, that's easy, because V8 already knows everything about that object. However, for the global object of a vm-created context, we use V8 interceptors (a set of hooks corresponding roughly to the internal methods of a JavaScript object) in order to forward all the properties of the backing sandbox to make them seem like they are actually on the global object.

The issue here is twofold: 1) V8 interceptors do not provide a 1:1 mapping with JavaScript internal methods, and 2) even for interceptors that are indeed provided, for one reason or another V8 doesn't call them as the spec ordinarily mandates.

An example for 1) is that V8 doesn't provide a hook for [[HasProperty]] – which is what's used by the in operator per ECMA-262. But that actually turned out to be fine: even in ECMA-262, the default [[HasProperty]] internal method – OrdinaryHasProperty – is actually implemented in terms of other internal methods, namely [[GetOwnProperty]] and [[GetPrototypeOf]]. So in theory, as long as I implement the V8 interceptor corresponding to [[GetOwnProperty]] and set the prototype correctly, I should get the correct behavior for the in operator anyway.

Another place 1) is in effect. There are actually two mutually exclusive flavors of V8 interceptors corresponding to the two constructors of v8::NamedPropertyHandlerConfiguration: the legacy flavor, which has a hook called a "QueryCallback"; and the new flavor, which has a hook called "DescriptorCallback". These two callbacks are both designed to allow overriding the [[GetOwnProperty]] internal method, but the former is defined to return an v8::PropertyAttribute value – corresponding to the concept in ES3 §8.6.1 – while the latter (created by @fhinkel if I remember correctly) is defined to return a ES5+ property descriptor we all know and love. Either way, in theory, both flavors should be treated equally because they do the same job, unless we are talking about something like Object.getOwnPropertyDescriptor().

Now here's the catch: for the purposes of the in operator on an object with interceptors specified, V8 would in fact use the QueryCallback if one exists, as one would expect. On the other hand, it would not use the DescriptorCallback, instead falling through to the GetterCallback to confirm if a property exists. This is an instance of 2): Specifically, V8 now calls the [[Get]] internal method of the global object when evaluating an in operator expression, when we are expecting a call to [[GetOwnProperty]] (hence #17480)! But, let's follow through to see how this doesn't work with Proxy objects being the sandbox object.

Ordinarily, that scheme works fine because we use the V8-specific v8::Object::GetRealNamedProperty() API to query the sandbox object for the existence of the requested property in a script-unobservable manner, and if the sandbox doesn't have the property we fallback onto the global proxy.

However, this breaks down when Proxy objects are in play, since v8::Object::GetRealNamedProperty() is constrained to only call the get() hook in Proxy. On the other hand, the get() hook doesn't distinguish between nonexistence and a property with value undefined. Reflect.get({ a: undefined }, 'a') and Reflect.get({ a: undefined }, 'b') would both return undefined. This then leads to issues like #17465: namely, v8::Object::GetRealNamedProperty() knows we cannot possibly intercept the get() call if it's not there, so it uses the old property existence test mechanism on the first argument of the new Proxy() call; but it is forced to use the get() hook if it is there, even if all it does is the exact same as the default behavior.

My first attempt at fixing the bug was https://crrev.com/c/816515. It brings parity between QueryCallback and DescriptorCallback by calling the latter as well when evaluating in. It was reviewed and got in, but was soon reverted because it made Chrome run 3-22% slower. (Quite an achievement, really.) The reasoning is not too difficult to understand: passing an integer around (the case for "QueryCallback") is obviously faster than converting a JavaScript object representing a property descriptor (the return value of a DescriptorCallback) into a internal C++ structure every time someone tests for the existence of a property.

After not doing much with the bug for a few months, I noticed some folks working on Chromium has been trying to migrate their stack to use the new "DescriptorCallback" flavor of interceptors but failing, because of their equivalent of #17480 and #17481. In fact, they even raised my original attempt as an option for fixing their bug, only to be pointed out that the performance is probably going to be abysmal _{(how I wish I had that information before spending the time to implement it…)}. Instead, they proposed an alternative scheme: have a third flavor of interceptors that allow both a QueryCallback and a DescriptorCallback, called an "all-in" interceptor. This way, the spec correctness provided by DescriptorCallback is guaranteed, but things like in that don't require a full descriptor can go through the faster QueryCallback.

@camillobruni implemented that proposal in https://crrev.com/c/1163882 and I backported it here, and everything works now. Yay!

[TimothyGu]

Landed in fa543c0...85c356c.

[rvagg]

this appears to be causing fatals, see #22723