Skip to content

Commit 17b3ee3

Browse files
dubzzzMylesBorins
authored andcommitted
vm: properly support symbols on globals
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>
1 parent 731a7ae commit 17b3ee3

File tree

2 files changed

+18
-1
lines changed

2 files changed

+18
-1
lines changed

src/node_contextify.cc

+3-1
Original file line numberDiff line numberDiff line change
@@ -528,7 +528,9 @@ void ContextifyContext::PropertySetterCallback(
528528
return;
529529

530530
USE(ctx->sandbox()->Set(context, property, value));
531-
args.GetReturnValue().Set(value);
531+
if (is_contextual_store || is_function) {
532+
args.GetReturnValue().Set(value);
533+
}
532534
}
533535

534536
// static
+15
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
'use strict';
2+
require('../common');
3+
const assert = require('assert');
4+
const vm = require('vm');
5+
6+
const global = vm.runInContext('this', vm.createContext());
7+
const totoSymbol = Symbol.for('toto');
8+
Object.defineProperty(global, totoSymbol, {
9+
enumerable: true,
10+
writable: true,
11+
value: 4,
12+
configurable: true,
13+
});
14+
assert.strictEqual(global[totoSymbol], 4);
15+
assert.ok(Object.getOwnPropertySymbols(global).includes(totoSymbol));

0 commit comments

Comments
 (0)