[Spice-devel] [PATCH spice-gtk 1/2] clipboard: do not release between client grabs

[Jakub Janku]

Hi,

On Thu, Mar 21, 2019 at 1:21 PM <marcandre.lureau at redhat.com> wrote:
>
> From: Marc-André Lureau <marcandre.lureau at redhat.com>
>
> On the client side, whenever the grab owner changes (and the clipboard
> was previously grabbed), spice-gtk sends a clipboard release followed
> immediately by a new grab. But some clipboard managers on the remote
> side react to clipboard release events by taking a clipboard grab,
> presumably to avoid empty clipboards.
>
> The two grabs, coming from the client and from the remote sides, will
> race in both directions, which may confuse the client & remote side,
> as both believe the other side is the current grab owner, and thus
> further clipboard data requests are likely to fail.
>
> Let's avoid sending a release event when re-grabing.
>
> The race described above may still happen in other rare circunstances,
> and will require a protocol change. To avoid the conflict, a discussed
> solution could use a clipboard serial number.
>
> Tested with current linux & windows vdagent. Looking at earlier
> version of the code, it doesn't seem like subsequent grabs will be
> treated as an error.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
> ---
>  src/spice-gtk-session.c | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index b48f92a..0e748b6 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -569,8 +569,7 @@ static void clipboard_get_targets(GtkClipboard *clipboard,
>      g_return_if_fail(selection != -1);
>
>      if (s->clip_grabbed[selection]) {
> -        SPICE_DEBUG("Clipboard is already grabbed, ignoring %d atoms", n_atoms);
> -        return;
> +        SPICE_DEBUG("Clipboard is already grabbed, re-grab: %d atoms", n_atoms);
>      }
>
>      /* Set all Atoms that matches our current protocol implementation */
> @@ -652,18 +651,14 @@ static void clipboard_owner_change(GtkClipboard        *clipboard,
>          return;
>      }
>
> -    /* In case we sent a grab to the agent, we need to release it now as
> -     * previous clipboard data should not be reachable anymore */
> -    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)) {
> -            spice_main_channel_clipboard_selection_release(s->main, selection);
> -        }
> -    }

If event->owner is NULL, the clipboard is empty at the moment and this
function exits without requesting the new targets. So in this case, we
should still send release to the agent, otherwise the guest might
think that clipboard data can be provided while the clipboard in the
client is empty for a long time. Pasting data in the guest would
result into an invalid request being sent to spice-gtk.
> -
> -    /* We are mostly interested when owner has changed in which case
> -     * we would like to let agent know about new clipboard data. */
>      if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER) {
> +        if (s->clip_grabbed[selection]) {
> +            /* grab was sent to the agent, so release it */
> +            s->clip_grabbed[selection] = FALSE;
> +            if (spice_main_channel_agent_test_capability(s->main, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND)) {
> +                spice_main_channel_clipboard_selection_release(s->main, selection);
> +            }
> +        }
>          s->clip_hasdata[selection] = FALSE;
>          return;
>      }
> --
> 2.21.0.4.g36eb1cb9cf
>

Regards,
Jakub