test: add vm module edge cases #11265

fhinkel · 2017-02-09T17:26:00Z

This PR adds two, admittedly contrived, examples that test edge cases of the vm module.

They demonstrate that the if statements if (maybe_rv.IsEmpty()) and
if (maybe_prop_attr.IsNothing()) in the GetterCallback
and the QueryCallback in src/node_contextify.cc
are observable. So far we have no tests that break when these
statements are removed (neither does Jest).

Both GetterCallback and QueryCallback
explicitly check the global_proxy() if a property is
not found on the sandbox. In these tests, the explicit check
inside the callback yields different results than deferring the
check until after the callback. The check is deferred, if the
callbacks do not intercept, i.e., if args.GetReturnValue().Set() is
not called.

Since we're working on fixing several of the vm issues, I'd like to have these
tests so we're at least aware if we cause any breaking changes.

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)

test

cjihrig · 2017-02-09T17:33:26Z

test/parallel/test-vm-property-not-on-sandbox.js

+let sandbox = {console};
+sandbox.document = { defaultView: sandbox };
+vm.createContext(sandbox);
+const code1 = 'Object.defineProperty(' +


We've been moving toward using block scopes in tests to avoid things like code1, code2, etc.

The joy of let and const 👍 . Fixed.

bnoordhuis

LGTM with style suggestions and if you can clarify the desc.value thing.

bnoordhuis · 2017-02-10T11:39:36Z

test/parallel/test-vm-property-not-on-sandbox.js

+// explicitly check the global_proxy() if a property is
+// not found on the sandbox. In the following tests, the explicit check
+// inside the callback yields different results than deferring the
+// check until after the callback. The check is deferred, if the


Treacherous comma? It reads as if the "if the callbacks do not intercept" part is its own sentence.

Comma deleted.

bnoordhuis · 2017-02-10T11:40:43Z

test/parallel/test-vm-property-not-on-sandbox.js

+// the global_proxy to keep the correct identities.
+//
+// This test case is partially inspired by
+// https://github.com/nodejs/node/issues/855


I'd either indent the comment or put the brace after the comment.

Not in block-scope anymore because split over different files.

bnoordhuis · 2017-02-10T11:41:59Z

test/parallel/test-vm-property-not-on-sandbox.js

+               '  "foo", ' +
+               '  { get: function() {return document.defaultView} }' +
+               ');' +
+               'var result = foo === this;';


You can use a backticks string if you want, probably easier to read.

bnoordhuis · 2017-02-10T11:46:37Z

test/parallel/test-vm-property-not-on-sandbox.js

+               '  { get: function() {return 17} }' +
+               ');' +
+               'var desc = Object.getOwnPropertyDescriptor(this, "foo");' +
+               'var result = typeof desc.value === "number";';


I don't follow why this would be the expected result instead of typeof desc.value === 'undefined' (and typeof desc.get === 'function'.) Is this a test that better belongs in test/known_issues?

Expected because it is the current behavior - not necessarily correct behavior. I wanted to make sure we have a test that breaks if somebody touches these 5 lines of code. But I'm getting more and more convinced, that these lines shouldn't be there. Moving this to known issues.

Add two, admittedly contrived, examples that test edge cases of the vm module. They demonstrate that the if statements `if (maybe_rv.IsEmpty())` and `if (maybe_prop_attr.IsNothing())` in the GetterCallback and the QueryCallback are observable. Both GetterCallback and QueryCallback explicitly check the global_proxy() if a property is not found on the sandbox. In these tests, the explicit check inside the callback yields different results than deferring the check until after the callback. The check is deferred, if the callbacks do not intercept, i.e., if args.GetReturnValue().Set() is not called.

fhinkel · 2017-02-10T16:28:12Z

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

Add two, admittedly contrived, examples that test edge cases of the vm module. They demonstrate that the if statements `if (maybe_rv.IsEmpty())` and `if (maybe_prop_attr.IsNothing())` in the GetterCallback and the QueryCallback are observable. Both GetterCallback and QueryCallback explicitly check the global_proxy() if a property is not found on the sandbox. In these tests, the explicit check inside the callback yields different results than deferring the check until after the callback. The check is deferred, if the callbacks do not intercept, i.e., if args.GetReturnValue().Set() is not called. PR-URL: #11265 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

jasnell · 2017-02-11T17:55:24Z

Landed in 5cd9d76

Add two, admittedly contrived, examples that test edge cases of the vm module. They demonstrate that the if statements `if (maybe_rv.IsEmpty())` and `if (maybe_prop_attr.IsNothing())` in the GetterCallback and the QueryCallback are observable. Both GetterCallback and QueryCallback explicitly check the global_proxy() if a property is not found on the sandbox. In these tests, the explicit check inside the callback yields different results than deferring the check until after the callback. The check is deferred, if the callbacks do not intercept, i.e., if args.GetReturnValue().Set() is not called. PR-URL: #11265 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Add two, admittedly contrived, examples that test edge cases of the vm module. They demonstrate that the if statements `if (maybe_rv.IsEmpty())` and `if (maybe_prop_attr.IsNothing())` in the GetterCallback and the QueryCallback are observable. Both GetterCallback and QueryCallback explicitly check the global_proxy() if a property is not found on the sandbox. In these tests, the explicit check inside the callback yields different results than deferring the check until after the callback. The check is deferred, if the callbacks do not intercept, i.e., if args.GetReturnValue().Set() is not called. PR-URL: nodejs#11265 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Add two, admittedly contrived, examples that test edge cases of the vm module. They demonstrate that the if statements `if (maybe_rv.IsEmpty())` and `if (maybe_prop_attr.IsNothing())` in the GetterCallback and the QueryCallback are observable. Both GetterCallback and QueryCallback explicitly check the global_proxy() if a property is not found on the sandbox. In these tests, the explicit check inside the callback yields different results than deferring the check until after the callback. The check is deferred, if the callbacks do not intercept, i.e., if args.GetReturnValue().Set() is not called. PR-URL: #11265 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 9, 2017

fhinkel added the vm Issues and PRs related to the vm subsystem. label Feb 9, 2017

cjihrig reviewed Feb 9, 2017

View reviewed changes

fhinkel force-pushed the vm-tests branch 2 times, most recently from a4a5bc8 to d10278c Compare February 9, 2017 17:43

cjihrig approved these changes Feb 9, 2017

View reviewed changes

jasnell approved these changes Feb 9, 2017

View reviewed changes

bnoordhuis approved these changes Feb 10, 2017

View reviewed changes

fhinkel force-pushed the vm-tests branch from d10278c to e4c3c59 Compare February 10, 2017 16:19

fhinkel force-pushed the vm-tests branch from e4c3c59 to 13f05fa Compare February 10, 2017 16:26

jasnell closed this Feb 11, 2017

italoacasas mentioned this pull request Feb 13, 2017

Proposal - 7.6.0 (Current) #11185

Merged

MylesBorins mentioned this pull request Mar 9, 2017

v6.10.1 proposal #11759

Merged

MylesBorins mentioned this pull request Mar 9, 2017

v4.8.1 proposal #11760

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add vm module edge cases #11265

test: add vm module edge cases #11265

fhinkel commented Feb 9, 2017

cjihrig Feb 9, 2017

fhinkel Feb 9, 2017

bnoordhuis left a comment

bnoordhuis Feb 10, 2017

fhinkel Feb 10, 2017

bnoordhuis Feb 10, 2017

fhinkel Feb 10, 2017

bnoordhuis Feb 10, 2017

fhinkel Feb 10, 2017

bnoordhuis Feb 10, 2017

fhinkel Feb 10, 2017

fhinkel commented Feb 10, 2017

jasnell commented Feb 11, 2017

test: add vm module edge cases #11265

test: add vm module edge cases #11265

Conversation

fhinkel commented Feb 9, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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 Feb 10, 2017

jasnell commented Feb 11, 2017