github/xrdp
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

chansrv: Make XrdpIme survive reconnects and stop native-client input being eaten

Browse Source 
Three fixes in input_ibus.c, found by capturing the IBus session bus while a
native Mac RDP client and a Guacamole (browser) client typed Chinese.

1. Keep the IBus thread alive across client disconnects.
   xrdp_input_unicode_destroy() used to stop the IBus thread and its private
   GMainContext on every disconnect, and unicode_init() started a new pair on
   every reconnect. ibus_bus_new() returns a process-wide singleton whose
   GDBusConnection stays bound to the context that was thread-default when it
   was first created, so a reconnect got the old connection back, bound to a
   context nobody iterates. ibus-daemon's CreateEngine call on our factory was
   never dispatched and every ibus_bus_set_global_engine() timed out ("failed
   to switch global engine to XrdpIme", 15 s each), losing all Unicode input
   until the session was fully logged out. (Same connection name on both
   connects in the bus trace.) destroy() now only restores the user's engine
   and drains the queue on the IBus thread (xrdp_input_reset_cb) and waits,
   bounded, for that; init() takes its existing "already ready" path.

2. Do not re-request XrdpIme while an instance is being created.
   Two characters arriving ~30 us apart each called set_global_engine(): the
   second found the name already XrdpIme but no instance yet, fell through,
   and made ibus destroy the instance under construction and build another
   ("IM enabled / IM disabled / IM enabled"), dropping what was queued for the
   first. A request made within XRDP_INPUT_ENGINE_CREATE_WAIT_US (2 s) is now
   treated as in flight and left alone; an older one is still treated as a
   stale name and re-set.

3. Measure the commit debounce from when the text was queued.
   "Quiet" was measured from the last raw key only. A user who pauses between
   typing the pinyin and confirming it (270 ms in the capture) makes the raw
   keys already quiet when the commit arrives, so it was committed at once and
   the client's cleanup Backspaces, one per leaked key and arriving 1-2 ms
   later, deleted the committed text and kept going into what was there
   before. Quiet is now measured from the later of the last raw key and the
   queue time, so every commit waits at least XRDP_INPUT_COMMIT_QUIET_US and
   any Backspace inside the window extends it (still capped by MAX_WAIT).

Tested against a real xrdp session in the agent-cn image, through guacd with a
script that mimics the native client (raw keys, pause, Unicode commit,
Backspaces after a variable delay): with the old code any delay >= 8 ms lost
the commit ("我是" became "wo"); now delays up to 90 ms pass, 110 ms and up
still fail. A fresh session and a reconnect both commit correctly, and the
"failed to switch global engine" timeouts are gone. Confirmed working with the
native client by hand. Costs every Unicode commit 100 ms of latency.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit was merged in pull request #1.
This commit is contained in:
Liyi Meng
2026-09-19 00:12:39 +00:00
parent f35530b523
commit 525be513be
1 changed files with 101 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files
sesman/chansrv/input_ibus.c
+101 -30
View File
@@ -75,6 +75,14 @@ static GCond state_cond;
static gboolean ibus_ready = FALSE;
static gboolean ibus_thread_exited = TRUE;
static gboolean ibus_shutting_down = FALSE;
/* TRUE from xrdp_input_unicode_destroy() until the IBus thread has run xrdp_input_reset_cb() */
static gboolean ibus_reset_pending = FALSE;
/* When xrdp_input_enable() last asked ibus to switch to XrdpIme (0 = never).
* Guarded by state_mutex. While the engine instance it asked for is still
* being created, asking again would make ibus destroy that instance and
* build another, and the characters queued for the first one are lost. */
static gint64 last_engine_request_time = 0;
#define XRDP_INPUT_ENGINE_CREATE_WAIT_US (2 * G_TIME_SPAN_SECOND)
static int engine_id = 0;
/* Debounce state for committing queued unicode text - IBus-thread only,
@@ -285,8 +293,31 @@ xrdp_input_enable(void)
* construction, so that guard would otherwise reject every
* commit that lands in this window - which, since composing
* even one phrase sends several characters in quick
* succession, was most of them). Fall through and let the
* queue/prepare-check mechanism wait for g_engine as usual. */
* succession, was most of them).
*
* If we asked for this engine moments ago, it is simply still
* being created: return and let the queue/prepare-check
* mechanism wait for g_engine. Do NOT ask again - that makes
* ibus destroy the instance under construction and create
* another (seen as "IM enabled / IM disabled / IM enabled" in
* the log), losing what was queued for the first. Only when no
* request is recent is this a stale name (ibus disabled the
* instance but left the name), which needs the re-set below. */
gboolean in_flight;
g_mutex_lock(&state_mutex);
in_flight = (last_engine_request_time != 0 &&
g_get_monotonic_time() - last_engine_request_time <
XRDP_INPUT_ENGINE_CREATE_WAIT_US);
g_mutex_unlock(&state_mutex);
if (in_flight)
{
g_free(last_input_name_snapshot);
g_object_unref(desc);
g_object_unref(local_bus);
return TRUE;
}
}
else if (last_input_name_snapshot != NULL && name != NULL &&
g_ascii_strcasecmp(name, last_input_name_snapshot) != 0)
@@ -335,6 +366,10 @@ xrdp_input_enable(void)
g_object_unref(desc);
}
g_mutex_lock(&state_mutex);
last_engine_request_time = g_get_monotonic_time();
g_mutex_unlock(&state_mutex);
result = ibus_bus_set_global_engine(local_bus, "XrdpIme");
if (!result)
{
@@ -383,7 +418,19 @@ xrdp_input_process_unicode_queue(gpointer data)
* client's own compensating keystrokes, if any, have had time to land),
* or XRDP_INPUT_COMMIT_MAX_WAIT_US has elapsed since the queue first had
* something pending, whichever comes first. If not yet ready, *wait_us
* (when non-NULL) is set to how much longer until it will be. */
* (when non-NULL) is set to how much longer until it will be.
*
* "Quiet" is measured from the later of the last raw key and the moment
* the text was queued. Measuring from the last raw key alone fails when
* the user has paused between typing the pinyin and confirming it: the
* raw keys are then already "quiet" when the commit arrives, so it is
* committed at once, and the client's cleanup Backspaces (which follow
* the commit by a few ms to a few tens of ms) delete the text just
* committed and keep going into what was there before. Measured against
* a real Mac client: a 270 ms pause before confirming, commit followed
* 2 ms later by as many Backspaces as leaked keys. Every commit therefore
* waits at least the quiet window, and any Backspace arriving inside it
* extends the wait. */
static gboolean
xrdp_input_queue_ready(gint64 *wait_us)
{
@@ -402,7 +449,8 @@ xrdp_input_queue_ready(gint64 *wait_us)
queue_wait_since = now;
}
deadline = MIN(last_raw_key_time + XRDP_INPUT_COMMIT_QUIET_US,
deadline = MIN(MAX(last_raw_key_time, queue_wait_since) +
XRDP_INPUT_COMMIT_QUIET_US,
queue_wait_since + XRDP_INPUT_COMMIT_MAX_WAIT_US);
if (now >= deadline)
@@ -476,15 +524,27 @@ xrdp_input_install_queue_source(void)
}
/*****************************************************************************/
/* Runs on the IBus thread when the RDP client disconnects. Puts back the
* user's own engine and drops anything still queued, but deliberately does
* NOT stop the loop or the thread.
*
* ibus_bus_new() returns a process-wide singleton here (see the note above
* xrdp_input_ibus_bus_disconnected), and its GDBusConnection stays bound to
* the GMainContext that was thread-default when it was first created. If
* the thread and that context were torn down on every client disconnect,
* the next connect would build a new private context but get the old
* connection back: nothing would iterate the old context, so ibus-daemon's
* CreateEngine call on our factory would never be dispatched and
* ibus_bus_set_global_engine() would time out ("failed to switch global
* engine to XrdpIme") on every reconnect. Keeping one IBus thread for the
* life of chansrv avoids the whole class of problem. */
static gboolean
xrdp_input_shutdown_cb(gpointer data)
xrdp_input_reset_cb(gpointer data)
{
XrdpUnicodeEvent *event;
gchar *original_engine_name;
LOG(LOG_LEVEL_INFO, "xrdp_input: shutting down IBus");
ibus_shutting_down = TRUE;
LOG(LOG_LEVEL_INFO, "xrdp_input: client disconnected, resetting IBus state");
/* Take ownership of last_input_name under state_mutex rather than
* reading then freeing it directly - xrdp_input_enable() on
@@ -515,10 +575,11 @@ xrdp_input_shutdown_cb(gpointer data)
g_free(event);
}
if (ibus_loop != NULL)
{
g_main_loop_quit(ibus_loop);
}
g_mutex_lock(&state_mutex);
last_engine_request_time = 0;
ibus_reset_pending = FALSE;
g_cond_broadcast(&state_cond);
g_mutex_unlock(&state_mutex);
return G_SOURCE_REMOVE;
}
@@ -881,6 +942,9 @@ xrdp_input_unicode_init(void)
if (connected)
{
/* Normal reconnect: the IBus thread was kept alive by
* xrdp_input_unicode_destroy(), just let it commit again. */
ibus_shutting_down = FALSE;
g_mutex_unlock(&state_mutex);
return 0;
}
@@ -964,11 +1028,12 @@ xrdp_input_unicode_destroy(void)
GMainContext *context;
LOG(LOG_LEVEL_DEBUG,
"xrdp_input_unicode_destroy: destroying IBus input");
"xrdp_input_unicode_destroy: client gone, resetting IBus input");
g_mutex_lock(&state_mutex);
ibus_shutting_down = TRUE;
context = ibus_context;
context = (ibus_ready && !ibus_thread_exited) ? ibus_context : NULL;
ibus_reset_pending = (context != NULL);
g_mutex_unlock(&state_mutex);
if (context != NULL)
@@ -976,29 +1041,35 @@ xrdp_input_unicode_destroy(void)
g_main_context_invoke_full(
context,
G_PRIORITY_HIGH,
xrdp_input_shutdown_cb,
xrdp_input_reset_cb,
NULL,
NULL);
g_main_context_wakeup(context);
}
g_mutex_lock(&state_mutex);
while (!ibus_thread_exited)
{
g_cond_wait(&state_cond, &state_mutex);
}
g_mutex_unlock(&state_mutex);
if (unicode_queue != NULL)
{
XrdpUnicodeEvent *event;
while ((event = g_async_queue_try_pop(unicode_queue)) != NULL)
/* Wait for the reset so a reconnect arriving right behind this
* cannot have its engine restored out from under it. Bounded,
* and also ends if the IBus thread exits (which never runs the
* callback), so this cannot hang. */
g_mutex_lock(&state_mutex);
{
g_free(event);
gint64 end = g_get_monotonic_time() + 5 * G_TIME_SPAN_SECOND;
while (ibus_reset_pending && !ibus_thread_exited)
{
if (!g_cond_wait_until(&state_cond, &state_mutex, end))
{
LOG(LOG_LEVEL_WARNING,
"xrdp_input_unicode_destroy: timed out waiting for "
"the IBus thread to reset");
break;
}
}
ibus_reset_pending = FALSE;
}
g_async_queue_unref(unicode_queue);
unicode_queue = NULL;
g_mutex_unlock(&state_mutex);
}
/* unicode_queue is left allocated: the IBus thread's queue source
* keeps using it across client connections. */
return 0;
}