Hi, Thanks for review! On Mon, Jan 07, 2019 at 09:28:04PM +0100, Jakub Janku wrote: > Hi, > > On Mon, Jan 7, 2019 at 1:41 PM Victor Toso <victortoso@xxxxxxxxxx> wrote: > > > > From: Victor Toso <me@xxxxxxxxxxxxxx> > > > > 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@xxxxxxxxxx> > > --- > > 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. I haven't but considering how Wayland works and the rationale behind it, I don't think we should care much if that might be a problem to other applications while our application is holding the keyboard grab. As the owner of the clipboard still is Spice when user leaves to another application (keyboard grab is lost) the request for clipboard data should succeed then. > 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. I don't think you are wrong that might affect clipboard managers but only if they are poking to every change that happens in the clipboard (like spice-vdagent on X11) > > + > > 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}) Yes, I have some clipboard cleanup patches to do, did not want to mix. I thought a bit about keeping clipboard_clear() log with this events but it can be quite verbose while user is just interacting with the guest; also, as that will be ignored by owner-event, the clipboard_clear log below is likely a non ignored event, hopefully more useful in the logs. I can remove the check and leave the log for all, no problem with that too. Some cleanup is needed with our without it. > > 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 Thanks again, Victor
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel