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

Object.defineProperty called inside the vm context fails to copy data properties onto the sandbox #10977

Closed
AnnaMag opened this issue Jan 24, 2017 · 8 comments

Comments

@AnnaMag
Copy link
Member

AnnaMag commented Jan 24, 2017

  • Version: current master
  • Platform: OS X
  • Subsystem: vm

Data properties defined with the Object.defineProperty call inside the vm context are not copied onto the sandbox in the current master.

Test:

'use strict';

require('../common');
var vm = require('vm');
const util = require('util');

const sandbox = {};
const context = vm.createContext(sandbox);

const code = `
   Object.defineProperty(this, "foo", {value: 5});
`;

const res = vm.runInContext(code, context);

console.log(util.inspect(sandbox)); // returns: {}

In v6.2.0 (homebrew installation):

> node test_setter.js
{ foo: 5 }

Debugging the core shows failure in GlobalPropertySetterCallback.

ctx->sandbox()->Set(property, value);

does not set 'foo' on the sandbox (property and value are as expected). 'foo' is present on the global object, but also not returned by

Local<Array> names = global->GetOwnPropertyNames(context).ToLocalChecked();

in CopyProperties.

@AnnaMag
Copy link
Member Author

AnnaMag commented Jan 24, 2017

I think it is a good idea to add the test to the known_issues.

@fhinkel

@jasnell
Copy link
Member

jasnell commented Jan 24, 2017

Agree. A PR would be wonderful :-)

@fhinkel
Copy link
Member

fhinkel commented Jan 24, 2017

On a side note, if you want to test different node versions, you probably want nvm.

@bnoordhuis
Copy link
Member

#10920 fixes this.

@AnnaMag
Copy link
Member Author

AnnaMag commented Jan 24, 2017

No need for a PR then, am I right?

@bnoordhuis
Copy link
Member

Not for this issue but #10920 will reintroduce #10223 and that will need to be fixed eventually. It's yours if you want it.

@AnnaMag
Copy link
Member Author

AnnaMag commented Jan 24, 2017

I'm working with the V8 5.5. API (removing CopyProperties). #10223 should be fixed once 5.5. lands in master and we can send PRs with the refactored code. Do you agree @fhinkel?

@fhinkel
Copy link
Member

fhinkel commented Jan 24, 2017

I think it's good to have the test. Either as known issue or as a regression test.

@AnnaMag AnnaMag closed this as completed Jan 24, 2017
@fhinkel fhinkel reopened this Jan 24, 2017
@fhinkel fhinkel closed this as completed Jan 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants