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 makes 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));
```

Regression introduced by: #42963.
So I basically attempted to start understanding what it changed to make
it fix the initial issue while not breaking the symbol related one.

Fixes: #45983
PR-URL: #46458
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
  • Loading branch information
dubzzz authored and MylesBorins committed Feb 18, 2023
1 parent 731a7ae commit 17b3ee3
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 1 deletion.
4 changes: 3 additions & 1 deletion src/node_contextify.cc
Expand Up @@ -528,7 +528,9 @@ void ContextifyContext::PropertySetterCallback(
return;

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

// static
Expand Down
15 changes: 15 additions & 0 deletions test/parallel/test-vm-global-symbol.js
@@ -0,0 +1,15 @@
'use strict';
require('../common');
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.strictEqual(global[totoSymbol], 4);
assert.ok(Object.getOwnPropertySymbols(global).includes(totoSymbol));

0 comments on commit 17b3ee3

Please sign in to comment.