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

lib: do not crash using workers with disabled shared array buffers #41023

Merged

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Nov 29, 2021

This allows the repl to function normally while using the
--no-harmony-sharedarraybuffer V8 flag.

Fixes: #39717

Signed-off-by: Ruben Bridgewater [email protected]

@BridgeAR BridgeAR requested a review from addaleax November 29, 2021 21:01
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support. labels Nov 29, 2021
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

It would be great if you could add a comment to these explaining why it's necessary since I don't think it's going to be obvious to someone reading the code. Especially the first occurrence that does not reference SAB directly.

@BridgeAR
Copy link
Member Author

I missed the prior work by @codebytere #39718. I might also have found the issue that blocked the PR from being merged. I'll try that later on.

@BridgeAR BridgeAR added the blocked PRs that are blocked by other issues or PRs. label Nov 30, 2021
@BridgeAR BridgeAR force-pushed the do-not-crash-with-disabled-shared-array-buffer branch 3 times, most recently from 73059c2 to a39f548 Compare December 4, 2021 01:23
@BridgeAR BridgeAR added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 6, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 6, 2021
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR force-pushed the do-not-crash-with-disabled-shared-array-buffer branch from a39f548 to f26a4d0 Compare December 8, 2021 18:40
@BridgeAR BridgeAR added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 8, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 8, 2021
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR removed the blocked PRs that are blocked by other issues or PRs. label Dec 11, 2021
@BridgeAR
Copy link
Member Author

I unblocked this again to move it forward. @jasnell @tniessen are you fine landing this instead of waiting for #39718?

This allows the repl to function normally while using the
`--no-harmony-sharedarraybuffer` V8 flag.
It also fixes using workers while using the
`--no-harmony-atomics` V8 flag.

Fixes: nodejs#39717

Signed-off-by: Ruben Bridgewater <[email protected]>

Co-authored-by: Shelley Vohr <[email protected]>
@BridgeAR BridgeAR force-pushed the do-not-crash-with-disabled-shared-array-buffer branch from 9c2ceb6 to ed3865f Compare February 17, 2023 18:29
@BridgeAR BridgeAR added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Feb 17, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 17, 2023
@nodejs-github-bot
Copy link
Collaborator

@codebytere
Copy link
Member

@BridgeAR is this still something you want to move forward? We still float the patch so it's definitely still useful to Electron!

@BridgeAR
Copy link
Member Author

@codebytere I just rebased the branch and started the CI to land this :)

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 18, 2023
@nodejs-github-bot 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 Feb 18, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/41023
✔  Done loading data for nodejs/node/pull/41023
----------------------------------- PR info ------------------------------------
Title      lib: do not crash using workers with disabled shared array buffers (#41023)
Author     Ruben Bridgewater  (@BridgeAR)
Branch     BridgeAR:do-not-crash-with-disabled-shared-array-buffer -> nodejs:main
Labels     author ready, worker, needs-ci
Commits    1
 - lib: do not crash using workers with disabled shared array buffers
Committers 1
 - Ruben Bridgewater 
PR-URL: https://github.com/nodejs/node/pull/41023
Fixes: https://github.com/nodejs/node/issues/39717
Reviewed-By: James M Snell 
Reviewed-By: Tobias Nießen 
Reviewed-By: Antoine du Hamel 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/41023
Fixes: https://github.com/nodejs/node/issues/39717
Reviewed-By: James M Snell 
Reviewed-By: Tobias Nießen 
Reviewed-By: Antoine du Hamel 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Mon, 29 Nov 2021 21:01:08 GMT
   ✔  Approvals: 3
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/41023#pullrequestreview-818336803
   ✔  - Tobias Nießen (@tniessen) (TSC): https://github.com/nodejs/node/pull/41023#pullrequestreview-818394046
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/41023#pullrequestreview-1304031055
   ✖  Last GitHub CI failed
   ℹ  Last Full PR CI on 2023-02-18T08:32:07Z: https://ci.nodejs.org/job/node-test-pull-request/49675/
- Querying data for job/node-test-pull-request/49675/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/4210704839

@aduh95 aduh95 merged commit fbd55a8 into nodejs:main Feb 18, 2023
@aduh95
Copy link
Contributor

aduh95 commented Feb 18, 2023

Landed in fbd55a8

MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
This allows the repl to function normally while using the
`--no-harmony-sharedarraybuffer` V8 flag.
It also fixes using workers while using the
`--no-harmony-atomics` V8 flag.

Fixes: #39717

Signed-off-by: Ruben Bridgewater <[email protected]>

Co-authored-by: Shelley Vohr <[email protected]>
PR-URL: #41023
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 19, 2023
MylesBorins pushed a commit that referenced this pull request Feb 20, 2023
This allows the repl to function normally while using the
`--no-harmony-sharedarraybuffer` V8 flag.
It also fixes using workers while using the
`--no-harmony-atomics` V8 flag.

Fixes: #39717

Signed-off-by: Ruben Bridgewater <[email protected]>

Co-authored-by: Shelley Vohr <[email protected]>
PR-URL: #41023
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
This allows the repl to function normally while using the
`--no-harmony-sharedarraybuffer` V8 flag.
It also fixes using workers while using the
`--no-harmony-atomics` V8 flag.

Fixes: #39717

Signed-off-by: Ruben Bridgewater <[email protected]>

Co-authored-by: Shelley Vohr <[email protected]>
PR-URL: #41023
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
codebytere added a commit to electron/electron that referenced this pull request Apr 14, 2023
codebytere added a commit to electron/electron that referenced this pull request Apr 17, 2023
codebytere added a commit to electron/electron that referenced this pull request Apr 18, 2023
* chore: bump node in DEPS to v18.16.0

* build,test: add proper support for IBM i

nodejs/node#46739

* lib: enforce use of trailing commas

nodejs/node#46881

* src: add initial support for single executable applications

nodejs/node#45038

* lib: do not crash using workers with disabled shared array buffers

nodejs/node#41023

* src: remove shadowed variable in OptionsParser::Parse

nodejs/node#46672

* src: allow embedder control of code generation policy

nodejs/node#46368

* src: allow optional Isolate termination in node::Stop()

nodejs/node#46583

* lib: fix BroadcastChannel initialization location

nodejs/node#46864

* chore: fixup patch indices

* chore: sync filenames.json

* fix: add simdutf dep to src/inspector BUILD.gn

- nodejs/node#46471
- nodejs/node#46472

* deps: replace url parser with Ada

nodejs/node#46410

* tls: support automatic DHE

nodejs/node#46978

* fixup! src: add initial support for single executable applications

* http: unify header treatment

nodejs/node#46528

* fix: libc++ buffer overflow in string_view ctor

nodejs/node#46410

* test: include strace openat test

nodejs/node#46150

* fixup! fixup! src: add initial support for single executable applications

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <[email protected]>
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. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Repl crash when SharedArrayBuffers are disabled
6 participants