Hi, On Mon, Jan 7, 2019 at 1:41 PM Victor Toso <victort...@redhat.com> wrote: > > From: Victor Toso <m...@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 <victort...@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 _______________________________________________ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
