Skip to content

Commit

Permalink
vm: properly support symbols on globals
Browse files Browse the repository at this point in the history
A regression has been introduced in node 18.2.0, it lakes the following snippet fails while it used to work in the past:

```
const assert = require('assert');
const vm = require('vm');
const global = vm.runInContext('this', vm.createContext());
const totoSymbol = Symbol.for('toto');
Object.defineProperty(global, totoSymbol, {
  enumerable: true,
  writable: true,
  value: 4,
  configurable: true,
});
assert(Object.getOwnPropertySymbols(global).includes(totoSymbol));
```

The PR that introduced the regression is: #42963. So I basically attempted to start understanding what it changed to make it still fix the initial issue while not breaking the symbol related one.

Fixes: #45983
  • Loading branch information
dubzzz committed Feb 1, 2023
1 parent 9e46e0b commit 873c416
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 1 deletion.
4 changes: 3 additions & 1 deletion src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -530,7 +530,9 @@ void ContextifyContext::PropertySetterCallback(
return;

USE(ctx->sandbox()->Set(context, property, value));
args.GetReturnValue().Set(value);
if (is_function) {
args.GetReturnValue().Set(value);
}
}

// static
Expand Down
14 changes: 14 additions & 0 deletions test/parallel/test-vm-global-symbol.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
'use strict';
const assert = require('assert');
const vm = require('vm');

const global = vm.runInContext('this', vm.createContext());
const totoSymbol = Symbol.for('toto');
Object.defineProperty(global, totoSymbol, {
enumerable: true,
writable: true,
value: 4,
configurable: true,
});
assert(global[totoSymbol] === 4);
assert(Object.getOwnPropertySymbols(global).includes(totoSymbol));

0 comments on commit 873c416

Please sign in to comment.