On Tue, Jan 8, 2019 at 10:06 AM Victor Toso <victortoso@xxxxxxxxxx> wrote: > > 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. Makes sense. > > > 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) Correct. > > > > + > > > 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 This is only a debug message that is logged when the --spice-debug option is used, so the verbosity shouldn't be a problem, or is it? > 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. Personally, I would move this change in logging to another patch as it isn't really related to the fix itself, but I'll leave it up to you. Acked-by: Jakub Janků <jjanku@xxxxxxxxxx> > > > > 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 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel