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 calling tray.popUpContextMenu() #39231

Merged
merged 1 commit into from Aug 1, 2023

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Jul 25, 2023

Description of Change

Speculative fix for a crash i've been seeing in CI a fair bit recently, but haven't been able to repro locally. It looks like the ElectronMenuModel is going away before this callback is called. We use base::Unretained here which isn't safe if we can't guarantee lifetime which I don't think we can here.

Sample Failure

Stacktrace

Received signal 11 SEGV_MAPERR 000000000018
0   Electron Framework                  0x0000000129ff4632 base::debug::CollectStackTrace(void**, unsigned long) + 18
1   Electron Framework                  0x0000000129fe2393 base::debug::StackTrace::StackTrace() + 19
2   Electron Framework                  0x0000000129ff4581 base::debug::(anonymous namespace)::StackDumpSignalHandler(int, __siginfo*, void*) + 1361
3   libsystem_platform.dylib            0x00007ff8126dedfd _sigtramp + 29
4   ???                                 0x00006000023fd380 0x0 + 105553154003840
5   Electron Framework                  0x0000000129bcc352 base::internal::Invoker<base::internal::BindState<void (electron::TrayIconCocoa::*)(electron::ElectronMenuModel*), base::WeakPtr<electron::TrayIconCocoa>, base::internal::UnretainedWrapper<electron::ElectronMenuModel, base::unretained_traits::MayNotDangle, (base::RawPtrTraits)0>>, void ()>::RunOnce(base::internal::BindStateBase*) + 98
6   Electron Framework                  0x0000000129f66dec base::TaskAnnotator::RunTaskImpl(base::PendingTask&) + 300
7   Electron Framework                  0x0000000129f96f17 base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWorkImpl(base::LazyNow*) + 1655
8   Electron Framework                  0x0000000129f962e4 base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWork() + 132
9   Electron Framework                  0x0000000129f97ae5 non-virtual thunk to base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWork() + 21
10  Electron Framework                  0x0000000129ffcfbc base::MessagePumpCFRunLoopBase::RunWork() + 204
11  Electron Framework                  0x0000000129ffce93 ___ZN4base24MessagePumpCFRunLoopBase19RunDelayedWorkTimerEP16__CFRunLoopTimerPv_block_invoke + 51
12  Electron Framework                  0x0000000129ffbe32 base::mac::CallWithEHFrame(void () block_pointer) + 10
13  Electron Framework                  0x0000000129ffc42f base::MessagePumpCFRunLoopBase::RunDelayedWorkTimer(__CFRunLoopTimer*, void*) + 63
14  CoreFoundation                      0x00007ff8127a8f69 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20
15  CoreFoundation                      0x00007ff8127a8a58 __CFRunLoopDoTimer + 923
16  CoreFoundation                      0x00007ff8127a85c8 __CFRunLoopDoTimers + 307
17  CoreFoundation                      0x00007ff81278ed06 __CFRunLoopRun + 2010
18  CoreFoundation                      0x00007ff81278de6c CFRunLoopRunSpecific + 562
19  HIToolbox                           0x00007ff81b43c5e6 RunCurrentEventLoopInMode + 292
20  HIToolbox                           0x00007ff81b43c34a ReceiveNextEventCommon + 594
21  HIToolbox                           0x00007ff81b43c0e5 _BlockUntilNextEventMatchingListInModeWithFilter + 70
22  AppKit                              0x00007ff8151c7fad _DPSNextEvent + 927
23  AppKit                              0x00007ff8151c666a -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 1394
24  AppKit                              0x00007ff8151b8d19 -[NSApplication run] + 586
25  Electron Framework                  0x0000000129ffdacf base::MessagePumpNSApplication::DoRun(base::MessagePump::Delegate*) + 511
26  Electron Framework                  0x0000000129ffbec3 base::MessagePumpCFRunLoopBase::Run(base::MessagePump::Delegate*) + 131
27  Electron Framework                  0x0000000129f9806b base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::Run(bool, base::TimeDelta) + 539
28  Electron Framework                  0x0000000129f40925 base::RunLoop::Run(base::Location const&) + 437
29  Electron Framework                  0x000000012878cdc2 content::BrowserMainLoop::RunMainMessageLoop() + 146
30  Electron Framework                  0x000000012878f195 content::BrowserMainRunnerImpl::Run() + 37
31  Electron Framework                  0x0000000128789c1a content::BrowserMain(content::MainFunctionParams) + 154
32  Electron Framework                  0x000000012466951c content::RunBrowserProcessMain(content::MainFunctionParams, content::ContentMainDelegate*) + 284
33  Electron Framework                  0x000000012466aa97 content::ContentMainRunnerImpl::RunBrowser(content::MainFunctionParams, bool) + 631
34  Electron Framework                  0x000000012466a678 content::ContentMainRunnerImpl::Run() + 824
35  Electron Framework                  0x0000000124668a3e content::RunContentProcess(content::ContentMainParams, content::ContentMainRunner*) + 1454
36  Electron Framework                  0x0000000124668d03 content::ContentMain(content::ContentMainParams) + 115
37  Electron Framework                  0x00000001241eeaf0 ElectronMain + 160
38  dyld                                0x000000010b08352e start + 462
[end of stack trace]
✗ Electron tests failed with kill signal SIGSEGV.

Checklist

Release Notes

Notes: Fixed a potential crash when calling tray.popUpContextMenu on macOS.

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/24-x-y PR should also be added to the "24-x-y" branch. target/25-x-y PR should also be added to the "25-x-y" branch. target/26-x-y PR should also be added to the "26-x-y" branch. labels Jul 25, 2023
@codebytere codebytere requested review from ckerr and zcbenz July 25, 2023 19:13
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jul 25, 2023
@codebytere codebytere marked this pull request as draft July 25, 2023 19:34
@codebytere codebytere force-pushed the maybe-fix-menu-crash branch 4 times, most recently from 8a961ff to 0234c47 Compare July 26, 2023 14:44
@codebytere codebytere marked this pull request as ready for review July 26, 2023 16:32
@codebytere codebytere marked this pull request as draft July 27, 2023 10:19
@codebytere codebytere force-pushed the maybe-fix-menu-crash branch 2 times, most recently from ff08197 to 770bdff Compare July 27, 2023 10:23
@codebytere codebytere marked this pull request as ready for review July 27, 2023 15:01
@codebytere codebytere changed the title fix: potential crash calling tray.popUpContextMenu fix: potential crash calling tray.popUpContextMenu() Jul 28, 2023
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jul 28, 2023
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.

Looks good to me based on the stack trace.

@zcbenz zcbenz merged commit 1f19a74 into main Aug 1, 2023
20 checks passed
@zcbenz zcbenz deleted the maybe-fix-menu-crash branch August 1, 2023 06:07
@release-clerk
Copy link

release-clerk bot commented Aug 1, 2023

Release Notes Persisted

Fixed a potential crash when calling tray.popUpContextMenu on macOS.

@trop
Copy link
Contributor

trop bot commented Aug 1, 2023

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

@trop trop bot removed the target/24-x-y PR should also be added to the "24-x-y" branch. label Aug 1, 2023
@trop
Copy link
Contributor

trop bot commented Aug 1, 2023

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

@trop trop bot added needs-manual-bp/24-x-y needs-manual-bp/25-x-y and removed target/25-x-y PR should also be added to the "25-x-y" branch. labels Aug 1, 2023
@trop
Copy link
Contributor

trop bot commented Aug 1, 2023

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

@trop trop bot added in-flight/26-x-y merged/26-x-y PR was merged to the "26-x-y" branch. and removed target/26-x-y PR should also be added to the "26-x-y" branch. in-flight/26-x-y labels Aug 1, 2023
win32ss pushed a commit to win32ss/supermium-electron that referenced this pull request Sep 24, 2023
fix: potential crash calling tray.popUpContextMenu
@trop
Copy link
Contributor

trop bot commented Oct 18, 2023

@codebytere has manually backported this PR to "25-x-y", please check out #40271

@trop
Copy link
Contributor

trop bot commented Oct 18, 2023

@codebytere has manually backported this PR to "24-x-y", please check out #40272

MrHuangJser pushed a commit to MrHuangJser/electron that referenced this pull request Dec 11, 2023
fix: potential crash calling tray.popUpContextMenu
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged/25-x-y PR was merged to the "25-x-y" branch. merged/26-x-y PR was merged to the "26-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants