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

Submitted by Victor Toso on Jan. 10, 2019, 12:47 p.m.

Details

Message ID 20190110124714.7018-3-victortoso@redhat.com
State New
Headers show
Series "gtk-session and clipboard in x11" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso Jan. 10, 2019, 12:47 p.m.
From: Victor Toso <me@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@redhat.com>
Tested-by: James Harvey @jamespharvey20
---
 src/spice-gtk-session.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Patch hide | download patch | download mbox

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");
+        return;
+    }
+#endif
+    SPICE_DEBUG("clipboard: owner-changed event: has-foucus=%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))

Comments

On Thu, Jan 10, 2019 at 01:47:13PM +0100, Victor Toso wrote:
> From: Victor Toso <me@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@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

^ leftover, removed.

>  #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");
> +        return;
> +    }
> +#endif
> +    SPICE_DEBUG("clipboard: owner-changed event: has-foucus=%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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
On Thu, Jan 10, 2019 at 4:47 PM Victor Toso <victortoso@redhat.com> wrote:
>
> From: Victor Toso <me@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@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?

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

"foucus" spelling

> +
>      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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
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@redhat.com> wrote:
> >
> > From: Victor Toso <me@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@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.
Hi

On Thu, Jan 10, 2019 at 5:40 PM Victor Toso <victortoso@redhat.com> wrote:
>
> 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@redhat.com> wrote:
> > >
> > > From: Victor Toso <me@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@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.


I thought that case was avoided with
gtk_clipboard_get_owner(clipboard) == G_OBJECT(self)

But you are saying that the gtk_clipboard_clear() call ends up
triggering gtk_clipboard_request_targets() and that is the source of
races.

Why not simply?

-    if (gtk_clipboard_get_owner(clipboard) == G_OBJECT(self)) {
+    GObject *owner = gtk_clipboard_get_owner(clipboard);
+    if (owner == NULL || gtk_clipboard_get_owner(clipboard) ==
G_OBJECT(self)) {
         return;
     }


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



--
Marc-André Lureau
Hi,

On Fri, Jan 11, 2019 at 01:39:22PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Jan 10, 2019 at 5:40 PM Victor Toso <victortoso@redhat.com> wrote:
> >
> > 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@redhat.com> wrote:
> > > >
> > > > From: Victor Toso <me@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@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.
> 
> 
> I thought that case was avoided with
> gtk_clipboard_get_owner(clipboard) == G_OBJECT(self)
> 
> But you are saying that the gtk_clipboard_clear() call ends up
> triggering gtk_clipboard_request_targets() and that is the source of
> races.
> 
> Why not simply?

I thought it should work too but it didn't as explained by Jakub.

    https://lists.freedesktop.org/archives/spice-devel/2019-January/047142.html

I don't mind ignoring 3/3 patch but some fix here would be good
prior the release.

Earlier on, I had a patch that combined 2/3 and 3/3 which was
acked but I though later that would be better to split the change
which is done on onwer-changed event and avoiding the
clipboard_get() as it could be two different issues IMHO.

    https://lists.freedesktop.org/archives/spice-devel/2019-January/047012.html

So, ignoring the clipboard_get() issue for now, do you have any
other idea to address this one?

> -    if (gtk_clipboard_get_owner(clipboard) == G_OBJECT(self)) {
> +    GObject *owner = gtk_clipboard_get_owner(clipboard);
> +    if (owner == NULL || gtk_clipboard_get_owner(clipboard) ==
> G_OBJECT(self)) {
>          return;
>      }

Cheers,
Victor
Hi

On Mon, Jan 14, 2019 at 3:09 PM Victor Toso <victortoso@redhat.com> wrote:
>
> Hi,
>
> On Fri, Jan 11, 2019 at 01:39:22PM +0400, Marc-André Lureau wrote:
> > Hi
> >
> > On Thu, Jan 10, 2019 at 5:40 PM Victor Toso <victortoso@redhat.com> wrote:
> > >
> > > 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@redhat.com> wrote:
> > > > >
> > > > > From: Victor Toso <me@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@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.
> >
> >
> > I thought that case was avoided with
> > gtk_clipboard_get_owner(clipboard) == G_OBJECT(self)
> >
> > But you are saying that the gtk_clipboard_clear() call ends up
> > triggering gtk_clipboard_request_targets() and that is the source of
> > races.
> >
> > Why not simply?
>
> I thought it should work too but it didn't as explained by Jakub.
>
>     https://lists.freedesktop.org/archives/spice-devel/2019-January/047142.html
>
> I don't mind ignoring 3/3 patch but some fix here would be good
> prior the release.
>
> Earlier on, I had a patch that combined 2/3 and 3/3 which was
> acked but I though later that would be better to split the change
> which is done on onwer-changed event and avoiding the
> clipboard_get() as it could be two different issues IMHO.
>
>     https://lists.freedesktop.org/archives/spice-devel/2019-January/047012.html
>
> So, ignoring the clipboard_get() issue for now, do you have any
> other idea to address this one?

I am a bit lost, can you send an updated patch with a good commit
message of what it is actually fixing?

thanks!

>
> > -    if (gtk_clipboard_get_owner(clipboard) == G_OBJECT(self)) {
> > +    GObject *owner = gtk_clipboard_get_owner(clipboard);
> > +    if (owner == NULL || gtk_clipboard_get_owner(clipboard) ==
> > G_OBJECT(self)) {
> >          return;
> >      }
>
> Cheers,
> Victor