Skip to content

Commit 904eec9

Browse files
committed
Improve GTK menu cleanup and thread safety
Ensures GTK menu and submenu signal handlers are properly disconnected and widgets are destroyed using gtk_widget_destroy for correct cleanup. Adds main thread enforcement for menu open/close operations to prevent threading issues. Simplifies pointer event handling in menu popup logic.
1 parent 902b1ef commit 904eec9

File tree

1 file changed

+99
-19
lines changed

1 file changed

+99
-19
lines changed

src/platform/linux/menu_linux.cpp

Lines changed: 99 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,18 @@ MenuItem::MenuItem(void* menu_item) {
206206
MenuItem::~MenuItem() {
207207
// Disconnect signal handlers before destruction to prevent accessing freed memory
208208
if (pimpl_->gtk_menu_item_) {
209+
// Disconnect submenu map/unmap handlers first if they exist
210+
if (pimpl_->submenu_ && pimpl_->submenu_->GetNativeObject()) {
211+
GtkWidget* submenu_widget = (GtkWidget*)pimpl_->submenu_->GetNativeObject();
212+
if (submenu_widget && GTK_IS_WIDGET(submenu_widget)) {
213+
g_signal_handlers_disconnect_by_func(G_OBJECT(submenu_widget),
214+
(gpointer)OnGtkSubmenuMap, this);
215+
g_signal_handlers_disconnect_by_func(G_OBJECT(submenu_widget),
216+
(gpointer)OnGtkSubmenuUnmap, this);
217+
}
218+
}
219+
220+
// Disconnect item-specific signal handlers
209221
if (pimpl_->activate_handler_id_ > 0) {
210222
g_signal_handler_disconnect(G_OBJECT(pimpl_->gtk_menu_item_), pimpl_->activate_handler_id_);
211223
pimpl_->activate_handler_id_ = 0;
@@ -215,16 +227,8 @@ MenuItem::~MenuItem() {
215227
pimpl_->toggled_handler_id_ = 0;
216228
}
217229

218-
// Disconnect submenu map/unmap handlers if they exist
219-
if (pimpl_->submenu_ && pimpl_->submenu_->GetNativeObject()) {
220-
GtkWidget* submenu_widget = (GtkWidget*)pimpl_->submenu_->GetNativeObject();
221-
if (submenu_widget) {
222-
g_signal_handlers_disconnect_by_func(G_OBJECT(submenu_widget),
223-
(gpointer)OnGtkSubmenuMap, this);
224-
g_signal_handlers_disconnect_by_func(G_OBJECT(submenu_widget),
225-
(gpointer)OnGtkSubmenuUnmap, this);
226-
}
227-
}
230+
// Note: We don't destroy the gtk_menu_item_ here because it's owned by the parent Menu
231+
// and will be destroyed when the Menu container is destroyed
228232
}
229233
}
230234

@@ -491,8 +495,14 @@ Menu::Menu(void* menu) {
491495
}
492496

493497
Menu::~Menu() {
494-
// Disconnect signal handlers before unreferencing to prevent accessing freed memory
498+
// Disconnect signal handlers and properly clean up GTK widget
495499
if (pimpl_->gtk_menu_) {
500+
// Ensure menu is closed before destroying to prevent processing events on freed widget
501+
if (gtk_widget_get_visible(pimpl_->gtk_menu_)) {
502+
gtk_menu_popdown(GTK_MENU(pimpl_->gtk_menu_));
503+
}
504+
505+
// Disconnect signal handlers before destroying to prevent accessing freed memory
496506
if (pimpl_->map_handler_id_ > 0) {
497507
g_signal_handler_disconnect(G_OBJECT(pimpl_->gtk_menu_), pimpl_->map_handler_id_);
498508
pimpl_->map_handler_id_ = 0;
@@ -502,7 +512,10 @@ Menu::~Menu() {
502512
pimpl_->unmap_handler_id_ = 0;
503513
}
504514

505-
g_object_unref(pimpl_->gtk_menu_);
515+
// Use gtk_widget_destroy() instead of g_object_unref() to properly clean up
516+
// the widget hierarchy and ensure all pending events are handled before destruction
517+
gtk_widget_destroy(pimpl_->gtk_menu_);
518+
pimpl_->gtk_menu_ = nullptr;
506519
}
507520
}
508521

@@ -603,6 +616,43 @@ std::vector<std::shared_ptr<MenuItem>> Menu::GetAllItems() const {
603616
}
604617

605618
bool Menu::Open(const PositioningStrategy& strategy, Placement placement) {
619+
// Ensure GTK operations run on the main thread (owner of default GMainContext)
620+
if (!g_main_context_is_owner(g_main_context_default())) {
621+
struct OpenInvokeData {
622+
Menu* self;
623+
PositioningStrategy strategy;
624+
Placement placement;
625+
bool result;
626+
GMutex mutex;
627+
GCond cond;
628+
bool done;
629+
} data{this, strategy, placement, false};
630+
631+
g_mutex_init(&data.mutex);
632+
g_cond_init(&data.cond);
633+
data.done = false;
634+
635+
g_mutex_lock(&data.mutex);
636+
g_main_context_invoke(nullptr, [](gpointer user_data) -> gboolean {
637+
OpenInvokeData* d = static_cast<OpenInvokeData*>(user_data);
638+
bool r = d->self->Open(d->strategy, d->placement);
639+
g_mutex_lock(&d->mutex);
640+
d->result = r;
641+
d->done = true;
642+
g_cond_signal(&d->cond);
643+
g_mutex_unlock(&d->mutex);
644+
return G_SOURCE_REMOVE;
645+
}, &data);
646+
647+
while (!data.done) {
648+
g_cond_wait(&data.cond, &data.mutex);
649+
}
650+
g_mutex_unlock(&data.mutex);
651+
g_cond_clear(&data.cond);
652+
g_mutex_clear(&data.mutex);
653+
return data.result;
654+
}
655+
606656
if (pimpl_->gtk_menu_) {
607657
gtk_widget_show_all(pimpl_->gtk_menu_);
608658

@@ -705,20 +755,50 @@ bool Menu::Open(const PositioningStrategy& strategy, Placement placement) {
705755
anchor_gravity, menu_gravity, nullptr);
706756
return true;
707757
} else {
708-
// Use pointer position only if we have a current event to anchor to
709-
GdkEvent* current_event = gtk_get_current_event();
710-
if (!current_event) {
711-
return false;
712-
}
713-
gtk_menu_popup_at_pointer(GTK_MENU(pimpl_->gtk_menu_), current_event);
714-
gdk_event_free(current_event);
758+
// Let GTK automatically use the current event
759+
gtk_menu_popup_at_pointer(GTK_MENU(pimpl_->gtk_menu_), nullptr);
715760
return true;
716761
}
717762
}
718763
return false;
719764
}
720765

721766
bool Menu::Close() {
767+
// Ensure GTK operations run on the main thread
768+
if (!g_main_context_is_owner(g_main_context_default())) {
769+
struct CloseInvokeData {
770+
Menu* self;
771+
bool result;
772+
GMutex mutex;
773+
GCond cond;
774+
bool done;
775+
} data{this, false};
776+
777+
g_mutex_init(&data.mutex);
778+
g_cond_init(&data.cond);
779+
data.done = false;
780+
781+
g_mutex_lock(&data.mutex);
782+
g_main_context_invoke(nullptr, [](gpointer user_data) -> gboolean {
783+
CloseInvokeData* d = static_cast<CloseInvokeData*>(user_data);
784+
bool r = d->self->Close();
785+
g_mutex_lock(&d->mutex);
786+
d->result = r;
787+
d->done = true;
788+
g_cond_signal(&d->cond);
789+
g_mutex_unlock(&d->mutex);
790+
return G_SOURCE_REMOVE;
791+
}, &data);
792+
793+
while (!data.done) {
794+
g_cond_wait(&data.cond, &data.mutex);
795+
}
796+
g_mutex_unlock(&data.mutex);
797+
g_cond_clear(&data.cond);
798+
g_mutex_clear(&data.mutex);
799+
return data.result;
800+
}
801+
722802
if (pimpl_->gtk_menu_) {
723803
gtk_menu_popdown(GTK_MENU(pimpl_->gtk_menu_));
724804
return true;

0 commit comments

Comments
 (0)