[Spice-devel] [spice-gtk v2] gtk-session: do not request guest's clipboard data unnecessarily

[Jakub Janku]

Hi,

On Mon, Jan 7, 2019 at 1:41 PM Victor Toso <victortoso at redhat.com> wrote:
>
> From: Victor Toso <me at victortoso.com>
>
> If SpiceGtkSession is holding the keyboard, that's huge indication
> that the client should not be requesting guest's clipboard data yet.
>
> This patch adds a check in clipboard_get() callback, to avoid such
> requests. In Linux, this only happens with X11 backend.
>
> This patch helps to handle a possible state race between who owns the
> grab between client and agent which could lead to agent clipboard
> failing or getting stuck, see:
>
> Linux guest:
>     https://gitlab.freedesktop.org/spice/linux/vd_agent/issues/9
>
> Windows guest:
>     https://gitlab.freedesktop.org/spice/win32/vd_agent/issues/6
>
> The way to reproduce the race might depend on guest system and
> applications but it is connected to amount of VDAGENTD_CLIPBOARD_GRAB
> sent by the agent which depends on the amount of clipboard changes in
> the guest. Simple example is on RHEL 6.10, with Gedit, select a text
> char by char; Client receives VDAGENTD_CLIPBOARD_GRAB every time a new
> char is selected instead of once when the full message is selected.
>
> v1 -> v2:
> * Moved the check to the right place, otherwise the patch would not
>   work on Wayland (Christophe, Jakub)
> * Improve commit log (Jakub)

Now I get it, thanks :)
>
> Related: https://gitlab.freedesktop.org/spice/win32/vd_agent/issues/6
> Related: https://gitlab.freedesktop.org/spice/linux/vd_agent/issues/9
> Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876
>
> Signed-off-by: Victor Toso <victortoso at redhat.com>
> ---
>  src/spice-gtk-session.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index 1ccae07..a78f619 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -59,6 +59,7 @@ struct _SpiceGtkSessionPrivate {
>      gboolean                clip_hasdata[CLIPBOARD_LAST];
>      gboolean                clip_grabbed[CLIPBOARD_LAST];
>      gboolean                clipboard_by_guest[CLIPBOARD_LAST];
> +    gboolean                clipboard_by_guest_released[CLIPBOARD_LAST];
>      /* auto-usbredir related */
>      gboolean                auto_usbredir_enable;
>      int                     auto_usbredir_reqs;
> @@ -634,6 +635,12 @@ static void clipboard_owner_change(GtkClipboard        *clipboard,
>      if (s->main == NULL)
>          return;
>
> +    if (s->clipboard_by_guest_released[selection]) {
> +        /* Ignore owner-change event if this is just about agent releasing grab */
> +        s->clipboard_by_guest_released[selection] = FALSE;
> +        return;
> +    }
> +
>      if (s->clip_grabbed[selection]) {
>          s->clip_grabbed[selection] = FALSE;
>          if (spice_main_channel_agent_test_capability(s->main, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
> @@ -728,6 +735,14 @@ static void clipboard_get(GtkClipboard *clipboard,
>      g_return_if_fail(info < SPICE_N_ELEMENTS(atom2agent));
>      g_return_if_fail(s->main != NULL);
>
> +    /* No real need to set clipboard data while no client application will
> +     * be requested. This is useful for clients on X11 as in Wayland, this
> +     * callback is only called when SpiceGtkSession:keyboard-focus is false */
> +    if (spice_gtk_session_get_keyboard_has_focus(self)) {
> +        SPICE_DEBUG("Do not request clipboard data while holding the keyboard focus");
> +        return;
> +    }

Have you tested this with some clipboard managers? I would expect them
to request the clipboard data as soon as they receive a new
owner-change event. So this patch may potentially cripple them, but I
might be wrong. Not sure whether this is a use-case we need to support
but it might be good to know the consequences of this patch.
> +
>      ri.selection_data = selection_data;
>      ri.info = info;
>      ri.loop = g_main_loop_new(NULL, FALSE);
> @@ -769,6 +784,15 @@ cleanup:
>
>  static void clipboard_clear(GtkClipboard *clipboard, gpointer user_data)
>  {
> +    SpiceGtkSession *self = user_data;
> +    SpiceGtkSessionPrivate *s = self->priv;
> +    gint selection = get_selection_from_clipboard(s, clipboard);
> +
> +    g_return_if_fail(selection != -1);
> +
> +    if (s->clipboard_by_guest_released[selection])
> +        return;
> +
This gave me a pause for a while. It seems rather weird to me that
only part of the clipboard code logs debug messages (e.g.
clipboard_get_targets() prints all the atoms but there's no logging in
clipboard_grab()). By bypassing the SPICE_DEBUG below, we would lose
track of the guest's clipboard release event as there's no log in
clipboard_release() either.

Maybe it would be useful to add some logging to the other functions as
well? (clipboard_{grab, request, release})

>      SPICE_DEBUG("clipboard_clear");
>      /* We watch for clipboard ownership changes and act on those, so we
>         don't need to do anything here */
> @@ -1035,6 +1059,8 @@ static void clipboard_release(SpiceMainChannel *main, guint selection,
>
>      if (!s->clipboard_by_guest[selection])
>          return;
> +
> +    s->clipboard_by_guest_released[selection] = TRUE;
>      gtk_clipboard_clear(clipboard);
>      s->clipboard_by_guest[selection] = FALSE;
>  }
> --
> 2.20.1
>

Otherwise seems good to me, but I haven't tested it.

Cheers,
Jakub