Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: implement query callbacks for vm #22390

Closed
wants to merge 4 commits into from
Closed

Conversation

TimothyGu
Copy link
Member

@TimothyGu TimothyGu commented Aug 18, 2018

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 nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 18, 2018
Original commit message:

    [api] Avoid needlessly calling descriptor interceptors

    Reland part of https://chromium-review.googlesource.com/c/v8/v8/+/816515.

    Change-Id: I72ad85ffd162fc0563fc25cdf35189e894f9dc82
    Reviewed-on: https://chromium-review.googlesource.com/1138808
    Commit-Queue: Timothy Gu <timothygu@chromium.org>
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#54492}

Refs: v8/v8@9eb96bb
Original commit message:

    [api][runtime]  Support all-in ctors of {Named,Indexed}PropertyHandlerConfiguration

    - Explicitly allows construction of
    {Named,Indexed}PropertyHandlerConfiguration with all the members filled.

    Bug: v8:7612
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: I426ea33846b5dbf2b3482c722c963a6e4b0abded
    Reviewed-on: https://chromium-review.googlesource.com/1163882
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Adam Klein <adamk@chromium.org>
    Commit-Queue: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#55142}

Refs: v8/v8@e1a7699
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@TimothyGu
Copy link
Member Author

/cc @nodejs/v8

This allows using a Proxy object as the sandbox for a VM context.

Fixes: nodejs#17480
Fixes: nodejs#17481
@TimothyGu
Copy link
Member Author

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
Copy link
Member

jdalton commented Aug 19, 2018

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

@@ -5878,6 +5878,26 @@ enum class PropertyHandlerFlags {
};

Copy link
Member

@jdalton jdalton Aug 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

descriptor(descriptor),
data(data),
flags(flags) {}

IndexedPropertyHandlerConfiguration(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 addaleax added the vm Issues and PRs related to the vm subsystem. label Aug 23, 2018
@addaleax
Copy link
Member

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
Copy link
Member Author

@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 added a commit that referenced this pull request Aug 24, 2018
Original commit message:

    [api] Avoid needlessly calling descriptor interceptors

    Reland part of https://chromium-review.googlesource.com/c/v8/v8/+/816515.

    Change-Id: I72ad85ffd162fc0563fc25cdf35189e894f9dc82
    Reviewed-on: https://chromium-review.googlesource.com/1138808
    Commit-Queue: Timothy Gu <timothygu@chromium.org>
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#54492}

PR-URL: #22390
Fixes: #17480
Fixes: #17481
Refs: v8/v8@9eb96bb
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
TimothyGu pushed a commit that referenced this pull request Aug 24, 2018
Original commit message:

    [api][runtime]  Support all-in ctors of {Named,Indexed}PropertyHandlerConfiguration

    - Explicitly allows construction of
    {Named,Indexed}PropertyHandlerConfiguration with all the members filled.

    Bug: v8:7612
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: I426ea33846b5dbf2b3482c722c963a6e4b0abded
    Reviewed-on: https://chromium-review.googlesource.com/1163882
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Adam Klein <adamk@chromium.org>
    Commit-Queue: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#55142}

PR-URL: #22390
Fixes: #17480
Fixes: #17481
Refs: v8/v8@e1a7699
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
TimothyGu added a commit that referenced this pull request Aug 24, 2018
This allows using a Proxy object as the sandbox for a VM context.

PR-URL: #22390
Fixes: #17480
Fixes: #17481
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
@TimothyGu
Copy link
Member Author

Landed in fa543c0...85c356c.

@TimothyGu TimothyGu closed this Aug 24, 2018
@TimothyGu TimothyGu deleted the v8-vm branch August 24, 2018 03:10
targos pushed a commit that referenced this pull request Aug 24, 2018
Original commit message:

    [api] Avoid needlessly calling descriptor interceptors

    Reland part of https://chromium-review.googlesource.com/c/v8/v8/+/816515.

    Change-Id: I72ad85ffd162fc0563fc25cdf35189e894f9dc82
    Reviewed-on: https://chromium-review.googlesource.com/1138808
    Commit-Queue: Timothy Gu <timothygu@chromium.org>
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#54492}

PR-URL: #22390
Fixes: #17480
Fixes: #17481
Refs: v8/v8@9eb96bb
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Aug 24, 2018
Original commit message:

    [api][runtime]  Support all-in ctors of {Named,Indexed}PropertyHandlerConfiguration

    - Explicitly allows construction of
    {Named,Indexed}PropertyHandlerConfiguration with all the members filled.

    Bug: v8:7612
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: I426ea33846b5dbf2b3482c722c963a6e4b0abded
    Reviewed-on: https://chromium-review.googlesource.com/1163882
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Adam Klein <adamk@chromium.org>
    Commit-Queue: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#55142}

PR-URL: #22390
Fixes: #17480
Fixes: #17481
Refs: v8/v8@e1a7699
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Aug 24, 2018
This allows using a Proxy object as the sandbox for a VM context.

PR-URL: #22390
Fixes: #17480
Fixes: #17481
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Sep 3, 2018
Original commit message:

    [api] Avoid needlessly calling descriptor interceptors

    Reland part of https://chromium-review.googlesource.com/c/v8/v8/+/816515.

    Change-Id: I72ad85ffd162fc0563fc25cdf35189e894f9dc82
    Reviewed-on: https://chromium-review.googlesource.com/1138808
    Commit-Queue: Timothy Gu <timothygu@chromium.org>
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#54492}

PR-URL: #22390
Fixes: #17480
Fixes: #17481
Refs: v8/v8@9eb96bb
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Sep 3, 2018
Original commit message:

    [api][runtime]  Support all-in ctors of {Named,Indexed}PropertyHandlerConfiguration

    - Explicitly allows construction of
    {Named,Indexed}PropertyHandlerConfiguration with all the members filled.

    Bug: v8:7612
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: I426ea33846b5dbf2b3482c722c963a6e4b0abded
    Reviewed-on: https://chromium-review.googlesource.com/1163882
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Adam Klein <adamk@chromium.org>
    Commit-Queue: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#55142}

PR-URL: #22390
Fixes: #17480
Fixes: #17481
Refs: v8/v8@e1a7699
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Sep 3, 2018
This allows using a Proxy object as the sandbox for a VM context.

PR-URL: #22390
Fixes: #17480
Fixes: #17481
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
@rvagg
Copy link
Member

rvagg commented Sep 6, 2018

this appears to be causing fatals, see #22723

targos pushed a commit to targos/node that referenced this pull request Sep 7, 2018
Original commit message:

    [api][runtime]  Support all-in ctors of {Named,Indexed}PropertyHandlerConfiguration

    - Explicitly allows construction of
    {Named,Indexed}PropertyHandlerConfiguration with all the members filled.

    Bug: v8:7612
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: I426ea33846b5dbf2b3482c722c963a6e4b0abded
    Reviewed-on: https://chromium-review.googlesource.com/1163882
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Adam Klein <adamk@chromium.org>
    Commit-Queue: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#55142}

PR-URL: nodejs#22390
Fixes: nodejs#17480
Fixes: nodejs#17481
Refs: v8/v8@e1a7699
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Sep 7, 2018
Original commit message:

    [api][runtime]  Support all-in ctors of {Named,Indexed}PropertyHandlerConfiguration

    - Explicitly allows construction of
    {Named,Indexed}PropertyHandlerConfiguration with all the members filled.

    Bug: v8:7612
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: I426ea33846b5dbf2b3482c722c963a6e4b0abded
    Reviewed-on: https://chromium-review.googlesource.com/1163882
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Adam Klein <adamk@chromium.org>
    Commit-Queue: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#55142}

PR-URL: nodejs#22390
Fixes: nodejs#17480
Fixes: nodejs#17481
Refs: v8/v8@e1a7699
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax mentioned this pull request Sep 13, 2018
3 tasks
addaleax added a commit to addaleax/node that referenced this pull request Sep 17, 2018
This reverts commit 85c356c
from PR nodejs#22390.

See the discussion in the (proposed) fix at
nodejs#22836.

Refs: nodejs#22836
Refs: nodejs#22390
Fixes: nodejs#22723
addaleax added a commit that referenced this pull request Sep 18, 2018
This reverts commit 85c356c
from PR #22390.

See the discussion in the (proposed) fix at
#22836.

Refs: #22836
Refs: #22390
Fixes: #22723

PR-URL: #22911
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 18, 2018
This is a re-land of a commit landed as part of
nodejs#22390.

---

This allows using a Proxy object as the sandbox for a VM context.

Refs: nodejs#22390
Fixes: nodejs#17480
Fixes: nodejs#17481
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit to addaleax/node that referenced this pull request Sep 18, 2018
Some of the choices here are odd, including that symbols are missing.
However, that matches previous behaviour.

What had to be changed was that inherited properties are no longer
included; the alternative would be to also refactor the descriptor
callbacks to provide data for inherited properties.

Fixes: nodejs#22723
Refs: nodejs#22390
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants