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

fix: potential async_hooks crash in NotifyWindowRestore on Windows #40576

Merged
merged 2 commits into from Jan 26, 2024

Conversation

codebytere
Copy link
Member

Description of Change

Closes #40573.

Fixes an issue where an async_hooks crash could occur on Windows only if the user is listening to the restore event after minimizing a maximized window and an error occurs in the restore callback. This was happening after 6b97beb, which created the first instance in our entire codebase where two Emit calls were called sequentially. As a result, given they were in the same callbackscope, when node::RunTimers ran and an error was occurring in the previous handler, async_hooks would blow up.

This fixes that error by putting the first Emit in its own CallbackScope.

Checklist

Release Notes

Notes: Fixed a potential async_hooks crash when listening for the restore event on Windows after minimizing a maximized BrowserWindow.

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/26-x-y PR should also be added to the "26-x-y" branch. target/27-x-y PR should also be added to the "27-x-y" branch. target/28-x-y PR should also be added to the "28-x-y" branch. labels Nov 21, 2023
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Nov 21, 2023
@codebytere codebytere requested review from jkleinsc and removed request for mlaurencin November 21, 2023 15:45
Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

Does it fix the problem if we wrap every Emit with a node::CallbackScope? i.e. Put the node::CallbackScope in CallMethodWithArgs at event_emitter_caller.cc.

Sequential emits is rare but it seems impossible to patch all corner cases, for example, the drag-ended and drop events of Tray emit in one tick:
https://github.com/electron/electron/blob/main/shell/browser/ui/tray_icon_cocoa.mm#L303-L309

So I would prefer a generic solution than patching each corner case.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Nov 22, 2023
@zcbenz
Copy link
Member

zcbenz commented Nov 29, 2023

The failing linux-x64-testing-asan-tests does not seem to be flaky.

@github-actions github-actions bot added the target/29-x-y PR should also be added to the "29-x-y" branch. label Dec 6, 2023
@codebytere codebytere force-pushed the fix-async-hook-crash branch 4 times, most recently from b171538 to ffb00ae Compare January 15, 2024 18:48
error->ToObject(isolate->GetCurrentContext()).ToLocalChecked(),
// Message is also emitted to keep compatibility with old code.
message);
std::vector<v8::Local<v8::Value>> args = {
Copy link
Member Author

Choose a reason for hiding this comment

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

autoUpdater is then only place in the codebase we use gin to emit a proper Error object. By using gin::EmitEvent to emit the Error object (thus adding a new CallbackScope there), and because Errors are handled specially in Node.js, we would otherwise trigger the uncaught exception handler to bypass the try/catch in our autoUpdate specs.

Either we add CallbackScopes on a case-by-case basis (as i initially tried in this PR) or we account for this edge case. I've chosen to account for this edge case.

@ckerr ckerr merged commit 8104c79 into main Jan 26, 2024
18 checks passed
@ckerr ckerr deleted the fix-async-hook-crash branch January 26, 2024 18:53
Copy link

release-clerk bot commented Jan 26, 2024

Release Notes Persisted

Fixed a potential async_hooks crash when listening for the restore event on Windows after minimizing a maximized BrowserWindow.

@trop
Copy link
Contributor

trop bot commented Jan 26, 2024

I was unable to backport this PR to "26-x-y" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Jan 26, 2024

I have automatically backported this PR to "27-x-y", please check out #41144

@trop trop bot added in-flight/27-x-y and removed target/27-x-y PR should also be added to the "27-x-y" branch. labels Jan 26, 2024
@trop
Copy link
Contributor

trop bot commented Jan 26, 2024

I have automatically backported this PR to "28-x-y", please check out #41145

@trop
Copy link
Contributor

trop bot commented Jan 26, 2024

I have automatically backported this PR to "29-x-y", please check out #41146

@trop trop bot added in-flight/28-x-y in-flight/29-x-y merged/27-x-y PR was merged to the "27-x-y" branch. merged/28-x-y PR was merged to the "28-x-y" branch. merged/29-x-y PR was merged to the "29-x-y" branch. and removed target/28-x-y PR should also be added to the "28-x-y" branch. target/29-x-y PR should also be added to the "29-x-y" branch. in-flight/27-x-y in-flight/28-x-y in-flight/29-x-y labels Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged/27-x-y PR was merged to the "27-x-y" branch. merged/28-x-y PR was merged to the "28-x-y" branch. merged/29-x-y PR was merged to the "29-x-y" branch. needs-manual-bp/26-x-y semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Async hook stack corruption in event callbacks with setTimeout
5 participants