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
fix: avoid using v8 on Isolate termination #35766
Conversation
5ac4f0f
to
8b3b7c4
Compare
34cf623
to
7ca3d50
Compare
7ca3d50
to
d72083b
Compare
I was cleaning my review comment and it went off 🔫 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if CI passes 👍
869878e
to
3ae2cd4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diff LGTM.
linux-x64-testing-node
has a few failures. DYK if these are relevant or if they're flakes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code lgtm, though setting only_terminate_in_safe_scope = false
would seem to suggest that in node mode we are okay to terminate in an unsafe scope...? what are the implications of that?
https://source.chromium.org/chromium/chromium/src/+/main:v8/include/v8-isolate.h;l=279-282;drc=cc40beb19a2be3cf1840576b75be5ebbd818c66d
also, do we need to do this for unsandboxed renderers too?
@nornagon i'd say we don't need to do this for unsandboxed renderers, since imo we should be staying in alignment with gin defaults for all other processes. Part of the reason we're doing this in this PR is because Node.js uses nested microtasks to handle certain operations in a way that Chromium never does - I've already commented upstream (to Node.js) about this in terms of whether it's something that can be fixed there. |
The linux-x64-testing-node CI failure does not disappear after a few reruns. |
077724b
to
c4f70f2
Compare
No Release Notes |
I was unable to backport this PR to "22-x-y" cleanly; |
I was unable to backport this PR to "21-x-y" cleanly; |
I was unable to backport this PR to "23-x-y" cleanly; |
* fix: avoid using v8 on Isolate termination * chore: refactor for review --------- Co-authored-by: electron-patch-conflict-fixer[bot] <83340002+electron-patch-conflict-fixer[bot]@users.noreply.github.com>
* fix: avoid using v8 on Isolate termination * chore: refactor for review --------- Co-authored-by: electron-patch-conflict-fixer[bot] <83340002+electron-patch-conflict-fixer[bot]@users.noreply.github.com>
@codebytere has manually backported this PR to "23-x-y", please check out #38000 |
Description of Change
Removes our patches disabling the Chromium check added in CL:3620285.
We also set
only_terminate_in_safe_scope
tofalse
inv8::Isolate::CreateParams
- gin enables it by default but Node.js does not. This can cause crashes owing to Node.js' unique usage of nested microtasks and possible termination. We disable this to align more closely with Node.js and leave it enabled in the renderer and browser processes.Checklist
npm test
passesRelease Notes
Notes: none