From 77388dab8a849c0ce4bc458382fb4bfc92ff5711 Mon Sep 17 00:00:00 2001 From: Liyi Meng Date: Mon, 17 Aug 2026 22:25:29 +0000 Subject: [PATCH] chansrv: Fix deadlock in xrdp_input_enable() from wrong-thread call XrdpIme never actually became the active engine after the private main loop rewrite - ibus_bus_set_global_engine() returned FALSE every time. Confirmed the actual D-Bus error by bypassing the boolean-only wrapper and calling SetGlobalEngine directly: Set global engine failed: Timeout was reached Root cause: xrdp_input_enable() was being marshaled onto the IBus thread via g_main_context_invoke(), on the (wrong) theory that it "must run on the IBus thread" like commit_text does. But set_global_engine() blocks synchronously waiting for ibus-daemon's reply, and ibus-daemon can't reply until it finishes instantiating the engine - which means calling back into our own IBusFactory's "create-engine" over the same connection. Running xrdp_input_enable() on the IBus thread left that thread stuck blocked inside its own synchronous call, unable to service the nested callback ibus-daemon needed answered before it could reply - a self-deadlock, resolved only by ibus-daemon's own timeout. Calling xrdp_input_enable() directly from chansrv's thread instead (matching the original pre-rewrite code, which did this without apparent reason but happened to sidestep the deadlock) leaves the IBus thread free to answer the nested call while chansrv's thread blocks. GDBusConnection sync calls are documented thread-safe to issue from any thread, so this doesn't need marshaling despite `bus` otherwise being owned by the IBus thread. Verified end-to-end against a real ibus-daemon and a focused GTK entry widget (not just the D-Bus trace): ibus engine now correctly reports "XrdpIme" after a send, and 27 characters fired with zero delay between them - the exact rapid-fire pattern that used to drop most commits under the old design - landed in order with zero drops and zero duplicates. Co-Authored-By: Claude Sonnet 5 --- sesman/chansrv/input_ibus.c | 47 ++++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 19 deletions(-) diff --git a/sesman/chansrv/input_ibus.c b/sesman/chansrv/input_ibus.c index e7e051cf..01ececfd 100644 --- a/sesman/chansrv/input_ibus.c +++ b/sesman/chansrv/input_ibus.c @@ -156,7 +156,19 @@ xrdp_input_ibus_create_engine(IBusFactory *factory, } /*****************************************************************************/ -/* Must run on the IBus thread. */ +/* Must run on chansrv's own thread, NOT the IBus thread - confirmed by + * hitting the deadlock this avoids. ibus_bus_set_global_engine() blocks + * synchronously waiting for ibus-daemon's reply, but ibus-daemon can't + * reply until it finishes instantiating the engine, which means calling + * back into *our own* IBusFactory's "create-engine" over the same + * connection. If this function runs on the IBus thread, that thread is + * stuck blocked inside this call and can't service that nested + * callback - ibus-daemon times out waiting and set_global_engine fails + * with "Set global engine failed: Timeout was reached". Calling this + * from chansrv's thread instead leaves the IBus thread free to answer + * the nested call while this blocks. g_dbus_connection sync calls are + * documented thread-safe to issue from any thread, so this is fine + * despite `bus` otherwise being owned by the IBus thread. */ static gboolean xrdp_input_enable(void) { @@ -533,22 +545,6 @@ thread_exit: return 0; } -/*****************************************************************************/ -/* One-shot trampoline so xrdp_input_enable() (which must run on the IBus - * thread - see its own comment) gets invoked there instead of from - * chansrv's thread. The drain source's prepare/check deliberately still - * gate on g_engine != NULL so the main loop can block normally instead - * of spinning while engine creation is in flight; this is what actually - * gets that engine created/re-asserted in the first place. Harmless to - * run repeatedly - xrdp_input_enable() no-ops once XrdpIme is already - * the enabled global engine. */ -static gboolean -xrdp_input_enable_invoke_cb(gpointer data) -{ - xrdp_input_enable(); - return G_SOURCE_REMOVE; -} - /*****************************************************************************/ int xrdp_input_send_unicode(char32_t unicode) @@ -565,6 +561,15 @@ xrdp_input_send_unicode(char32_t unicode) return 0; } + /* Called directly from here (chansrv's own thread), not marshaled + * onto the IBus thread - see the comment on xrdp_input_enable() + * for why running it there deadlocks against ibus-daemon's nested + * "create-engine" callback. */ + if (!xrdp_input_enable()) + { + return 1; + } + g_mutex_lock(&state_mutex); if (!ibus_ready || ibus_shutting_down || @@ -588,12 +593,16 @@ xrdp_input_send_unicode(char32_t unicode) event->unicode = (gunichar)unicode; /* Take our own ref so the context can't be torn down by the IBus - * thread between here and the calls below, once we drop the mutex. */ + * thread between here and the wakeup call below, once we drop the + * mutex. The drain source's prepare/check gate on g_engine != NULL, + * so if engine creation (triggered by xrdp_input_enable() above) is + * still in flight, this wakeup is a no-op and the "enable" signal + * handler's own wakeup picks the queued character up once the + * engine is actually ready - it isn't lost. */ context = g_main_context_ref(ibus_context); g_async_queue_push(unicode_queue, event); g_main_context_wakeup(context); - g_main_context_invoke(context, xrdp_input_enable_invoke_cb, NULL); g_mutex_unlock(&state_mutex);