[Spice-devel] [spice-gtk v4 2/2] gtk-session: clipboard: x11: owner-change improvement

[Jakub Janku]

Hi,

On Mon, Jan 14, 2019 at 1:41 PM Marc-André Lureau
<marcandre.lureau at gmail.com> wrote:
>
> Hi
>
> On Mon, Jan 14, 2019 at 4:34 PM Victor Toso <victortoso at redhat.com> wrote:
> >
> > From: Victor Toso <me at victortoso.com>
> >
> > On X11, the release-grab message might end up clearing the
> > GtkClipboard which triggers the owner-changed callback having the
> > event owner as NULL.
> >
> > We should not be calling gtk_clipboard_request_targets() in this
> > situation as is prone to errors as the intention here is request
> > clipboard information from changes made by client OS.
> >
> > The fix is to avoid any such request while spice client is holding the
> > keyboard grab.
>
> Two things that bug me about this:
> - it's x11 specific, weird

What's so weird about that?
It's also X11-specific that we can receive these "owner-change" events
while spice-gtk is holding the focus since on Wayland, apps need to
gain focus first to be able to set the clipboard, afaik.
The clipboard protocols on X11 and Wayland have simply been designed
in different ways. So although GTK tries to unify the behavior and
provide the same experience in both environments, some limitations
just cannot be overcome.

> - the condition seems wrong: if an application has the keyboard grab,
> that doesn't mean that another cannot update the clipboard. For
> example, I suppose this can easily happen with multiple
> pointers/inputs or some automation.

Sure, another app can update the clipboard, but imagine the following situation:
1) user copies something in the guest
2) an app in the client's system grabs the clipboard without having
keyboard focus -- this means that the grab likely wasn't directly
initiated by the user
3) spice-gtk receives "owner-change" event and subsequently sends grab
to the agent
4) based on the grab message from spice-gtk in step 3), vdagent grabs
the clipboard in the guest system -- and by that, it overwrites the
clipboard contents copied in step 1)

So the clipboard is suddenly changed, although the user didn't take any action.
I think this is unwanted and this patch solves it, so I'm all for
ack-ing this just for this reason. If this also solves the bug, it's
even better.

Nevertheless, I would be really happy if we could track down what
exactly causes the issues that have been reported and describe it in
more detail.
>
> Other than that, if it fixes or workaround real clipboard issues, I am
> not against it, but I think we should add more comments about the
> "hack".
>
>
> >
> > 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
> >
> > Changed in v4:
> > - Updated commit log
> >
> > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > Tested-by: James Harvey @jamespharvey20

Does this mean it fixes the issues for him?
> > ---
> >  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 abce43f..20df70d 100644
> > --- a/src/spice-gtk-session.c
> > +++ b/src/spice-gtk-session.c
> > @@ -674,6 +674,19 @@ static void clipboard_owner_change(GtkClipboard        *clipboard,
> >          return;
> >      }
> >
> > +#ifdef GDK_WINDOWING_X11
> > +    /* In X11, while holding the keyboard-grab we are not interested in this
> > +     * event as it either came by grab or release messages from agent.  */

For completeness, it might be good to mention that the event can be
caused by other applications too (as Marc-André pointed out), even
though spice-gtk is holding the keyboard-grab.

> > +    if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
> > +        spice_gtk_session_get_keyboard_has_focus(self)) {
> > +        SPICE_DEBUG("clipboard: owner-changed event: not requesting client's target "
> > +                    "while holding focus");
> > +        return;
> > +    }
> > +#endif
> > +    SPICE_DEBUG("clipboard: owner-changed event: has-focus=%d",
> > +                spice_gtk_session_get_keyboard_has_focus(self));
> > +
> >      s->clipboard_by_guest[selection] = FALSE;
> >      s->clip_hasdata[selection] = TRUE;
> >      if (s->auto_clipboard_enable && !read_only(self))
> > --
> > 2.20.1
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
Cheers,
Jakub