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

bootstrap: polyfill Symbol.dispose/Symbol.asyncDispose with correct descriptor #48703

Merged
merged 1 commit into from
Jul 12, 2023
Merged

bootstrap: polyfill Symbol.dispose/Symbol.asyncDispose with correct descriptor #48703

merged 1 commit into from
Jul 12, 2023

Conversation

ljharb
Copy link
Member

@ljharb ljharb commented Jul 8, 2023 •
edited
Loading

Followup to #48518; fixes #48699

Happy to add a test if needed - but I both wasn't sure where it would go, and I also wasn't sure if this was worth testing since node rarely polyfills JS builtins and v8 will supply it soon enough anyways (backed by test262)

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Jul 8, 2023
@ljharb ljharb mentioned this pull request Jul 8, 2023
Closed
@ljharb ljharb force-pushed the symbol-descriptors branch 2 times, most recently from d8e640c to c70ae63 Compare July 8, 2023 19:09
@ljharb
Copy link
Member Author

ljharb commented Jul 9, 2023

I'm assuming this Mac OS test is flaky since it timed out - could someone rerun that one if needed?

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 9, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 9, 2023
@nodejs-github-bot
Copy link
Collaborator

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

benjamingr
benjamingr reviewed Jul 9, 2023
View reviewed changes
lib/internal/process/pre_execution.js
@@ -128,9 +128,26 @@ function prepareExecution(options) {
function setupSymbolDisposePolyfill() {
// TODO(MoLow): Remove this polyfill once Symbol.dispose and Symbol.asyncDispose are available in V8.
// eslint-disable-next-line node-core/prefer-primordials
Symbol.dispose ??= SymbolDispose;
if (typeof Symbol.dispose !== 'symbol') {
Copy link
Member

@benjamingr benjamingr Jul 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're guaranteed this runs before any userland code right?

(Just making sure this is safe since it's weird we set up Symbol.dispose here (if someone tampered with symbol they can set up a getter that throws here) but we use ObjectDefineProperty which is a primordial that guards against this sort of tampering))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m assuming that the file called “pre execution” runs before any userland code - if not, this polyfill is highly problematic conceptually :-)

@ljharb
Copy link
Member Author

ljharb commented Jul 11, 2023

Are these 3 failing tests something I need to address?

@nodejs-github-bot
Copy link
Collaborator

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

@MoLow
Copy link
Member

MoLow commented Jul 11, 2023

Are these 3 failing tests something I need to address?

seems like a fluke, I reran the tests

@ljharb
Copy link
Member Author

ljharb commented Jul 12, 2023

@MoLow still failing; are they flaky or do i need to look into it?

@nodejs-github-bot
Copy link
Collaborator

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

@MoLow
Copy link
Member

MoLow commented Jul 12, 2023

@MoLow still failing; are they flaky or do i need to look into it?

passed now

@MoLow MoLow added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 12, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 12, 2023
@nodejs-github-bot nodejs-github-bot merged commit c2c7260 into nodejs:main Jul 12, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in c2c7260

@ljharb ljharb deleted the symbol-descriptors branch July 12, 2023 12:02
juanarbol pushed a commit that referenced this pull request Jul 13, 2023
@ljharb @juanarbol
bootstrap: use correct descriptor for Symbol.{dispose,asyncDispose}
eb0aba5 
Followup to #48518; fixes #48699

PR-URL: #48703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@juanarbol juanarbol mentioned this pull request Jul 13, 2023
Merged
@github-actions github-actions bot mentioned this pull request Jul 16, 2023
Open
23 tasks
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@ljharb @Ceres6
bootstrap: use correct descriptor for Symbol.{dispose,asyncDispose}
8396660 
Followup to nodejs#48518; fixes nodejs#48699

PR-URL: nodejs#48703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@ljharb @Ceres6
bootstrap: use correct descriptor for Symbol.{dispose,asyncDispose}
9c52644 
Followup to nodejs#48518; fixes nodejs#48699

PR-URL: nodejs#48703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@ruyadorno
Copy link
Member

This commit does not land cleanly on v18.x-staging and will need manual backport in case we want it in v18.

@ruyadorno ruyadorno added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Sep 11, 2023
@ljharb
Copy link
Member Author

ljharb commented Sep 11, 2023

@ruyadorno node 18 doesn't seem to have Symbol.dispose polyfilled, so i think this PR should have "do not backport" to <= 18?

@SimenB
Copy link
Member

SimenB commented Sep 19, 2023

node 18.18 was released today with the symbols, so maybe?

@ljharb
Copy link
Member Author

ljharb commented Sep 19, 2023

oof, ok then perhaps it will indeed backport cleanly into 18.18 :-)

targos pushed a commit that referenced this pull request Nov 27, 2023
@ljharb @targos
bootstrap: use correct descriptor for Symbol.{dispose,asyncDispose}
e08fd98 
Followup to #48518; fixes #48699

PR-URL: #48703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@targos targos mentioned this pull request Nov 28, 2023
Merged
@mhofman mhofman mentioned this pull request Dec 4, 2023
Closed
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
bootstrap: use correct descriptor for
8320281 
Followup to #48518; fixes #48699

PR-URL: nodejs/node#48703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
bootstrap: use correct descriptor for
80fb5b8 
Followup to #48518; fixes #48699

PR-URL: nodejs/node#48703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjamingr benjamingr benjamingr approved these changes

@MoLow MoLow MoLow approved these changes

@lpinca lpinca lpinca approved these changes

@Linkgoron Linkgoron Linkgoron approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Symbol.{ dispose, asyncDispose } have incorrect descriptors
8 participants
@ljharb @nodejs-github-bot @MoLow @ruyadorno @SimenB @Linkgoron @benjamingr @lpinca