Node: Object.defineProperty called inside the vm context fails to copy data properties onto the sandbox

[AnnaMag]

• 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]

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

@fhinkel

[jasnell]

Agree. A PR would be wonderful :-)

[fhinkel]

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

[bnoordhuis]

10920 fixes this.

[AnnaMag]

No need for a PR then, am I right?

[bnoordhuis]

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]

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]

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