From 92fdb099595ba0d079ce5d165794c5e529b29705 Mon Sep 17 00:00:00 2001 From: Liyi Meng Date: Mon, 17 Aug 2026 20:38:20 +0000 Subject: [PATCH] chansrv: Fix stale engine handling and cross-thread ibus commit A few follow-on fixes found while testing the ibus reconnect changes in a live session: - xrdp_input_unicode_init() failed outright if ibus had no default global engine yet at connect time, which is a normal state on a fresh session (nothing has chosen one yet), not an error. Since nothing else used the return value, this permanently killed unicode input for the whole session over a spurious check. - ibus can disable our engine instance (e.g. on a focus change) while leaving the global engine name as "XrdpIme", since those are tracked separately. xrdp_input_enable()'s fast path only checked the name, so it could skip re-asserting and leave xrdp_input_send_unicode() committing text through a disabled g_engine. Clear g_engine on disable and require it to be set for the fast path to apply. - ibus_engine_commit_text() was being called from chansrv's own thread, not the thread pumping the glib main loop that owns the engine's D-Bus connection (xrdp_input_main_loop). Marshal the actual commit through g_main_context_invoke() onto the correct thread. None of these are the full fix for intermittent dropped commits during real pinyin input testing - that's still open, with the current lead being ibus Reset calls interleaved with commit bursts, likely from the FocusOut/FocusIn churn caused by passing every raw keystroke through engine_process_key_event_cb. To be continued. Co-Authored-By: Claude Sonnet 5 --- sesman/chansrv/input_ibus.c | 77 +++++++++++++++++++++++++++++++------ 1 file changed, 66 insertions(+), 11 deletions(-) diff --git a/sesman/chansrv/input_ibus.c b/sesman/chansrv/input_ibus.c index 4be198cc..9b604945 100644 --- a/sesman/chansrv/input_ibus.c +++ b/sesman/chansrv/input_ibus.c @@ -53,8 +53,14 @@ xrdp_input_enable(void) * committing text to an engine that's no longer active. */ desc = ibus_bus_get_global_engine(bus); name = desc ? ibus_engine_desc_get_name(desc) : NULL; - if (name && !g_ascii_strcasecmp(name, "XrdpIme")) + if (name && !g_ascii_strcasecmp(name, "XrdpIme") && g_engine) { + /* XrdpIme is the global engine AND our cached engine instance is + * still the enabled one - genuinely nothing to do. If g_engine + * is NULL here, ibus disabled our instance (see + * xrdp_input_ibus_engine_disable) without changing the global + * engine name, so fall through and reassert below to get a + * fresh, enabled instance. */ g_object_unref(desc); return 0; } @@ -83,6 +89,40 @@ xrdp_input_enable(void) return 0; } +static gboolean +xrdp_input_commit_text_cb(gpointer data) +{ + gunichar chr = GPOINTER_TO_UINT(data); + + /* Runs on the same thread/main context as xrdp_input_main_loop() + * (see the g_main_context_invoke() call below), so this re-check of + * g_engine is race-free against xrdp_input_ibus_engine_enable() / + * xrdp_input_ibus_engine_disable(), which are dispatched on that + * same thread. + * + * NOTE: engine->has_focus was tried here as a gate (skip/retry the + * commit until the engine reports focus) on the theory that ibus + * drops commits sent while unfocused. That made things strictly + * worse: has_focus read FALSE on every single attempt, even ones + * that would otherwise have succeeded, because IBusEngine does not + * appear to auto-maintain that field from the FocusIn/FocusOut + * D-Bus calls in this setup - nothing in this file ever writes to + * it, so it's not a trustworthy signal without our own focus-in/ + * focus-out handlers actually tracking it. Do not reintroduce a + * has_focus check without first confirming it actually flips to + * TRUE (e.g. by logging it from a real focus-in signal handler). */ + if (g_engine) + { + ibus_engine_commit_text(g_engine, ibus_text_new_from_unichar(chr)); + } + else + { + LOG(LOG_LEVEL_ERROR, "xrdp_input_send_unicode: no active ibus engine to commit to"); + } + + return G_SOURCE_REMOVE; +} + int xrdp_input_send_unicode(char32_t unicode) { @@ -93,8 +133,16 @@ xrdp_input_send_unicode(char32_t unicode) return 1; } - gunichar chr = unicode; - ibus_engine_commit_text(g_engine, ibus_text_new_from_unichar(chr)); + /* ibus_engine_commit_text() must run on the same thread that's + * pumping the glib main loop the engine's D-Bus connection belongs + * to (the xrdp_input_main_loop thread) - that's also the thread + * that processes FocusIn/FocusOut for the engine. Calling it + * directly from here (chansrv's own thread) lets the commit race + * against that focus churn with no ordering guarantee, so ibus can + * silently drop it. g_main_context_invoke() marshals the actual + * commit onto the correct thread instead. */ + g_main_context_invoke(NULL, xrdp_input_commit_text_cb, + GUINT_TO_POINTER((guint)unicode)); return 0; } @@ -110,6 +158,16 @@ static void xrdp_input_ibus_engine_disable(IBusEngine *engine) { LOG(LOG_LEVEL_INFO, "xrdp_ibus_engine_disable: IM disabled"); + + /* ibus can disable our engine instance (e.g. it loses focus) while + * still reporting "XrdpIme" as the global engine name, since that's + * a separate piece of bookkeeping. Clear g_engine so xrdp_input_enable() + * notices the mismatch instead of committing text to a disabled + * engine on the next keypress. */ + if (engine == g_engine) + { + g_engine = NULL; + } } static void @@ -314,14 +372,11 @@ xrdp_input_unicode_init(void) tc_thread_create(xrdp_input_main_loop, NULL); - if (!ibus_bus_get_global_engine(bus)) - { - /* The bus connection itself is fine (and is now owned by the - * ibus main loop thread we just started), so leave it in place - * rather than tearing it down here. */ - LOG(LOG_LEVEL_ERROR, "xrdp_ibus_init: failed to get origin global engine"); - return 1; - } + /* No global engine may be set yet at this point (e.g. ibus hasn't + * picked a default on this fresh session, or the user simply never + * had one configured) - that's a normal state, not a failure. There + * is nothing to remember here; xrdp_input_enable() already handles + * a NULL/absent current engine correctly when it's actually needed. */ return 0; }