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

Fri Mar 15 20:31:09 UTC 2019

Hey,

On Fri, Mar 15, 2019 at 8:02 PM Marc-André Lureau
<marcandre.lureau at gmail.com> wrote:
>
> Hi
>
> On Fri, Mar 15, 2019 at 4:43 PM Jakub Janku <jjanku at redhat.com> wrote:
> >
> > Hi,
> >
> > On Thu, Mar 14, 2019 at 6:18 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,
> > > > 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 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()) &&
> > > > +        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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-clipboard-delay-grab-messages-before-sending.patch
Type: text/x-patch
Size: 851 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190315/9017dc73/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: test-case.c
Type: text/x-csrc
Size: 728 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190315/9017dc73/attachment-0001.c>