vm: remove CopyProperties()- code review #13265

AnnaMag · 2017-05-28T11:27:01Z

This patch removes the CP() function, replacing
callbacks with ones that allow the ES6 syntax.

This is a code review patch and it is not meant to land yet
for 2 reasons:

It works for named properties and temporarily breaks
setting indexed properties. This is done on purpose for easier
revision. Including the latter mirrors this code closely,
thus will be an extension of this patch.
It uses V8 API changes, which are under revision
https://codereview.chromium.org/2807333003/,

Changes included:

Extend NamedPropertyHandlerConfiguration
with PropertyDescriptorCallback and
PropertyDefinerCallback to query ES6 property
descriptors and use DefineProperty() API.
(remove Query callback, which becomes the Descriptor).
CopyProperties() is removed and not called anymore
in RunInContext().
A number of tests can be moved from
/known_issues to /parallel.
V8 API extension code (which IMHO can be ignored as
it is being reviewed elsewhere).

Checklist

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

Affected core subsystem(s)

vm

AnnaMag · 2017-05-28T11:27:33Z

cc/ @fhinkel

addaleax · 2017-05-28T11:35:15Z

src/node_contextify.cc

@@ -392,7 +306,7 @@ class ContextifyContext {
    MaybeLocal<Value> maybe_rv =
        sandbox->GetRealNamedProperty(context, property);
    if (maybe_rv.IsEmpty()) {
-      maybe_rv =
+        maybe_rv =


Is this by accident?

addaleax · 2017-05-28T11:37:36Z

src/node_contextify.cc

-      args.GetReturnValue().Set(prop_attr);
+    Maybe<bool> has = sandbox->HasOwnProperty(context, key);
+    Local<Value> descriptor_intercepted = has.IsNothing() ? ctx->global_proxy()
+        ->GetOwnPropertyDescriptor(context, key).ToLocalChecked() :


I think this might be more readable if you had a line break after the ?… if that makes the line too long, you can put the .ToLocalCheck() onto its own line (with 4 extra spaces), I guess

Done. Thanks

addaleax · 2017-05-28T11:38:42Z

src/node_contextify.cc

+
+    // If the property is not found on sandbox nor global
+    if (!descriptor_intercepted->IsUndefined()) {
+      info.GetReturnValue().Set(descriptor_intercepted);


Wouldn’t the default return value by Undefined, too?

Apologies for the typo. I meant If the property IS found on sandbox and/or global.
If the property/its descriptor are new (first time definition), we do not need to intercept and query it, so we don't call info.GetReturnValue().Set() and proceed with definition.

@addaleax, I updated the comment and I think it holds true. I might be missing on some edge case though, so please feel free to question it.
@fhinkel, is this correct?
Thank you!

addaleax · 2017-05-28T11:39:10Z

src/node_contextify.cc

+        Local<Name> property,
+        const PropertyDescriptor& desc,
+        const PropertyCallbackInfo<Value>& info) {
+      ContextifyContext* ctx;


I think this function body has 1 space too much indentation

addaleax · 2017-05-28T11:40:21Z

src/node_contextify.cc

+              sandbox->DefineProperty(context, property, *desc_for_sandbox);
+              // Set the property on the global object bypassing interceptors
+              // by using the v8::SKIP_INTERCEPTORS flag (default is
+              // v8::DONT::SKIP_INTERCEPTORS).


typo: DONT_SKIP_INTERCEPTORS?

fhinkel · 2017-05-29T06:59:57Z

test/known_issues/test-vm-attributes-property-not-on-sandbox.js

@@ -1,25 +0,0 @@
-'use strict';
-require('../common');


Why did you delete this instead of moving it over to tests?

There is a new test parallel/test-vm-attributes-on-sandbox.js, which corresponds to the edit of this one.

fhinkel · 2017-05-29T07:01:38Z

Great, thanks. Did you run the Jest tests to see if anything breaks? /cc @cpojer

fhinkel · 2017-05-29T07:03:16Z

src/node_contextify.cc

@@ -115,93 +115,6 @@ class ContextifyContext {
    return Local<Object>::Cast(context()->GetEmbedderData(kSandboxObjectIndex));
  }

-  // XXX(isaacs): This function only exists because of a shortcoming of


🎉 So nice seeing this function getting deleted.

cpojer · 2017-05-29T20:29:03Z

Running yarn and then yarn test on Jest's repository should confirm that this works or not :) Thank you for thinking of us!

AnnaMag · 2017-05-29T21:58:12Z

I have just run Jest tests on current master and the patched version and get exactly the same error messages from child_process.js and process/next_tick.js.
Not sure what those failures imply on master, but since there is no change, should we take it as no regression? 😄 /cc @fhinkel, @cpojer

fhinkel · 2017-06-06T14:51:10Z

@AnnaMag, I'm pretty sure yarn tests should be green on master. Do they pass if you use a release branch? Can you raise an issue on the yarn tracker?

AnnaMag · 2017-07-16T16:20:10Z

I cannot get yarn tests to run without errors with the current Jest master and master + official Node.js releases (via nvm, 5+). Is this a known issue, @cpojer? /cc @fhinkel

refack · 2017-07-16T19:41:41Z

There were know jest issues #13804 should be fixed in #14188.
Not 100% sure it's what you are seeing.

addaleax · 2017-07-16T21:12:09Z

@refack That bug never made it into official Node releases.

AnnaMag · 2017-07-18T16:00:15Z

@refack, thank you for pointing to the issue. The errors I was seeing were connected to the Jest integration tests. It is solved:
jestjs/jest#4062

TimothyGu · 2017-07-19T06:49:40Z

@AnnaMag ~~Is there anything preventing this from being merged except testing? I seem to remember some requisite V8 changes not being merged yet?~~ Never mind, seems like the V8 changes are squashed in the lone commit in this PR.

AnnaMag · 2017-07-19T10:18:25Z

@TimothyGu Ah yes, it might have been clearer to separate it into a standalone commit. Once those changes land in V8🤞, this (with a few extra lines of code) can be turned into an official PR.

fhinkel · 2017-08-03T11:00:28Z

@AnnaMag do you want to rebase this?

bnoordhuis · 2017-08-17T13:52:05Z

@AnnaMag Is this still relevant?

AnnaMag · 2017-08-17T20:42:53Z

@bnoordhuis, yes, but it is blocked by https://codereview.chromium.org/2807333003/
This vm patch was (and might still be) the only use case for that particular extension to the V8 API. It would be a courtesy on the V8 side to include the changes :) /cc @fhinkel.

I'll do the long overdue rebase to check the status. Thanks for pinging me about it 👍

BridgeAR · 2017-09-12T19:54:53Z

@fhinkel is there something you can do to get this unblocked?

AnnaMag · 2017-09-15T12:38:59Z

@BridgeAR, things are moving forward 👍 I will update and revive it asap.

Original commit message: [api] Intercept DefineProperty after Descriptor query Analog to other interceptors, intercept the DefineProperty call only after obtaining the property descriptor. This behavior allows us to mirror calls on a sandboxed object as it is needed in Node. See for example nodejs#13265 Bug: Change-Id: I73b8f8908d13473939b37fb6727858d0bee6bda3 Reviewed-on: https://chromium-review.googlesource.com/725295 Reviewed-by: Andreas Haas <ahaas@chromium.org> Commit-Queue: Franziska Hinkelmann <franzih@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#48683} PR-URL: nodejs#16294 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Original commit message: [api] Intercept DefineProperty after Descriptor query Analog to other interceptors, intercept the DefineProperty call only after obtaining the property descriptor. This behavior allows us to mirror calls on a sandboxed object as it is needed in Node. See for example #13265 Bug: Change-Id: I73b8f8908d13473939b37fb6727858d0bee6bda3 Reviewed-on: https://chromium-review.googlesource.com/725295 Reviewed-by: Andreas Haas <ahaas@chromium.org> Commit-Queue: Franziska Hinkelmann <franzih@chromium.org> Cr-Commit-Position: refs/heads/master@{#48683} PR-URL: #16294 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Original commit message: [api] Intercept DefineProperty after Descriptor query Analog to other interceptors, intercept the DefineProperty call only after obtaining the property descriptor. This behavior allows us to mirror calls on a sandboxed object as it is needed in Node. See for example nodejs/node#13265 Bug: Change-Id: I73b8f8908d13473939b37fb6727858d0bee6bda3 Reviewed-on: https://chromium-review.googlesource.com/725295 Reviewed-by: Andreas Haas <ahaas@chromium.org> Commit-Queue: Franziska Hinkelmann <franzih@chromium.org> Cr-Commit-Position: refs/heads/master@{#48683} PR-URL: nodejs/node#16294 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Remove the CopyProperties() hack in the vm module, i.e., Contextify. Use different V8 API methods, that allow interception of DefineProperty() and do not flatten accessor descriptors to property descriptors. Move many known issues to test cases. Factor out the last test in test-vm-context.js for nodejs/node#10223 into its own file, test-vm-strict-assign.js. Part of this CL is taken from a stalled PR by https://github.com/AnnaMag nodejs/node#13265 This PR requires a backport of https://chromium.googlesource.com/v8/v8/+/37a3a15c3e52e2146e45f41c427f24414e4d7f6f PR-URL: nodejs/node#16293 Fixes: nodejs/node#2734 Fixes: nodejs/node#10223 Fixes: nodejs/node#11803 Fixes: nodejs/node#11902 Ref: nodejs/node#6283 Ref: nodejs/node#15114 Ref: nodejs/node#13265 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Original commit message: [api] Intercept DefineProperty after Descriptor query Analog to other interceptors, intercept the DefineProperty call only after obtaining the property descriptor. This behavior allows us to mirror calls on a sandboxed object as it is needed in Node. See for example nodejs#13265 Bug: Change-Id: I73b8f8908d13473939b37fb6727858d0bee6bda3 Reviewed-on: https://chromium-review.googlesource.com/725295 Reviewed-by: Andreas Haas <ahaas@chromium.org> Commit-Queue: Franziska Hinkelmann <franzih@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#48683} PR-URL: nodejs#16294 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Original commit message: [api] Intercept DefineProperty after Descriptor query Analog to other interceptors, intercept the DefineProperty call only after obtaining the property descriptor. This behavior allows us to mirror calls on a sandboxed object as it is needed in Node. See for example nodejs#13265 Bug: Change-Id: I73b8f8908d13473939b37fb6727858d0bee6bda3 Reviewed-on: https://chromium-review.googlesource.com/725295 Reviewed-by: Andreas Haas <ahaas@chromium.org> Commit-Queue: Franziska Hinkelmann <franzih@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#48683} PR-URL: nodejs#16294 Backport-PR-URL: nodejs#16413 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Original commit message: [api] Intercept DefineProperty after Descriptor query Analog to other interceptors, intercept the DefineProperty call only after obtaining the property descriptor. This behavior allows us to mirror calls on a sandboxed object as it is needed in Node. See for example nodejs#13265 Bug: Change-Id: I73b8f8908d13473939b37fb6727858d0bee6bda3 Reviewed-on: https://chromium-review.googlesource.com/725295 Reviewed-by: Andreas Haas <ahaas@chromium.org> Commit-Queue: Franziska Hinkelmann <franzih@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#48683} PR-URL: nodejs#16294 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Original commit message: [api] Intercept DefineProperty after Descriptor query Analog to other interceptors, intercept the DefineProperty call only after obtaining the property descriptor. This behavior allows us to mirror calls on a sandboxed object as it is needed in Node. See for example #13265 Bug: Change-Id: I73b8f8908d13473939b37fb6727858d0bee6bda3 Reviewed-on: https://chromium-review.googlesource.com/725295 Reviewed-by: Andreas Haas <ahaas@chromium.org> Commit-Queue: Franziska Hinkelmann <franzih@chromium.org> Cr-Commit-Position: refs/heads/master@{#48683} PR-URL: #16294 Backport-PR-URL: #16413 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency. vm Issues and PRs related to the vm subsystem. labels May 28, 2017

addaleax added the wip Issues and PRs that are still a work in progress. label May 28, 2017

addaleax reviewed May 28, 2017

View reviewed changes

AnnaMag force-pushed the v8-5_removeCP_review branch 2 times, most recently from 027facd to 89ed92d Compare May 28, 2017 12:03

fhinkel reviewed May 29, 2017

View reviewed changes

This was referenced Jun 3, 2017

VM context object silently fails on Object.defineProperty #5679

Closed

node_contextify tracking issue #6283

Closed

TimothyGu mentioned this pull request Jul 18, 2017

[wip] src, inspector: support opted-in VM contexts #14231

Closed

4 tasks

TimothyGu mentioned this pull request Aug 7, 2017

VM module doesn't handle undefined/NaN/null correctly #14660

Closed

bnoordhuis added the stalled Issues and PRs that are stalled. label Aug 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vm: remove CopyProperties()- code review #13265

vm: remove CopyProperties()- code review #13265

AnnaMag commented May 28, 2017 •

edited

Loading

AnnaMag commented May 28, 2017

addaleax May 28, 2017

AnnaMag May 28, 2017

addaleax May 28, 2017

AnnaMag May 28, 2017

addaleax May 28, 2017

AnnaMag May 28, 2017 •

edited

Loading

AnnaMag May 28, 2017 •

edited

Loading

addaleax May 28, 2017

AnnaMag May 28, 2017

addaleax May 28, 2017

AnnaMag May 28, 2017

fhinkel May 29, 2017

AnnaMag May 29, 2017

fhinkel commented May 29, 2017

fhinkel May 29, 2017

cpojer commented May 29, 2017

AnnaMag commented May 29, 2017 •

edited

Loading

fhinkel commented Jun 6, 2017

AnnaMag commented Jul 16, 2017 •

edited

Loading

refack commented Jul 16, 2017

addaleax commented Jul 16, 2017

AnnaMag commented Jul 18, 2017 •

edited

Loading

TimothyGu commented Jul 19, 2017 •

edited

Loading

AnnaMag commented Jul 19, 2017

fhinkel commented Aug 3, 2017

bnoordhuis commented Aug 17, 2017

AnnaMag commented Aug 17, 2017 •

edited

Loading

BridgeAR commented Sep 12, 2017

AnnaMag commented Sep 15, 2017 •

edited

Loading

vm: remove CopyProperties()- code review #13265

vm: remove CopyProperties()- code review #13265

Conversation

AnnaMag commented May 28, 2017 • edited Loading

Checklist

Affected core subsystem(s)

AnnaMag commented May 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnnaMag May 28, 2017 • edited Loading

Choose a reason for hiding this comment

AnnaMag May 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhinkel commented May 29, 2017

Choose a reason for hiding this comment

cpojer commented May 29, 2017

AnnaMag commented May 29, 2017 • edited Loading

fhinkel commented Jun 6, 2017

AnnaMag commented Jul 16, 2017 • edited Loading

refack commented Jul 16, 2017

addaleax commented Jul 16, 2017

AnnaMag commented Jul 18, 2017 • edited Loading

TimothyGu commented Jul 19, 2017 • edited Loading

AnnaMag commented Jul 19, 2017

fhinkel commented Aug 3, 2017

bnoordhuis commented Aug 17, 2017

AnnaMag commented Aug 17, 2017 • edited Loading

BridgeAR commented Sep 12, 2017

AnnaMag commented Sep 15, 2017 • edited Loading

AnnaMag commented May 28, 2017 •

edited

Loading

AnnaMag May 28, 2017 •

edited

Loading

AnnaMag May 28, 2017 •

edited

Loading

AnnaMag commented May 29, 2017 •

edited

Loading

AnnaMag commented Jul 16, 2017 •

edited

Loading

AnnaMag commented Jul 18, 2017 •

edited

Loading

TimothyGu commented Jul 19, 2017 •

edited

Loading

AnnaMag commented Aug 17, 2017 •

edited

Loading

AnnaMag commented Sep 15, 2017 •

edited

Loading