vm: properly handle defining symbol props #47572

dubzzz · 2023-04-15T16:12:17Z

This PR is a follow-up of #46615.

When bumping V8 for node 18, various bugs appeared around vm. Some have been fixed along the flow, but there is at least one remaining and described at:
jestjs/jest#13338.

The current fix does nothing on node 20: the new bump of v8 done for node 20 seems to fix it. But it would fix the problem in both node 18 and node 19. I confirmed it would fix node 19 by cherry-picking the commit on v19.x-staging and launching make -j4 jstest on it.

Fixes #47717

This PR is a follow-up of nodejs#46615. When bumping V8 for node 18, various bugs appeared around vm. Some have been fixed along the flow, but there is at least one remaining and described at: jestjs/jest#13338. The current fix does nothing on node 20: the new bump of v8 done for node 20 seems to fix it. But it would fix the problem in both node 18 and node 19. I confirmed it would fix node 19 by cherry-picking the commit on v19.x-staging and launching `make -j4 jstest` on it.

targos · 2023-05-01T10:44:51Z

@nodejs/vm

dubzzz · 2023-05-09T22:52:35Z

@jasnell @targos Any blocker to merge it?

nodejs-github-bot · 2023-05-10T03:56:15Z

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

jasnell · 2023-05-10T03:56:23Z

Needs a CI run. Just started it.

dubzzz · 2023-05-10T05:57:52Z

Thank you so much

dubzzz · 2023-05-10T07:25:22Z

👀 It seems that the CI passed

dubzzz · 2023-05-15T06:49:23Z

@jasnell Do you think we can merge it?

dubzzz · 2023-05-20T17:15:34Z

@jasnell Sorry for the ping, it seems that everything is green. Looking forward to see this fix merged to fix a long running issue in Jest when used with Node 18 and 19

dubzzz · 2023-05-24T18:43:10Z

Should I rebase it or merge master into it? The branch has been opened 1 month+ ago and does not seem to conflict but I was wondering if it needed to be resynced at some time to re-run the tests against an updated version of master.

benjamingr · 2023-05-26T17:52:42Z

I added "dont-land-on-v20" given the openjs slack message but as a reviewer please guide what's right here @jasnell

nodejs-github-bot · 2023-05-26T17:57:50Z

Commit Queue failed

- Loading data for nodejs/node/pull/47572 ✔ Done loading data for nodejs/node/pull/47572 ----------------------------------- PR info ------------------------------------ Title vm: properly handle defining symbol props (#47572) Author Nicolas DUBIEN (@dubzzz) Branch dubzzz:fix-symbol-vm-node-master -> nodejs:main Labels c++, vm, needs-ci, dont-land-on-v20.x Commits 4 - vm: properly handle defining symbol props - share more across variants of the test - run assertions from the sandbox too - run even more assertions against sandbox Committers 1 - Nicolas DUBIEN PR-URL: https://github.com/nodejs/node/pull/47572 Reviewed-By: James M Snell ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/47572 Reviewed-By: James M Snell -------------------------------------------------------------------------------- ℹ This PR was created on Sat, 15 Apr 2023 16:12:17 GMT ✔ Approvals: 1 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/47572#pullrequestreview-1414068765 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2023-05-10T03:56:15Z: https://ci.nodejs.org/job/node-test-pull-request/51725/ - Querying data for job/node-test-pull-request/51725/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 47572 From https://github.com/nodejs/node * branch refs/pull/47572/merge -> FETCH_HEAD ✔ Fetched commits as 70da07595447..d108a9c6b972 -------------------------------------------------------------------------------- Auto-merging src/node_contextify.cc [main feeadaf4a8] vm: properly handle defining symbol props Author: Nicolas DUBIEN Date: Sat Apr 15 14:33:13 2023 +0000 3 files changed, 81 insertions(+), 26 deletions(-) create mode 100644 test/parallel/test-vm-global-get-own.js delete mode 100644 test/parallel/test-vm-global-symbol.js [main 56049eb66f] share more across variants of the test Author: Nicolas DUBIEN Date: Wed Apr 19 11:27:08 2023 +0000 1 file changed, 60 insertions(+), 63 deletions(-) [main 195e77b537] run assertions from the sandbox too Author: Nicolas DUBIEN Date: Wed Apr 19 11:42:21 2023 +0000 1 file changed, 85 insertions(+), 33 deletions(-) [main 71cd77fd40] run even more assertions against sandbox Author: Nicolas DUBIEN Date: Wed Apr 19 18:35:07 2023 +0000 1 file changed, 24 insertions(+), 48 deletions(-) ✔ Patches applied There are 4 commits in the PR. Attempting autorebase. Rebasing (2/8)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
vm: properly handle defining symbol props

This PR is a follow-up of #46615.

When bumping V8 for node 18, various bugs appeared around vm.
Some have been fixed along the flow, but there is at least one
remaining and described at:
jestjs/jest#13338.

The current fix does nothing on node 20: the new bump of v8 done for
node 20 seems to fix it. But it would fix the problem in both node 18
and node 19. I confirmed it would fix node 19 by cherry-picking the
commit on v19.x-staging and launching make -j4 jstest on it.

PR-URL: #47572
Reviewed-By: James M Snell jasnell@gmail.com

[detached HEAD e3989a52b7] vm: properly handle defining symbol props
Author: Nicolas DUBIEN github@dubien.org
Date: Sat Apr 15 14:33:13 2023 +0000
3 files changed, 81 insertions(+), 26 deletions(-)
create mode 100644 test/parallel/test-vm-global-get-own.js
delete mode 100644 test/parallel/test-vm-global-symbol.js
Rebasing (3/8)
Rebasing (4/8)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
share more across variants of the test

PR-URL: #47572
Reviewed-By: James M Snell jasnell@gmail.com

[detached HEAD 9d3829086b] share more across variants of the test
Author: Nicolas DUBIEN github@dubien.org
Date: Wed Apr 19 11:27:08 2023 +0000
1 file changed, 60 insertions(+), 63 deletions(-)
Rebasing (5/8)
Rebasing (6/8)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
run assertions from the sandbox too

PR-URL: #47572
Reviewed-By: James M Snell jasnell@gmail.com

[detached HEAD 0194c7f82d] run assertions from the sandbox too
Author: Nicolas DUBIEN github@dubien.org
Date: Wed Apr 19 11:42:21 2023 +0000
1 file changed, 85 insertions(+), 33 deletions(-)
Rebasing (7/8)
Rebasing (8/8)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
run even more assertions against sandbox

PR-URL: #47572
Reviewed-By: James M Snell jasnell@gmail.com

[detached HEAD 7229614bab] run even more assertions against sandbox
Author: Nicolas DUBIEN github@dubien.org
Date: Wed Apr 19 18:35:07 2023 +0000
1 file changed, 24 insertions(+), 48 deletions(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/5093621801

dubzzz · 2023-05-26T19:37:16Z

As asked on openjs slack, why don't we at least merge the test part on node 20? Maybe I don't get the meaning of the don't land in tag 🤔

nodejs-github-bot · 2023-05-26T19:37:38Z

Landed in 06a211b

This PR is a follow-up of #46615. When bumping V8 for node 18, various bugs appeared around vm. Some have been fixed along the flow, but there is at least one remaining and described at: jestjs/jest#13338. The current fix does nothing on node 20: the new bump of v8 done for node 20 seems to fix it. But it would fix the problem in both node 18 and node 19. I confirmed it would fix node 19 by cherry-picking the commit on v19.x-staging and launching `make -j4 jstest` on it. PR-URL: #47572 Reviewed-By: James M Snell <jasnell@gmail.com>

dubzzz · 2023-07-03T09:44:41Z

Any chance to have it in a patch of node 18?

This PR is a follow-up of #46615. When bumping V8 for node 18, various bugs appeared around vm. Some have been fixed along the flow, but there is at least one remaining and described at: jestjs/jest#13338. The current fix does nothing on node 20: the new bump of v8 done for node 20 seems to fix it. But it would fix the problem in both node 18 and node 19. I confirmed it would fix node 19 by cherry-picking the commit on v19.x-staging and launching `make -j4 jstest` on it. PR-URL: #47572 Reviewed-By: James M Snell <jasnell@gmail.com>

This PR is a follow-up of nodejs#46615. When bumping V8 for node 18, various bugs appeared around vm. Some have been fixed along the flow, but there is at least one remaining and described at: jestjs/jest#13338. The current fix does nothing on node 20: the new bump of v8 done for node 20 seems to fix it. But it would fix the problem in both node 18 and node 19. I confirmed it would fix node 19 by cherry-picking the commit on v19.x-staging and launching `make -j4 jstest` on it. PR-URL: nodejs#47572 Reviewed-By: James M Snell <jasnell@gmail.com>

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 Apr 15, 2023

dubzzz added 3 commits April 19, 2023 11:27

share more across variants of the test

d7fc35e

run assertions from the sandbox too

6ab652b

run even more assertions against sandbox

d108a9c

dubzzz mentioned this pull request Apr 25, 2023

Object.getOwnPropertySymbols not returning all symbols when using vm #47717

Closed

jasnell approved these changes May 5, 2023

View reviewed changes

dubzzz mentioned this pull request May 5, 2023

Setting __proto__ on vm context's globalThis causes a crash (regression in v19.9.0) #47798

Closed

benjamingr added commit-queue Add this label to land a pull request using GitHub Actions. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. labels May 26, 2023

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels May 26, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 26, 2023

nodejs-github-bot merged commit 06a211b into nodejs:main May 26, 2023
64 checks passed

benjamingr removed the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label May 26, 2023

targos mentioned this pull request Jun 4, 2023

v20.3.0 release proposal #48332

Merged

danielleadams mentioned this pull request Jul 10, 2023

v18.17.0 release proposal #48694

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vm: properly handle defining symbol props #47572

vm: properly handle defining symbol props #47572

dubzzz commented Apr 15, 2023 •

edited

targos commented May 1, 2023

dubzzz commented May 9, 2023

nodejs-github-bot commented May 10, 2023

jasnell commented May 10, 2023

dubzzz commented May 10, 2023

dubzzz commented May 10, 2023

dubzzz commented May 15, 2023

dubzzz commented May 20, 2023

dubzzz commented May 24, 2023

benjamingr commented May 26, 2023

nodejs-github-bot commented May 26, 2023

PR-URL: #47572
Reviewed-By: James M Snell jasnell@gmail.com

PR-URL: #47572
Reviewed-By: James M Snell jasnell@gmail.com

PR-URL: #47572
Reviewed-By: James M Snell jasnell@gmail.com

PR-URL: #47572
Reviewed-By: James M Snell jasnell@gmail.com

Successfully rebased and updated refs/heads/main.

dubzzz commented May 26, 2023 •

edited

nodejs-github-bot commented May 26, 2023

dubzzz commented Jul 3, 2023

vm: properly handle defining symbol props #47572

vm: properly handle defining symbol props #47572

Conversation

dubzzz commented Apr 15, 2023 • edited

targos commented May 1, 2023

dubzzz commented May 9, 2023

nodejs-github-bot commented May 10, 2023

jasnell commented May 10, 2023

dubzzz commented May 10, 2023

dubzzz commented May 10, 2023

dubzzz commented May 15, 2023

dubzzz commented May 20, 2023

dubzzz commented May 24, 2023

benjamingr commented May 26, 2023

nodejs-github-bot commented May 26, 2023

PR-URL: #47572 Reviewed-By: James M Snell jasnell@gmail.com

PR-URL: #47572 Reviewed-By: James M Snell jasnell@gmail.com

PR-URL: #47572 Reviewed-By: James M Snell jasnell@gmail.com

PR-URL: #47572 Reviewed-By: James M Snell jasnell@gmail.com

Successfully rebased and updated refs/heads/main.

dubzzz commented May 26, 2023 • edited

nodejs-github-bot commented May 26, 2023

dubzzz commented Jul 3, 2023

dubzzz commented Apr 15, 2023 •

edited

PR-URL: #47572
Reviewed-By: James M Snell jasnell@gmail.com

PR-URL: #47572
Reviewed-By: James M Snell jasnell@gmail.com

PR-URL: #47572
Reviewed-By: James M Snell jasnell@gmail.com

PR-URL: #47572
Reviewed-By: James M Snell jasnell@gmail.com

dubzzz commented May 26, 2023 •

edited