[Spice-devel] [spice-gtk v4 2/2] gtk-session: clipboard: x11: owner-change improvement

[Jakub Janku]

Hi,

I tried to fix this bug in a less radical way, but my patch unfortunately
did not cover all the cases.

I obtained some logs from James Harvey which make the situation clearer -
it can be found here:
https://termbin.com/40un
So I'll try to explain what's happening:

James uses KDE which has a clipboard manager integrated in (klipper).

(1) user copies something in the guest, grab is sent to the spice-gtk
(2) clipboard manager immediately requests the data, data is retrieved from
the vdagent
(3) user pastes the copied data in guest, this causes a quick re-grab in
the guest = a clipboard release message is sent to spice-gtk and it is
immediately followed by a new grab
(4) spice-gtk receives clipboard release, so it clears the clipboard
(5) clipboard manager detects that the clipboard has no owner, so it grabs
it itself, advertising the data it originally obtained from us in step (2)
(this normally enables the user to paste the data even after the source app
has been closed)
(6) spice-gtk receives "owner-change" signal caused by the grab in step
(5), requests clipboard targets and sends a grab to vdagent
(7) spice-gtk finally receives the grab from step (3), so it sets the
clipboard using gtk_clipboard_set_with_owner()
(8) vdagent receives grab from step (6), so it too sets the clipboard using
gtk_clipboard_set_with_owner()

This is clearly a race. We ended up in a state where both spice-gtk and
vdagent thinks the other component can provide the data, but in reality
none of them can.

(This does not happen always, as can be seen in the log, the first paste
succeeds.)

Given current spice protocol, I don't see a way to detect the race on
either side, but I'd love to be wrong. So I'd update the commit log and
comment in the code to explain the situation and then it's ack from me.

Cheers,
Jakub

On Mon, Jan 14, 2019, 1:34 PM Victor Toso <victortoso at redhat.com wrote:

> From: Victor Toso <me at victortoso.com>
>
> On X11, the release-grab message might end up clearing the
> GtkClipboard which triggers the owner-changed callback having the
> event owner as NULL.
>
> We should not be calling gtk_clipboard_request_targets() in this
> situation as is prone to errors as the intention here is request
> clipboard information from changes made by client OS.
>
> The fix is to avoid any such request while spice client is holding the
> keyboard grab.
>
> 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
>
> Changed in v4:
> - Updated commit log
>
> Signed-off-by: Victor Toso <victortoso at redhat.com>
> Tested-by: James Harvey @jamespharvey20
> ---
>  src/spice-gtk-session.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index abce43f..20df70d 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -674,6 +674,19 @@ static void clipboard_owner_change(GtkClipboard
>   *clipboard,
>          return;
>      }
>
> +#ifdef GDK_WINDOWING_X11
> +    /* In X11, while holding the keyboard-grab we are not interested in
> this
> +     * event as it either came by grab or release messages from agent.  */
> +    if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
> +        spice_gtk_session_get_keyboard_has_focus(self)) {
> +        SPICE_DEBUG("clipboard: owner-changed event: not requesting
> client's target "
> +                    "while holding focus");
> +        return;
> +    }
> +#endif
> +    SPICE_DEBUG("clipboard: owner-changed event: has-focus=%d",
> +                spice_gtk_session_get_keyboard_has_focus(self));
> +
>      s->clipboard_by_guest[selection] = FALSE;
>      s->clip_hasdata[selection] = TRUE;
>      if (s->auto_clipboard_enable && !read_only(self))
> --
> 2.20.1
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190128/3f6afd08/attachment.html>