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

[Victor Toso]

Hi,

On Thu, Jan 10, 2019 at 05:18:48PM +0400, Marc-André Lureau wrote:
> On Thu, Jan 10, 2019 at 4:47 PM Victor Toso <victortoso at redhat.com> wrote:
> >
> > From: Victor Toso <me at victortoso.com>
> >
> > While interacting within the guest the VD_AGENT_CLIPBOARD_GRAB message
> > will be sent by the guest agent to notify that agent is holding some
> > clipboard data. When this clipboard data changes, the agent will send
> > VD_AGENT_CLIPBOARD_RELEASE to simply notify that previous clipboard
> > data is not available anymore and a new VD_AGENT_CLIPBOARD_GRAB
> > follows up with the current clipboard data being hold by agent.
> >
> > This patch helps in fixing a state race, in X11, between who is
> > 'holding' the clipboard grab.
> >
> > The bug happens because gtk_clipboard_clear() will set owner to none,
> > making the rest of the code path consider that clipboard data has
> > changed in the *client* and ends up requesting the metadata with
> > gtk_clipboard_request_targets(), handled by clipboard_get_targets().
> >
> > It is possible to have clipboard_get_targets() being called between
> > one VD_AGENT_CLIPBOARD_RELEASE and other VD_AGENT_CLIPBOARD_GRAB.
> >
> > 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
> >
> > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > Tested-by: James Harvey @jamespharvey20
> > ---
> >  src/spice-gtk-session.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > index adc72a2..85d5880 100644
> > --- a/src/spice-gtk-session.c
> > +++ b/src/spice-gtk-session.c
> > @@ -27,6 +27,9 @@
> >  #include <X11/Xlib.h>
> >  #include <gdk/gdkx.h>
> >  #endif
> > +#ifdef GDK_WINDOWING_WAYLAND
> > +#include <gdk/gdkwayland.h>
> > +#endif
> >  #ifdef G_OS_WIN32
> >  #include <windows.h>
> >  #include <gdk/gdkwin32.h>
> > @@ -674,6 +677,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.  */
> > +    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");
> 
> Couldn't this race happen without keyboard focus?

In X11 we would receive owner-changed every time that clipboard
data has changed so, if guest agent sends grab/release without
user triggering them, it might happen but I don't think that
would be common.

This patch addresses doing gtk_clipboard_request_targets() while
spice-gtk is the one changing GtkClipboard.

> > +        return;
> > +    }
> > +#endif
> > +    SPICE_DEBUG("clipboard: owner-changed event: has-foucus=%d",
> > +                spice_gtk_session_get_keyboard_has_focus(self));
> 
> "foucus" spelling

Thanks, fixed.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190110/61d082dc/attachment-0001.sig>