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 crash on macOS app exit #29941

Merged
merged 1 commit into from Jun 30, 2021
Merged

Conversation

codebytere
Copy link
Member

Description of Change

Closes #29260.

Fixes an issue where Electron could crash after calling app.quit() on macOS. This can happen as a result of [NSEvent removeMonitor:](https://developer.apple.com/documentation/appkit/nsevent/1533709-removemonitor?language=objc) being called twice; from the documentation discussion:

You must ensure that eventMonitor is removed only once. Removing the same eventMonitor instance multiple times results in an over-release condition, even in a Garbage Collected environment

This fixes any potential issues by ensuring that we only call [NSEvent removeMonitor:] on a non-nil id. Chromium uses a similar approach here.

Checklist

Release Notes

Notes: Fixes a potential crash when calling app.quit() on macOS.

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/12-x-y labels Jun 29, 2021
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 29, 2021
@@ -415,7 +415,7 @@ void ViewDidMoveToSuperview(NSView* self, SEL _cmd) {
// Use an NSEvent monitor to listen for the wheel event.
BOOL __block began = NO;
wheel_event_monitor_ = [NSEvent
addLocalMonitorForEventsMatchingMask:NSScrollWheelMask
Copy link
Member Author

Choose a reason for hiding this comment

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

this is deprecated in favor of NSEventMaskScrollWheel

Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

nice!

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 30, 2021
@codebytere codebytere merged commit 372ecf3 into main Jun 30, 2021
@codebytere codebytere deleted the fix-event-monitor-crash branch June 30, 2021 18:12
@release-clerk
Copy link

release-clerk bot commented Jun 30, 2021

Release Notes Persisted

Fixes a potential crash when calling app.quit() on macOS.

@trop
Copy link
Contributor

trop bot commented Jun 30, 2021

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

@trop
Copy link
Contributor

trop bot commented Jun 30, 2021

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

@trop trop bot removed the target/13-x-y label Jun 30, 2021
@trop
Copy link
Contributor

trop bot commented Jun 30, 2021

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

@trop
Copy link
Contributor

trop bot commented Jul 14, 2021

@miniak has manually backported this PR to "12-x-y", please check out #30138

zcbenz pushed a commit that referenced this pull request Jul 15, 2021
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
BlackHole1 pushed a commit to BlackHole1/electron that referenced this pull request Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Crash on app.exit()
6 participants