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

vm: accessor properties get converted to data properties inside the vm #2734

Closed
domenic opened this issue Sep 8, 2015 · 6 comments
Closed
Labels
vm Issues and PRs related to the vm subsystem.

Comments

@domenic
Copy link
Contributor

domenic commented Sep 8, 2015

Example:

'use strict';
const vm = require('vm');

const x = {};
Object.defineProperty(x, 'prop', {
  get() { return 'foo'; }
});
const o = vm.createContext(x);

const code = 'Object.getOwnPropertyDescriptor(this, "prop")';
const res = vm.runInContext(code, o, 'test');

console.log(res);

gives

$ iojs test.js
Object {
  value: 'foo',
  writable: true,
  enumerable: false,
  configurable: false }

In 659dadd I fixed it to return the correct data descriptor. But it appears the accessor descriptor is just broken.

As far as I can tell this is a shortcoming of the V8 API. The named properties handler and the GlobalPropertyQueryCallback always returns data descriptors. This is probably because that is all that is required by Web IDL for the browser use case, where the named property handler is meant to be used for things like window.idOrName or window.forms.foo, which are always data descriptors.

I think Chrome also has this problem. Even for non-named properties, things like window.top are data descriptors. I'm asking related questions on blink-dev.

Original discovery: jsdom/jsdom#1208, where this is affecting Facebook's use in jest.

/cc @nodejs/v8

@domenic domenic added the vm Issues and PRs related to the vm subsystem. label Sep 8, 2015
@nicolashenry
Copy link

I have an issue that may be related :

function Window() {
    this.Window = Window;
    this.console = console;
}

var w = new Window();

var vm = require("vm");

vm.createContext(w);

vm.runInContext('console.log(this instanceof Window);', w);

gives

false

cjihrig added a commit to cjihrig/node that referenced this issue Mar 12, 2016
This commit adds tests for several known issues.

Refs: nodejs#1901
Refs: nodejs#728
Refs: nodejs#4778
Refs: nodejs#947
Refs: nodejs#2734
PR-URL: nodejs#5653
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
evanlucas pushed a commit that referenced this issue Mar 14, 2016
This commit adds tests for several known issues.

Refs: #1901
Refs: #728
Refs: #4778
Refs: #947
Refs: #2734
PR-URL: #5653
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
rvagg pushed a commit that referenced this issue Mar 16, 2016
This commit adds tests for several known issues.

Refs: #1901
Refs: #728
Refs: #4778
Refs: #947
Refs: #2734
PR-URL: #5653
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 30, 2016
This commit adds tests for several known issues.

Refs: #1901
Refs: #728
Refs: #4778
Refs: #947
Refs: #2734
PR-URL: #5653
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 30, 2016
This commit adds tests for several known issues.

Refs: #1901
Refs: #728
Refs: #4778
Refs: #947
Refs: #2734
PR-URL: #5653
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@jasnell
Copy link
Member

jasnell commented Apr 13, 2016

@domenic @ofrobots ... I see this is still occurring v4.9 in master. Any updates on this?

@brettz9
Copy link

brettz9 commented Feb 3, 2017

I don't know about other contexts, but the problem only appears to occur for me when the accessor is on the vm's global.

This code (which adds an intervening parent object to two lines of code) produces the expected result showing the getter in the accessor:

'use strict';
const vm = require('vm');

const x = {};
Object.defineProperty(x, 'prop', {
  get() { return 'foo'; }
});
const o = vm.createContext({parent: x});

const code = 'Object.getOwnPropertyDescriptor(parent, "prop")';
const res = vm.runInContext(code, o, 'test');

console.log(res);

which logs:

{ get: [Function: get],
  set: undefined,
  enumerable: false,
  configurable: false }

@Trott
Copy link
Member

Trott commented Jul 16, 2017

Should this remain open? (I'm guessing the answer is "yes" but I'd be happy to find out I'm wrong.)

@TimothyGu
Copy link
Member

@Trott Yes, @domenic's error case is still valid.

@TimothyGu
Copy link
Member

In fact, we should take another look at this issue now that v8/v8@b0a7738 has been committed.

targos added a commit to targos/node that referenced this issue Sep 5, 2017
fhinkel added a commit to fhinkel/node that referenced this issue Oct 23, 2017
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
addaleax pushed a commit to ayojs/ayo that referenced this issue Oct 26, 2017
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this issue Dec 7, 2017
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vm Issues and PRs related to the vm subsystem.
Projects
None yet
7 participants