[Spice-devel] [PATCH spice-gtk 1/3] clipboard: accept grab only from the side with keyboard focus

Thu Mar 7 21:36:27 UTC 2019

Hi,

thanks for having a look!

On Wed, Mar 6, 2019 at 6:42 PM Marc-André Lureau
<marcandre.lureau at gmail.com> wrote:
>
> Hi
>
> On Thu, Feb 28, 2019 at 8:12 PM Jakub Janků <jjanku at redhat.com> 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.

Depends on what you consider to be "detailed analysis". Since you
correctly restated the problem with your own words, it seems like my
rather short description served its purpose :)
Also please note that I've tried to provide a more detailed analysis
of the issue reported by James Harvey earlier in [0]

[0] https://lists.freedesktop.org/archives/spice-devel/2019-January/047614.html

As for the arguments, I've actually expressed my main argument in the
cover letter, have you read it?
"Intention of these patches is to make spice-gtk
behave reasonably well with older agents."
I would like to have this patch reviewed with this in mind.
So this patch NOT trying to be a final solution to the problem.

If you look at the race condition from the standpoint of spice-gtk,
the situation looks the following:
1) spice-gtk sends grab
2) spice-gtk receives grab
You can't tell if it's a race or not. The normal behaviour can look the same.

Now, if you consider fix in spice-gtk only (to make older vdagents
play nicely with new spice-gtk), I see these possible mitigations:
a) measure time between 1) and 2) and discard one grab if the elapsed
time is too short:
this solution is rather empirical and seems unreliable to me
b) accept grab only from one side at a moment - that is what this patch does

>
> 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:

Yes, a slight improvement. However, the original grabs both in client
and guest would be lost, which is unwanted.
>
> 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.

Not necessarily. Consider James' case: if we make the client win, the
clipboard manager in the client would take over the clipboard in the
guest. The clipboard manager filters out some of the targets and
additionally, the "marching ants" in Excel would disappear.

>But how to distinguish the conflict case with
> the normal case? Perhaps an incremental counter (age/generation,
> whatever you call it) would be enough.

This would need a protocol change.
So given my note in the cover letter, this comment is quite unrelated
to this patch. I would prefer to discuss the protocol change later.
>
> 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.

If spice-gtk has keyboard focus, the user interacts with the guest system.
If it doesn't, user interacts with the client system.

Sure, the clipboard can change without user's input. E.g. with some
automation systems, as you pointer out earlier.
However, this is not the usual use case. And with this patch, I'm
trying to fix the most common scenario to provide a better default
experience.
>
> > (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 at redhat.com>
> > ---
> >
> > 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.

True.
>
>
>
>
>
> > +        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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
>
> --
> Marc-André Lureau

Cheers,
Jakub