src: use new V8 API in vm module #16293

fhinkel · 2017-10-18T14:41:04Z

This PR removes the CopyProperties() hack in the vm module, i.e., Contextify.
Instead, it uses 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
#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
#13265

Refs: #6283
Refs: #15114
Refs: #13265

Fixes: #2734
Fixes: #10223
Fixes: #11803
Fixes: #11902

This PR requires a backport of
37a3a15 otherwise some tests will fail. Cherry pick PR.

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

Affected core subsystem(s)

src

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

Affected core subsystem(s)

src

jasnell

largely rubber stamp lgtm

bnoordhuis · 2017-10-18T16:20:17Z

src/node_contextify.cc

-          ctx->global_proxy()->GetRealNamedPropertyAttributes(context,
-                                                              property);
-    }
+    Local<String> key = property->ToString(isolate);


Can you use the overload that takes a Local<Context>?

bnoordhuis · 2017-10-18T16:21:43Z

src/node_contextify.cc

+    Local<Value> descriptor_intercepted = has.IsNothing() ?
+        ctx->global_proxy()->GetOwnPropertyDescriptor(context, key)
+            .ToLocalChecked() :
+        sandbox->GetOwnPropertyDescriptor(context, key).ToLocalChecked();


I'd write this as an if statement. It's rather hard to read.

Agree. Done.

bnoordhuis · 2017-10-18T16:23:24Z

src/node_contextify.cc

+    // If the property had already been defined on the sandbox or global object,
+    // we intercept, query and return it.
+    // Else, there is no need to intercept with first time definitions.
+    if (!descriptor_intercepted->IsUndefined()) {


I suppose the conditional isn't needed?

Why? If we intercept, it signals that the property was found. I think that's not correct if the property is not present.

Undefined is the default return value of API callbacks. I.e., the return value is going to be the same, with or without the IsUndefined() check.

ReturnValue on the CallbackArgs is set to undefined, but the call to the interceptor returns true or false depending on whether the return value was set. This then changes the control flow, see objects.cc.

But taking a closer look, I think the logic can be substantially simplified: take the value from the sandbox if present, otherwise proceed regularly. What do you think?

bnoordhuis · 2017-10-18T16:26:02Z

src/node_contextify.cc

+    } else {
+      Local<Value> value =
+          desc.has_value() ? desc.value() :
+          v8::Undefined(isolate).As<Value>();


This fits on one line (well, two, if you count the LHS.)

bnoordhuis · 2017-10-18T16:26:22Z

src/node_contextify.cc

+      }
+    }
+
+    printf("foo\n");


Stray debug code, I presume?

bnoordhuis · 2017-10-18T16:35:15Z

src/node_contextify.cc

@@ -65,6 +66,12 @@ using v8::WeakCallbackInfo;

 namespace {

+v8::Local<v8::Name> UIntToName(v8::Isolate* isolate, uint32_t index) {
+  const char* x = std::to_string(index).c_str();


If this function gets called a lot (which it looks like it is), it's arguably better to use snprintf() instead to avoid the extra allocation, or even to do manual stringification. C++17 adds std::to_chars() but we can't use that yet, unfortunately.

It might even be worthwhile to implement this as Uint32::New(...)->ToString(...) because that would reuse V8's number-to-string cache.

Yes, if there's any larger array, this is called for every index. Using Uint32::New()->ToString().

bnoordhuis · 2017-10-18T16:44:49Z

src/node_contextify.cc

+    bool is_function = value->IsFunction();
+
+    if (!is_declared && args.ShouldThrowOnError() && is_contextual_store &&
+    !is_function)


You fixed the style for this further up but not here.

bnoordhuis · 2017-10-18T16:49:18Z

src/node_contextify.cc

+      }
+    }
+
+    printf("foo\n");


Debug code again.

There's a lot of duplication. Couldn't you implement them by calling UIntToName(isolate, index) and passing the result to GlobalPropertyDefinerCallback(), etc.?

It's all duplication. Let me think about how to get rid of the code duplication while maintaining readability.

bnoordhuis · 2017-10-18T16:50:15Z

test/parallel/test-vm-global-non-writable-properties.js

-assert.strictEqual(ctx.x, undefined);  // Not copied out by cloneProperty().
+
+// This is fixed now!
+// assert.strictEqual(ctx.x, undefined);  // Not copied out by cloneProperty().


Can you drop the comment?

bnoordhuis · 2017-10-18T16:50:50Z

test/parallel/test-vm-strict-assign.js

+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.


You can drop the copyright boilerplate, we don't use it in new files.

fhinkel · 2017-10-19T06:24:25Z

@cpojer, since this is fixing bugs/altering behavior, do you want to check if that causes problems for Jest?

TimothyGu · 2017-10-19T07:44:15Z

I'll run this through the jsdom test suite as well.

fhinkel · 2017-10-19T08:09:28Z

Nits addressed, refactoring the code duplication part. Will squash afterwards.

bnoordhuis

See my comment about the default return value but otherwise LGTM. Nice work, Franziska and @AnnaMag.

bnoordhuis · 2017-10-19T09:51:13Z

src/node_contextify.cc

+    // Still initializing
+    if (ctx->context_.IsEmpty())
+      return;
+


Some extraneous blank lines here.

bnoordhuis · 2017-10-19T09:52:04Z

test/parallel/test-vm-global-non-writable-properties.js

@@ -7,7 +7,7 @@ const vm = require('vm');

 const ctx = vm.createContext();
 vm.runInContext('Object.defineProperty(this, "x", { value: 42 })', ctx);
-assert.strictEqual(ctx.x, undefined);  // Not copied out by cloneProperty().
+


Can you remove the blank line here?

cpojer · 2017-10-19T12:03:37Z

@fhinkel mind running this against Jest's version on master?

fhinkel · 2017-10-19T12:47:35Z

@cpojer Jest's tests helped me find a fatal error on my side 😄
I get the same test failures with Node master with and without my patch on Jest master. I guess that means you're not relying on any of our broken behavior that this patch fixes.

jest/integration_tests/coverage-threshold/__tests__/a-banana.js
  1:1  error  Delete `⏎······`  prettier/prettier

cpojer · 2017-10-19T13:04:43Z

Can you just try yarn jest on the repo, that'll run the JS tests? It seems like there is some weird eslint issue on your state of the Jest repo (master is green, so not sure what's up).

fhinkel · 2017-10-19T13:24:40Z

@cpojer I'm not getting very far with Node master, opend an issue here jestjs/jest#4731.

SimenB · 2017-10-19T14:34:35Z

As mentioned in the Jest issue, using node 9 RC fails worker-farm's tests as well. Jest uses worker-farm to parallelise test runs.

https://github.com/rvagg/node-worker-farm

$ npm version
{ 'worker-farm': '1.5.0',
  npm: '5.3.0',
  ares: '1.13.0',
  cldr: '31.0.1',
  http_parser: '2.7.0',
  icu: '59.1',
  modules: '58',
  nghttp2: '1.25.0',
  node: '9.0.0-rc.0',
  openssl: '1.0.2l',
  tz: '2017b',
  unicode: '9.0',
  uv: '1.15.0',
  v8: '6.1.534.42',
  zlib: '1.2.11' }

> worker-farm@1.5.0 test /Users/simbekkh/repos/node-worker-farm
> node ./tests/

TAP version 13
# simple, exports=function test
ok 1 pid makes sense
ok 2 pid makes sense
ok 3 rnd result makes sense
ok 4 workerFarm ended
# simple, exports.fn test
ok 5 pid makes sense
ok 6 pid makes sense
ok 7 rnd result makes sense
ok 8 workerFarm ended
# single worker
ok 9 only a single process (by pid)
ok 10 workerFarm ended
# two workers
ok 11 only two child processes (by pid)
ok 12 workerFarm ended
# many workers
ok 13 pids are all the same (by pid)
ok 14 workerFarm ended
# auto start workers
ok 15 child has been up before the request
ok 16 child has been up before the request
ok 17 child has been up before the request
ok 18 workerFarm ended
# single call per worker
ok 19 one process for each call (by pid)
ok 20 workerFarm ended
# two calls per worker
ok 21 one process for each call (by pid)
ok 22 workerFarm ended
# many concurrent calls
ok 23 processed tasks concurrently (173ms)
ok 24 workerFarm ended
# single concurrent call
ok 25 processed tasks sequentially (304ms)
ok 26 workerFarm ended
# multiple concurrent calls
ok 27 processed tasks concurrently (277ms)
ok 28 workerFarm ended
# durability
events.js:195
      throw er; // Unhandled 'error' event
      ^

Error [ERR_IPC_CHANNEL_CLOSED]: Channel closed
    at ChildProcess.target.send (internal/child_process.js:606:16)
    at Object.send (/Users/simbekkh/repos/node-worker-farm/lib/fork.js:24:17)
    at Farm.stopChild (/Users/simbekkh/repos/node-worker-farm/lib/farm.js:131:11)
    at Farm.<anonymous> (/Users/simbekkh/repos/node-worker-farm/lib/farm.js:96:10)
    at ontimeout (timers.js:478:11)
    at tryOnTimeout (timers.js:302:5)
    at Timer.listOnTimeout (timers.js:262:5)
npm ERR! Test failed.  See above for more details.

SimenB · 2017-10-19T16:15:07Z

git bisect blames f2b01cb

#4670

Bisect script if anyone wants to double-check

#!/bin/sh

./configure

make -j4 || exit 125 # an exit code of 125 asks "git bisect" to "skip" the current commit

# run the application and check that it produces good output
./node ../node-worker-farm/tests 2>&1 | grep 'ERR_IPC_CHANNEL_CLOSED'

if [ $? -eq 0 ]; then
   exit 1
fi

exit 0

I can confirm that reverting f2b01cb makes both worker-farm's and jest's test suites pass

fhinkel · 2017-10-19T16:26:56Z

@SimenB Do you mind putting your code snippets in details, as they are not related to this issue? Thanks!

fhinkel · 2017-10-23T09:57:40Z

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

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#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#13265 This PR requires a backport of https://chromium.googlesource.com/v8/v8/+/37a3a15c3e52e2146e45f41c427f24414e4d7f6f Refs: nodejs#6283 Refs: nodejs#15114 Refs: nodejs#13265 Fixes: nodejs#2734 Fixes: nodejs#10223 Fixes: nodejs#11803 Fixes: nodejs#11902

fhinkel · 2017-10-23T11:12:19Z

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

fhinkel · 2017-10-23T12:38:38Z

Landed in f1d6b04

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#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#13265 This PR requires a backport of https://chromium.googlesource.com/v8/v8/+/37a3a15c3e52e2146e45f41c427f24414e4d7f6f PR-URL: nodejs#16293 Fixes: nodejs#2734 Fixes: nodejs#10223 Fixes: nodejs#11803 Fixes: nodejs#11902 Ref: nodejs#6283 Ref: nodejs#15114 Ref: nodejs#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>

targos · 2017-10-23T12:40:15Z

Nice. Thank you and @AnnaMag for doing this!

Refs: nodejs#16293

The known issue is fixed with nodejs#16293. The text needs to call `Object.hasOwnProperty(this)` instead of `this.hasOwnProperty()`, otherwise `this` is from the wrong context is used. Add a second test case taken verbatim from issue nodejs#5350 Fixes: nodejs#5350 Refs: nodejs#16293

The known issue is fixed with nodejs#16293. The text needs to call `Object.hasOwnProperty(this)` instead of `this.hasOwnProperty()`, otherwise `this` is from the wrong context is used. Add a second test case taken verbatim from issue nodejs#5350 PR-URL: nodejs#16411 Fixes: nodejs#5350 Ref: nodejs#16293 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

PR-URL: #16409 Refs: #16293 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.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>

The known issue is fixed with nodejs/node#16293. The text needs to call `Object.hasOwnProperty(this)` instead of `this.hasOwnProperty()`, otherwise `this` is from the wrong context is used. Add a second test case taken verbatim from issue nodejs/node#5350 PR-URL: nodejs/node#16411 Fixes: nodejs/node#5350 Ref: nodejs/node#16293 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

PR-URL: nodejs/node#16409 Refs: nodejs/node#16293 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

The known issue is fixed with #16293. The text needs to call `Object.hasOwnProperty(this)` instead of `this.hasOwnProperty()`, otherwise `this` is from the wrong context is used. Add a second test case taken verbatim from issue #5350 PR-URL: #16411 Fixes: #5350 Ref: #16293 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.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>

The known issue is fixed with nodejs/node#16293. The text needs to call `Object.hasOwnProperty(this)` instead of `this.hasOwnProperty()`, otherwise `this` is from the wrong context is used. Add a second test case taken verbatim from issue nodejs/node#5350 PR-URL: nodejs/node#16411 Fixes: nodejs/node#5350 Ref: nodejs/node#16293 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

PR-URL: nodejs/node#16409 Refs: nodejs/node#16293 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem. labels Oct 18, 2017

fhinkel mentioned this pull request Oct 18, 2017

deps: cherry-pick 37a3a15c3 from V8 upstream #16294

Closed

1 task

fhinkel added dont-land-on-v4.x labels Oct 18, 2017

jasnell approved these changes Oct 18, 2017

View reviewed changes

bnoordhuis reviewed Oct 18, 2017

View reviewed changes

fhinkel added this to the 9.0.0 milestone Oct 18, 2017

TimothyGu self-assigned this Oct 19, 2017

fhinkel force-pushed the Oct/defineProp branch from 5eb8aaf to 324c912 Compare October 19, 2017 08:08

fhinkel force-pushed the Oct/defineProp branch 2 times, most recently from 367473e to fecefd0 Compare October 19, 2017 08:19

bnoordhuis approved these changes Oct 19, 2017

View reviewed changes

fhinkel force-pushed the Oct/defineProp branch from 743de90 to 03358aa Compare October 19, 2017 11:51

tniessen approved these changes Oct 19, 2017

View reviewed changes

fhinkel mentioned this pull request Oct 19, 2017

Channel closed error when running tests with Node 9.0.0-pre jestjs/jest#4731

Closed

addaleax approved these changes Oct 19, 2017

View reviewed changes

SimenB mentioned this pull request Oct 19, 2017

node v9 rc breaks test suite of worker-farm #16322

Closed

fhinkel force-pushed the Oct/defineProp branch from 96fcee8 to 6539413 Compare October 23, 2017 03:34

fhinkel force-pushed the Oct/defineProp branch from 6539413 to f7d22e5 Compare October 23, 2017 11:01

fhinkel closed this Oct 23, 2017

targos added a commit to targos/node that referenced this pull request Oct 23, 2017

test: add test for prop interceptors on sandbox

2c27b2e

Refs: nodejs#16293

targos mentioned this pull request Oct 23, 2017

test: add test for prop interceptors on sandbox #16409

Closed

2 tasks

This was referenced Oct 23, 2017

sandbox inheritied properties flattened after vm.runInNewContext #5350

Closed

test: fix test for inherited properties on vm #16411

Closed

fhinkel mentioned this pull request Oct 24, 2017

vm: remove CopyProperties()- code review #13265

Closed

3 tasks

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

MSLaguana mentioned this pull request Oct 31, 2017

vm.runInContext is not fully supported after upstream change nodejs/node-chakracore#420

Open

src: use new V8 API in vm module #16293

src: use new V8 API in vm module #16293

Conversation

fhinkel commented Oct 18, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Checklist

Affected core subsystem(s)

jasnell left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhinkel commented Oct 19, 2017

TimothyGu commented Oct 19, 2017

fhinkel commented Oct 19, 2017

bnoordhuis left a 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

cpojer commented Oct 19, 2017

fhinkel commented Oct 19, 2017 • edited Loading

cpojer commented Oct 19, 2017

fhinkel commented Oct 19, 2017

SimenB commented Oct 19, 2017 • edited Loading

SimenB commented Oct 19, 2017 • edited Loading

fhinkel commented Oct 19, 2017

fhinkel commented Oct 23, 2017

fhinkel commented Oct 23, 2017

fhinkel commented Oct 23, 2017

targos commented Oct 23, 2017

fhinkel commented Oct 18, 2017 •

edited

Loading

fhinkel commented Oct 19, 2017 •

edited

Loading

SimenB commented Oct 19, 2017 •

edited

Loading

SimenB commented Oct 19, 2017 •

edited

Loading