fix(gui): drop queued notifications once amulegui's window is gone - #865
Merged
Merged
Conversation
CamuleApp::OnNotifyEvent tests theApp->amuledlg before running a queued notification. CamuleRemoteGuiApp::OnNotifyEvent did not, and amulegui is the build where the window routinely goes away while the app keeps running. MuleNotify has two halves. On the main thread HandleNotification() runs the notification inline, and already declines to when there is no window to update. Off the main thread -- the EC socket's thread, for anything the daemon pushes -- it queues a CMuleGUIEvent instead, and that half had no such check. amulegui destroys its dialog whenever the EC link drops (ShutDown() calls Destroy() and nulls amuledlg), a remote core restart being the everyday way that happens, and the wx pending-event queue is not drained first. Whatever was already queued then runs against a null dialog. The handlers behind Notify() read theApp->amuledlg->m_transferwnd and its siblings without checking the dialog itself, so this dereferenced null rather than merely doing nothing. The crash report says so precisely: EXC_BAD_ACCESS, KERN_INVALID_ADDRESS at 0x3c0, and 0x3c0 is 960, which is where m_transferwnd sits in CamuleDlg under CLIENT_GUI -- a freed dialog would have faulted on a wild address, not a small one. The stack is ProcessPendingEvents -> wxEventHashTable::HandleEvent, i.e. the static EVT_MULE_NOTIFY entry, which is this function. Guarding here rather than in each handler: the handlers are only ever reached through Notify(), whose two entry points are this dispatcher and the main-thread path that already checks, so one test covers all of them and keeps the two halves symmetric. A notification with no window to update has nothing to do.
got3nks
added a commit
that referenced
this pull request
Aug 9, 2026
…#869) #865 stopped amulegui connecting. It guarded CamuleRemoteGuiApp::OnNotifyEvent on the dialog, which is right for the notifications that update the GUI and wrong for the ones that carry the EC socket: MuleNotify has two classes, and DoNotifyAlways() exists precisely because its callers must run whatever the GUI is doing. CoreNotify_LibSocketConnect and friends are how an asio callback reaches the main thread, and amulegui has no dialog until an EC connection has been made -- so dropping them meant the connection could never complete and every attempt ended at the 15 s watchdog. The distinction now travels with the event. HandleNotificationAlways() tags the CMuleGUIEvent it queues, Clone() carries the tag, and both dispatchers run a tagged event regardless of the dialog. The monolithic one gets the same treatment: it has always dropped socket notifications when amuledlg was null, which is far less exposed there (the dialog exists before networking starts) but is wrong in the same way during shutdown. Two GUI notifications are queued through DoNotifyAlways() as well -- DownloadCtrlSort and DownloadCtrlDoItemSelectionChanged -- so the exemption reached them too and the original crash came straight back. Both read theApp->amuledlg->m_transferwnd, which is the 0x3c0 in the reports. They now test the dialog themselves; the nine socket handlers on that path touch no GUI at all, checked one by one rather than assumed. That is still only one of the two ways a view is reached with no dialog. The remote GUI's container layer calls into the views directly from EC replies, bypassing MuleNotify entirely, and a reply can land while the dialog is gone for the same reason: it is destroyed when the link drops and rebuilt by Startup() after the next connect, and replies in flight are not drained. An audit of the whole tree found 26 such calls across 11 reply-driven handlers in 8 classes -- HandlePacket, ProcessUpdate, ProcessItemUpdate, ProcessItemUpdatePartfile, CreateItem, DeleteItem, ApplySearchProgress. Each is guarded so the container still updates its model and only the view call is skipped; the model has to keep tracking the daemon whether or not anyone is looking at it. Where a flag already gated the work (batchDownloadList, batchSharedList) the test folded into it rather than nesting another branch. The views themselves are not exposed. A control is a child of the dialog and cannot outlive it, so the derefs in DownloadListCtrl, SearchListCtrl, SharedFilesCtrl, GenericClientListCtrl and FriendListCtrl are safe, and the wxDataView port did not introduce this. Nor are the dialogs, the tray icon (RemoveSystray() runs before the dialog is destroyed) or the core sources.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
amulegui crashes when the remote core is restarted. Reproduced from a crash report on
rev. 3.0.1-495-g285a46a3f.What it is
CamuleApp::OnNotifyEventteststheApp->amuledlgbefore running a queued notification.CamuleRemoteGuiApp::OnNotifyEventdid not — and amulegui is the build where the window routinely goes away while the app keeps running.MuleNotifyhas two halves. On the main threadHandleNotification()runs the notification inline, and already declines to when there is no window to update. Off the main thread — the EC socket's thread, for anything the daemon pushes — it queues aCMuleGUIEventinstead, and that half had no such check. amulegui destroys its dialog whenever the EC link drops (ShutDown()callsDestroy()and nullsamuledlg), a remote core restart being the everyday way that happens, and the wx pending-event queue is not drained first. Whatever was already queued then runs against a null dialog.The handlers behind
Notify()readtheApp->amuledlg->m_transferwndand its siblings without checking the dialog itself, so this dereferences null rather than merely doing nothing.How the report pins it
0x3c0is 960, and 960 is wherem_transferwndsits inCamuleDlgunderCLIENT_GUI(confirmed with-fdump-record-layoutsagainst the amulegui compile flags). A freed dialog would have faulted on a wild address; a small one means the base pointer was null.wxEventHashTable::HandleEventis the static-event-table path, which is theEVT_MULE_NOTIFYentry — this dispatcher.The fix
One guard, mirroring the monolithic one.
Deliberately here rather than in each handler: the handlers are only ever reached through
Notify(), whose two entry points are this dispatcher and the main-thread path that already checks, so a single test covers all of them and keeps the two halves symmetric. Guarding the ~40 call sites inGuiEvents.cppindividually would be churn that hides the actual asymmetry.Verification
Builds clean on macOS, monolithic and amulegui, no warnings from the touched file; clang-format and both clang-tidy tiers clean over the diff.
Not yet exercised against a live core restart with the window hidden — that is the reproducer, and it is worth running before this is trusted rather than after.