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, With such as statement, you should provide detailed analysis, and strong arguments for workarounds. I think you are describing this conflicting situation: (all messages are asynchronous here, A & B are either client or remote end interchangeably) A->B: grab B->A: grab A: handle B grab B: handle A grab Both A&B think the other end has the grab... If we would instead explicitly release the grab, none would have it, which could be considered an improvement: A->B: grab B->A: grab A: handle B grab A->B: release B: handle A grab B->A: release B: handle A release A: handle B release Instead, we should probably have a priority for who wins. I suppose it should be the client. But how to distinguish the conflict case with the normal case? Perhaps an incremental counter (age/generation, whatever you call it) would be enough. Did you thought about something along those lines? Would you be able to come up with protocol changes for that? At this point, I think we could use some nice sequence diagrams in the spec! > 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 I have a hard time understanding why keyboard focus is related to clipboard. clipboard can be interacted with mouse only, or independently from any user input. Or from multiple inputs. I try to fit the keyboard input this into clipboard interaction issues, and I don't think that helps much. > (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()) && Note, gdk_display_get_default() is probably wrong. gtk_widget_get_display () is probably better. Not a big deal, needs to be changed over the whole tree I guess. > + spice_gtk_session_get_keyboard_has_focus(self)) { > + SPICE_DEBUG("%s: spice-gtk has keyboard focus, " > + "ignoring grab from other app", __FUNCTION__); > + return; > + } > #endif > > 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; > + } > + > for (n = 0; n < ntypes; ++n) { > found = FALSE; > for (m = 0; m < SPICE_N_ELEMENTS(atom2agent); m++) { > -- > 2.20.1 > > _______________________________________________ > Spice-devel mailing list > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/spice-devel -- Marc-André Lureau _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel