Hey, On Fri, Mar 15, 2019 at 8:02 PM Marc-André Lureau <marcandre.lureau@xxxxxxxxx> wrote: > > Hi > > On Fri, Mar 15, 2019 at 4:43 PM Jakub Janku <jjanku@xxxxxxxxxx> wrote: > > > > Hi, > > > > On Thu, Mar 14, 2019 at 6:18 PM Marc-André Lureau > > <marcandre.lureau@xxxxxxxxx> wrote: > > > > > > Hi > > > > > > On Thu, Feb 28, 2019 at 8:12 PM Jakub Janků <jjanku@xxxxxxxxxx> wrote: > > > > > > > > If two grab messages in opposite directions "meet" on their way > > > > to their destinations, we end up in a state when both spice-gtk > > > > and spice-vdagent think that the other component can provide > > > > clipboard data. As a consequence of this, neither of them can. > > > > > > > > This is a bug in the spice-protocol. To mitigate the issue, > > > > accept grab only from the side that currently has keyboard focus, > > > > this means: > > > > (1) spice-gtk has focus ==> > > > > * accept grabs from vdagent, > > > > * ignore grabs from other apps running in the host > > > > (2) spice-gtk doesn't have focus ==> > > > > * accept grabs from other apps running in the host > > > > * ignore grabs from vdagent > > > > > > > > The check in clipboard_owner_change() is X11-specific, > > > > because on Wayland, spice-gtk is first notified about the > > > > owner-change once it gains focus. > > > > > > > > The check in clipboard_grab() can be generic. > > > > Note that on Wayland, calling gtk_clipboard_set_with_owner() > > > > while not having focus doesn't have any effect anyway, > > > > only focused clients can set clipboard. > > > > > > > > With this patch, the race can still occur around the time > > > > when focus changes (rare, but possible), on X11 as well as Wayland. > > > > > > > > Related: https://gitlab.freedesktop.org/spice/spice-gtk/issues/82 > > > > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876 > > > > > > > > Signed-off-by: Jakub Janků <jjanku@xxxxxxxxxx> > > > > --- > > > > > > > > Victor, half of this patch is basically yours, > > > > so feel free to add signed-off or something, > > > > in the case this gets upstream :) > > > > > > > > --- > > > > 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 b48f92a..7b7370c 100644 > > > > --- a/src/spice-gtk-session.c > > > > +++ b/src/spice-gtk-session.c > > > > @@ -680,6 +680,13 @@ static void clipboard_owner_change(GtkClipboard *clipboard, > > > > s->clip_hasdata[selection] = FALSE; > > > > return; > > > > } > > > > + > > > > + if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) && > > > > + spice_gtk_session_get_keyboard_has_focus(self)) { > > > > + SPICE_DEBUG("%s: spice-gtk has keyboard focus, " > > > > + "ignoring grab from other app", __FUNCTION__); > > > > + return; > > > > + } > > > > #endif > > > > > > This will break clipboard managers interactions, which may take the > > > clipboard content, save it and/or modify it. > > > > Depends on the implementation of the given clipboard manager, I'd say. > > I tried the "clipboard indicator" you're using. Seems like no problem there :) > > without, and with that patch I suppose Sure > > > > > > > > > > > > s->clip_hasdata[selection] = TRUE; > > > > @@ -823,6 +830,12 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection, > > > > cb = get_clipboard_from_selection(s, selection); > > > > g_return_val_if_fail(cb != NULL, FALSE); > > > > > > > > + if (!spice_gtk_session_get_keyboard_has_focus(self)) { > > > > + SPICE_DEBUG("%s: spice-gtk doesn't have keyboard focus, " > > > > + "ignoring grab from guest agent", __FUNCTION__); > > > > + return FALSE; > > > > + } > > > > > > > > > Beside automation, the cursor alone may easily create a new clipboard > > > content which won't be available to the client side (the auto-grab may > > > fail to follow cursor etc). > > > > > > It's a bit unclear why it's not X11 specific but for client side > > > change it is, this could deserve a code comment. > > > > Tried to describe that in the commit log. I could add a comment in the > > code as well. > > yes, please > > > > > > > All in all, this feels weak and breaks some legitimate cases. > > > > > > I am not very strongly against this, as I understand it may help with > > > some races we discussed, > > > > Is this an ack or nack? > > If you ask me, it's a nack at this point, as I don't have a way to > reproduce the problem at this point and I clearly see problematic case > created by the patch. Iow, it looks like a regression to me. > > > Seems like we're just going round in circles now... > > Hopefully I am not the only one reviewing and questioning this patch > > > > but it feels like the problem is elsewhere > > > > sorry, but that's very vague, I have no idea what you're referring to > > Some clipboard managers seem to work fine. So what is klipper doing? I've already tried to describe that in [0]. [0] https://lists.freedesktop.org/archives/spice-devel/2019-March/048402.html > > > > > > and we need a better solution to prevent the race from happening. > > > > > > I haven't read the bug reports: this kind of workaround needs a > > > description of a broken use case (not a theoretical description of a > > > race that "never" happen in practice). > > > > I described a broken case in the previous email: > > KDE with klipper, "prevent empty clipboard" enabled + slow network > > I guess I will have to install KDE to understand and reproduce the issue. Again, see [0]. To simplify it for you, I'm including two files: Compile the 'test-case.c' and run it in the guest (can be Gnome). Use X11 in the client. As I mentioned in [0], it can be hard to reproduce the issue on a quick network. So the second file is a very simple patch for spice-gtk that delays grab messages for 200 ms before they're sent. Now try copying some text several times in a row, you should be able to see the following error from spice-gtk: | GSpice-CRITICAL **: 20:39:23.009: clipboard_request: assertion 's->clipboard_by_guest[selection] == FALSE' failed If not, try playing with the delay. Timing is crucial here. > > Rgardless if this patch is accepted or not, we should work on a real > solution to prevent the race. I agree with you on that. Jakub
From 87410cfefd54f1f55a59e8bd79c3495836a444b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jank=C5=AF?= <jjanku@xxxxxxxxxx> Date: Fri, 15 Mar 2019 20:40:50 +0100 Subject: [PATCH spice-gtk] clipboard: delay grab messages before sending This is to "simulate" slow network. --- src/spice-gtk-session.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c index b48f92a..361b25f 100644 --- a/src/spice-gtk-session.c +++ b/src/spice-gtk-session.c @@ -611,6 +611,8 @@ static void clipboard_get_targets(GtkClipboard *clipboard, s->clip_grabbed[selection] = TRUE; + g_usleep(300 * 1000); + if (spice_main_channel_agent_test_capability(s->main, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND)) spice_main_channel_clipboard_selection_grab(s->main, selection, types, num_types); -- 2.20.1
#include <gtk/gtk.h> GtkClipboard *clipboard; static void clipboard_owner_change_cb(GtkClipboard *clipboard, GdkEventOwnerChange *event, gpointer user_data) { g_message("owner change: owner=%p", event->owner); if (event->owner == NULL) { gtk_clipboard_set_text(clipboard, "test_case", -1); } } int main(int argc, char *argv[]) { gtk_init(&argc, &argv); clipboard = gtk_clipboard_get(GDK_SELECTION_CLIPBOARD); g_signal_connect(clipboard, "owner-change", G_CALLBACK(clipboard_owner_change_cb), NULL); gtk_main (); return 0; }
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel