diff --git a/sesman/chansrv/input_ibus.c b/sesman/chansrv/input_ibus.c index 745176c0..f134f0ef 100644 --- a/sesman/chansrv/input_ibus.c +++ b/sesman/chansrv/input_ibus.c @@ -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; }