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.

Sign up for GitHub

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

Jump to bottom

vm module regression in node 18.2.0: failing to list properties of an object #45983

Closed
dubzzz opened this issue Dec 26, 2022 · 1 comment · Fixed by #46458
Closed

vm module regression in node 18.2.0: failing to list properties of an object #45983

dubzzz opened this issue Dec 26, 2022 · 1 comment · Fixed by #46458

Comments

@dubzzz
Copy link
Contributor

dubzzz commented Dec 26, 2022

Version

18.2.0

Platform

Linux codespaces-828c09 5.4.0-1094-azure #100~18.04.1-Ubuntu SMP Mon Oct 17 11:44:30 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

While trying to understand a bug in Jest (see jestjs/jest#13338) that started to appear on node 18.2.0 (worked on node 18.1.0), I fall onto the issue #42962 and its fix #42963.

So I played a bit with vm and it turns out that the code below worked in node 18.1.0 while it started to fail on node 18.2.0. This code is close to what Jest does internally so it might explain the issue observed on Jest's side (see code in Jest https://github.com/facebook/jest/blob/fb2de8a10f8e808b080af67aa771f67b5ea537ce/packages/jest-environment-node/src/index.ts#L72).

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

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior?

The code should pass.

What do you see instead?

The code crashes.

Additional information

Just ran some additional manual tries:

// after the defineProperty
console.log(Object.getOwnPropertySymbols(myglobal)); // []
console.log(myglobal[totoSymbol]); // 4
This was referenced Dec 26, 2022
Closed
Merged
@dubzzz
Copy link
Contributor Author

dubzzz commented Jan 25, 2023 •
edited
Loading

@joyeecheung @aduh95 as you both approved the PR possibly causing the behaviour change, I wanted to ping you so that you can confirm me if the bug report is really related to an unexpected regression or if it is a suitable behaviour.

Sorry for the direct ping, I was mostly looking to have a status for the Jest related issue

dubzzz added a commit to dubzzz/node that referenced this issue Feb 1, 2023
@dubzzz
vm: properly support symbols on globals
873c416 
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: nodejs#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: nodejs#45983
@dubzzz dubzzz mentioned this issue Feb 1, 2023
Merged
dubzzz added a commit to dubzzz/node that referenced this issue Feb 1, 2023
@dubzzz
vm: properly support symbols on globals
0599236 
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: nodejs#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: nodejs#45983
nodejs-github-bot pushed a commit that referenced this issue Feb 4, 2023
@dubzzz
vm: properly support symbols on globals
6bbc2fb 
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this issue Feb 18, 2023
@dubzzz @MylesBorins
vm: properly support symbols on globals
17b3ee3 
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
danielleadams pushed a commit that referenced this issue Apr 11, 2023
@dubzzz @danielleadams
vm: properly support symbols on globals
b801873 
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

vm: properly support symbols on globals dubzzz/node
1 participant
@dubzzz