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.

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

vm: properly support symbols on globals #46458

Merged
merged 3 commits into from
Feb 4, 2023

Conversation

dubzzz
Copy link
Contributor

@dubzzz dubzzz commented Feb 1, 2023

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

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. vm Issues and PRs related to the vm subsystem. labels Feb 1, 2023
@dubzzz
Copy link
Contributor Author

dubzzz commented Feb 1, 2023

I'll rework the commit message to fit the guidelines. Do I need to change the commit in-place and force push or can I push an extra commit on top of it?

@aduh95
Copy link
Contributor

aduh95 commented Feb 1, 2023

I'll rework the commit message to fit the guidelines. Do I need to change the commit in-place and force push or can I push an extra commit on top of it?

Force pushing is the only way in this instance, as we typically squash all the commits in the PR into the first one.

@dubzzz

This comment was marked as outdated.

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
@dubzzz
Copy link
Contributor Author

dubzzz commented Feb 1, 2023

Normally everything should be good now. As suggested by @aduh95, I force-pushed on my original commit to fix the commit message. Only changes made since the original commit (before force-push) are: edit commit message and fix lint on the new test file.

@aduh95
Copy link
Contributor

aduh95 commented Feb 1, 2023

This PR makes test/parallel/test-repl-underscore fail:

=== release test-repl-underscore ===
Path: parallel/test-repl-underscore
Error: --- stderr ---
node:assert:124
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected

  [
    "'baz'",
    'Expression assignment to _error now disabled.',
+   'Uncaught TypeError: Cannot redefine property: _error',
-   '0',
    'Uncaught Error: quux',
    '0'
  ]
    at assertOutput (/Users/runner/work/node/node/test/parallel/test-repl-underscore.js:246:10)
    at Immediate._onImmediate (/Users/runner/work/node/node/test/parallel/test-repl-underscore.js:214:5)
    at process.processImmediate (node:internal/timers:475:21)
    at process.topLevelDomainCallback (node:domain:161:15)
    at process.callbackTrampoline (node:internal/async_hooks:128:24) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: [
    "'baz'",
    'Expression assignment to _error now disabled.',
    'Uncaught TypeError: Cannot redefine property: _error',
    'Uncaught Error: quux',
    '0'
  ],
  expected: [
    "'baz'",
    'Expression assignment to _error now disabled.',
    '0',
    'Uncaught Error: quux',
    '0'
  ],
  operator: 'deepStrictEqual'
}

Node.js v20.0.0-pre
Command: out/Release/node /Users/runner/work/node/node/test/parallel/test-repl-underscore.js

===
=== 1 tests failed
===

Failed tests:
out/Release/node /Users/runner/work/node/node/test/parallel/test-repl-underscore.js

@dubzzz
Copy link
Contributor Author

dubzzz commented Feb 1, 2023

This PR makes test/parallel/test-repl-underscore fail

I'll rework it to make sure this test passes too. I may have missed one condition in the code I added.

@aduh95
Copy link
Contributor

aduh95 commented Feb 1, 2023

Can you get rid of the merge commit please? It tends to break our tooling. (git reset 020bc92a500df7e8e48b632aed011a06cac0f169 --hard && git cherry-pick dda9dc3c9a30fdc91e30d7f754191114fac94b9f && git push -f should do the trick).

@dubzzz
Copy link
Contributor Author

dubzzz commented Feb 1, 2023

Yes definitely I will

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Feb 2, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 2, 2023
@nodejs-github-bot
Copy link
Collaborator

@dubzzz
Copy link
Contributor Author

dubzzz commented Feb 2, 2023

Not sure to get the reason of the remaining failures 🤔 I started to check on jenkins side but I don't exactly get the reason of the failure and the link to the current change.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@dubzzz
Copy link
Contributor Author

dubzzz commented Feb 3, 2023

@aduh95 Not sure what to do for the nodejs-github-bot failures, I don't get exactly what is failing 🤯

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/49351/

@aduh95
Copy link
Contributor

aduh95 commented Feb 4, 2023

Not sure what to do for the nodejs-github-bot failures, I don't get exactly what is failing 🤯

Unfortunately sometimes the CI reports unrelated CI failures (either because the test is flaky, or because of a hardware/infra failure). Restarting the CI did not trigger the same issue twice, so it's fair to assume it was indeed unrelated :)

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Feb 4, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 4, 2023
@nodejs-github-bot nodejs-github-bot merged commit 6bbc2fb into nodejs:main Feb 4, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 6bbc2fb

@dubzzz
Copy link
Contributor Author

dubzzz commented Feb 4, 2023

As it fixes an issue in node 18, any way to put it in the next release of node 18 too?

@aduh95
Copy link
Contributor

aduh95 commented Feb 4, 2023

As it fixes an issue in node 18, any way to put it in the next release of node 18 too?

Before landing on Node.js 18.x, it will need to be released on Node.js 19.x for at least 2 weeks. The backport should be done by a releaser without any action from you after that, or if a manual backport is needed, they'll add a comment here.

## What can be backported?
The "Current" release line is much more lenient than the LTS release lines in
what can be landed. Our LTS release lines (see the [Release Plan][])
require that commits mature in the Current release for at least 2 weeks before
they can be landed in an LTS staging branch. Only after "maturation" will those
commits be cherry-picked or backported.

@dubzzz
Copy link
Contributor Author

dubzzz commented Feb 6, 2023

For the record, as some other regressions have been reported following #42963 (the original PR, I patched with this one), I'm trying to cover all of them via dedicated tests and possibly adapt again the proposed option (by adding comment).

dubzzz added a commit to dubzzz/node that referenced this pull request Feb 11, 2023
While it was supposed to fix most of the remaining issues,
nodejs#46458 missed some in strict mode.

This PR adds some additional checks. It also clarifies what we are
really checking to execute or not the `GetReturnValue`.
dubzzz added a commit to dubzzz/node that referenced this pull request Feb 11, 2023
While it was supposed to fix most of the remaining issues,
nodejs#46458 missed some in strict mode.

This PR adds some additional checks. It also clarifies what we are
really checking to execute or not the `GetReturnValue`.
@dubzzz
Copy link
Contributor Author

dubzzz commented Feb 11, 2023

I opened a follow-up PR that should fix the previously reported issues linked to 18.2.0 and 18.0.0. See #46615

MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
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>
@MylesBorins MylesBorins mentioned this pull request Feb 19, 2023
dubzzz added a commit to dubzzz/node that referenced this pull request Mar 9, 2023
While it was supposed to fix most of the remaining issues,
nodejs#46458 missed some in strict mode.

This PR adds some additional checks. It also clarifies what we are
really checking to execute or not the `GetReturnValue`.
nodejs-github-bot pushed a commit that referenced this pull request Mar 18, 2023
While it was supposed to fix most of the remaining issues,
#46458 missed some in strict mode.

This PR adds some additional checks. It also clarifies what we are
really checking to execute or not the `GetReturnValue`.

PR-URL: #46615
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Mar 18, 2023
While it was supposed to fix most of the remaining issues,
#46458 missed some in strict mode.

This PR adds some additional checks. It also clarifies what we are
really checking to execute or not the `GetReturnValue`.

PR-URL: #46615
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Mar 18, 2023
While it was supposed to fix most of the remaining issues,
#46458 missed some in strict mode.

This PR adds some additional checks. It also clarifies what we are
really checking to execute or not the `GetReturnValue`.

PR-URL: #46615
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
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>
MylesBorins pushed a commit that referenced this pull request Jun 23, 2023
While it was supposed to fix most of the remaining issues,
#46458 missed some in strict mode.

This PR adds some additional checks. It also clarifies what we are
really checking to execute or not the `GetReturnValue`.

PR-URL: #46615
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vm module regression in node 18.2.0: failing to list properties of an object
4 participants