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: When applied result of vm.runInContext in 'Object.getOwnPropertyNames(result)' ignores its own property #3158

Closed
princejwesley opened this issue Oct 2, 2015 · 10 comments
Labels
confirmed-bug Issues with confirmed bugs. vm Issues and PRs related to the vm subsystem.

Comments

@princejwesley
Copy link
Contributor

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

const name = {
  firstName: 'prince'
};

const ctx = vm.createContext(name);
const result = vm.runInContext('this', ctx);
const ownProperties = Object.getOwnPropertyNames(result);

console.log(ownProperties.indexOf('firstName'));
console.log('result.firstName', result.firstName);
console.log("result.hasOwnProperty('firstName')", result.hasOwnProperty('firstName'));

gives

$ node vmIssue.js
-1
result.firstName prince
result.hasOwnProperty('firstName') true

In the above example, Object.getOwnPropertyNames(result) is not returning its own property.

It affects REPL auto-completion results when useGlobal is false.

@mscdex mscdex added the vm Issues and PRs related to the vm subsystem. label Oct 2, 2015
@bnoordhuis bnoordhuis added the confirmed-bug Issues with confirmed bugs. label Oct 2, 2015
@bnoordhuis
Copy link
Member

I can confirm the issue. I think it's because firstName lives on the global object's hidden prototype object, not the global object itself. Not sure how to fix that or even if we can or want to fix that.

/cc @domenic

@princejwesley
Copy link
Contributor Author

Object.keys() in the below code gives expected result.

const ownKeys = Object.keys(result);
console.log(ownKeys.indexOf('firstName'));  // 0

@bnoordhuis Object. getOwnPropertyNames() is a super set of Object.keys(), right?

@domenic
Copy link
Contributor

domenic commented Oct 2, 2015

Which Node version? We recently fixed something related in v3 I think.

The hidden prototype stuff should be unobservable IIRC, and the inconsistency implies something is going wrong here, so yeah, this should be fixable. Even if it's just a V8 issue.

One thing I've been meaning to investigate is using non-masking mode which might help with some of these issues...

@princejwesley
Copy link
Contributor Author

@domenic I have tested against v4.1.1

@bnoordhuis
Copy link
Member

Object. getOwnPropertyNames() is a super set of Object.keys(), right?

Implementation-wise it's nuanced. I did notice that V8's %OwnKeys() intrinsic enumerates named interceptors (that's how the global proxy object is implemented) whereas %GetOwnPropertyNames() doesn't seem to.

(It does have logic around named interceptors using %GetInterceptorInfo() but that returns zero a.k.a. 'no interceptors' . The net result is that ContextifyContext::GlobalPropertyEnumeratorCallback() is never called.)

@princejwesley
Copy link
Contributor Author

@bnoordhuis Is this same hidden prototype issue?
screen shot 2016-01-29 at 9 30 31 pm

@bnoordhuis
Copy link
Member

@princejwesley No, if I understand you right. Each VM context has its own instance of Object so an object from outside is never an instance of that context's Object.prototype (unless you start mucking about with Object.setPrototypeOf().)

Example:

> const o = {}
> Object.prototype.isPrototypeOf(o)
true

> vm.runInThisContext('Object.prototype.isPrototypeOf(o)')
true

> vm.runInContext('Object.prototype.isPrototypeOf(o)', vm.createContext({o:o}));
false

# and that's because...
> vm.runInContext('this.Object === that.Object', vm.createContext({that:this}))
false

And to be clear, when you call Object.prototype.isPrototypeOf(o), what you're asking is if o has Object.prototype in its prototype chain. It does when it's from the same context, it doesn't when it's from elsewhere.

@princejwesley
Copy link
Contributor Author

@bnoordhuis thanks, it make sense now.

@jasnell
Copy link
Member

jasnell commented Apr 2, 2016

@bnoordhuis ... is this actually a bug? this is definitely still happening but it's not clear if it's something that needs fixed.

@bnoordhuis
Copy link
Member

It's fixed in master thanks to a recent V8 upgrade. We'll be shipping that as v6.0.0 later this month so I'll go ahead and close the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants