[spice-gtk,v2] gtk-session: do not request guest's clipboard data unnecessarily

Submitted by Victor Toso on Jan. 7, 2019, 12:41 p.m.

Details

Message ID 20190107124110.32177-1-victortoso@redhat.com
State New
Headers show
Series "gtk-session: do not request guest's clipboard data unnecessarily" ( rev: 2 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso Jan. 7, 2019, 12:41 p.m.
From: Victor Toso <me@victortoso.com>

If SpiceGtkSession is holding the keyboard, that's huge indication
that the client should not be requesting guest's clipboard data yet.

This patch adds a check in clipboard_get() callback, to avoid such
requests. In Linux, this only happens with X11 backend.

This patch helps to handle a possible state race between who owns the
grab between client and agent which could lead to agent clipboard
failing or getting stuck, see:

Linux guest:
    https://gitlab.freedesktop.org/spice/linux/vd_agent/issues/9

Windows guest:
    https://gitlab.freedesktop.org/spice/win32/vd_agent/issues/6

The way to reproduce the race might depend on guest system and
applications but it is connected to amount of VDAGENTD_CLIPBOARD_GRAB
sent by the agent which depends on the amount of clipboard changes in
the guest. Simple example is on RHEL 6.10, with Gedit, select a text
char by char; Client receives VDAGENTD_CLIPBOARD_GRAB every time a new
char is selected instead of once when the full message is selected.

v1 -> v2:
* Moved the check to the right place, otherwise the patch would not
  work on Wayland (Christophe, Jakub)
* Improve commit log (Jakub)

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>
---
 src/spice-gtk-session.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index 1ccae07..a78f619 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -59,6 +59,7 @@  struct _SpiceGtkSessionPrivate {
     gboolean                clip_hasdata[CLIPBOARD_LAST];
     gboolean                clip_grabbed[CLIPBOARD_LAST];
     gboolean                clipboard_by_guest[CLIPBOARD_LAST];
+    gboolean                clipboard_by_guest_released[CLIPBOARD_LAST];
     /* auto-usbredir related */
     gboolean                auto_usbredir_enable;
     int                     auto_usbredir_reqs;
@@ -634,6 +635,12 @@  static void clipboard_owner_change(GtkClipboard        *clipboard,
     if (s->main == NULL)
         return;
 
+    if (s->clipboard_by_guest_released[selection]) {
+        /* Ignore owner-change event if this is just about agent releasing grab */
+        s->clipboard_by_guest_released[selection] = FALSE;
+        return;
+    }
+
     if (s->clip_grabbed[selection]) {
         s->clip_grabbed[selection] = FALSE;
         if (spice_main_channel_agent_test_capability(s->main, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
@@ -728,6 +735,14 @@  static void clipboard_get(GtkClipboard *clipboard,
     g_return_if_fail(info < SPICE_N_ELEMENTS(atom2agent));
     g_return_if_fail(s->main != NULL);
 
+    /* No real need to set clipboard data while no client application will
+     * be requested. This is useful for clients on X11 as in Wayland, this
+     * callback is only called when SpiceGtkSession:keyboard-focus is false */
+    if (spice_gtk_session_get_keyboard_has_focus(self)) {
+        SPICE_DEBUG("Do not request clipboard data while holding the keyboard focus");
+        return;
+    }
+
     ri.selection_data = selection_data;
     ri.info = info;
     ri.loop = g_main_loop_new(NULL, FALSE);
@@ -769,6 +784,15 @@  cleanup:
 
 static void clipboard_clear(GtkClipboard *clipboard, gpointer user_data)
 {
+    SpiceGtkSession *self = user_data;
+    SpiceGtkSessionPrivate *s = self->priv;
+    gint selection = get_selection_from_clipboard(s, clipboard);
+
+    g_return_if_fail(selection != -1);
+
+    if (s->clipboard_by_guest_released[selection])
+        return;
+
     SPICE_DEBUG("clipboard_clear");
     /* We watch for clipboard ownership changes and act on those, so we
        don't need to do anything here */
@@ -1035,6 +1059,8 @@  static void clipboard_release(SpiceMainChannel *main, guint selection,
 
     if (!s->clipboard_by_guest[selection])
         return;
+
+    s->clipboard_by_guest_released[selection] = TRUE;
     gtk_clipboard_clear(clipboard);
     s->clipboard_by_guest[selection] = FALSE;
 }

Comments

Hi,

On Mon, Jan 7, 2019 at 1:41 PM Victor Toso <victortoso@redhat.com> wrote:
>
> From: Victor Toso <me@victortoso.com>
>
> If SpiceGtkSession is holding the keyboard, that's huge indication
> that the client should not be requesting guest's clipboard data yet.
>
> This patch adds a check in clipboard_get() callback, to avoid such
> requests. In Linux, this only happens with X11 backend.
>
> This patch helps to handle a possible state race between who owns the
> grab between client and agent which could lead to agent clipboard
> failing or getting stuck, see:
>
> Linux guest:
>     https://gitlab.freedesktop.org/spice/linux/vd_agent/issues/9
>
> Windows guest:
>     https://gitlab.freedesktop.org/spice/win32/vd_agent/issues/6
>
> The way to reproduce the race might depend on guest system and
> applications but it is connected to amount of VDAGENTD_CLIPBOARD_GRAB
> sent by the agent which depends on the amount of clipboard changes in
> the guest. Simple example is on RHEL 6.10, with Gedit, select a text
> char by char; Client receives VDAGENTD_CLIPBOARD_GRAB every time a new
> char is selected instead of once when the full message is selected.
>
> v1 -> v2:
> * Moved the check to the right place, otherwise the patch would not
>   work on Wayland (Christophe, Jakub)
> * Improve commit log (Jakub)

Now I get it, thanks :)
>
> 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>
> ---
>  src/spice-gtk-session.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index 1ccae07..a78f619 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -59,6 +59,7 @@ struct _SpiceGtkSessionPrivate {
>      gboolean                clip_hasdata[CLIPBOARD_LAST];
>      gboolean                clip_grabbed[CLIPBOARD_LAST];
>      gboolean                clipboard_by_guest[CLIPBOARD_LAST];
> +    gboolean                clipboard_by_guest_released[CLIPBOARD_LAST];
>      /* auto-usbredir related */
>      gboolean                auto_usbredir_enable;
>      int                     auto_usbredir_reqs;
> @@ -634,6 +635,12 @@ static void clipboard_owner_change(GtkClipboard        *clipboard,
>      if (s->main == NULL)
>          return;
>
> +    if (s->clipboard_by_guest_released[selection]) {
> +        /* Ignore owner-change event if this is just about agent releasing grab */
> +        s->clipboard_by_guest_released[selection] = FALSE;
> +        return;
> +    }
> +
>      if (s->clip_grabbed[selection]) {
>          s->clip_grabbed[selection] = FALSE;
>          if (spice_main_channel_agent_test_capability(s->main, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
> @@ -728,6 +735,14 @@ static void clipboard_get(GtkClipboard *clipboard,
>      g_return_if_fail(info < SPICE_N_ELEMENTS(atom2agent));
>      g_return_if_fail(s->main != NULL);
>
> +    /* No real need to set clipboard data while no client application will
> +     * be requested. This is useful for clients on X11 as in Wayland, this
> +     * callback is only called when SpiceGtkSession:keyboard-focus is false */
> +    if (spice_gtk_session_get_keyboard_has_focus(self)) {
> +        SPICE_DEBUG("Do not request clipboard data while holding the keyboard focus");
> +        return;
> +    }

Have you tested this with some clipboard managers? I would expect them
to request the clipboard data as soon as they receive a new
owner-change event. So this patch may potentially cripple them, but I
might be wrong. Not sure whether this is a use-case we need to support
but it might be good to know the consequences of this patch.
> +
>      ri.selection_data = selection_data;
>      ri.info = info;
>      ri.loop = g_main_loop_new(NULL, FALSE);
> @@ -769,6 +784,15 @@ cleanup:
>
>  static void clipboard_clear(GtkClipboard *clipboard, gpointer user_data)
>  {
> +    SpiceGtkSession *self = user_data;
> +    SpiceGtkSessionPrivate *s = self->priv;
> +    gint selection = get_selection_from_clipboard(s, clipboard);
> +
> +    g_return_if_fail(selection != -1);
> +
> +    if (s->clipboard_by_guest_released[selection])
> +        return;
> +
This gave me a pause for a while. It seems rather weird to me that
only part of the clipboard code logs debug messages (e.g.
clipboard_get_targets() prints all the atoms but there's no logging in
clipboard_grab()). By bypassing the SPICE_DEBUG below, we would lose
track of the guest's clipboard release event as there's no log in
clipboard_release() either.

Maybe it would be useful to add some logging to the other functions as
well? (clipboard_{grab, request, release})

>      SPICE_DEBUG("clipboard_clear");
>      /* We watch for clipboard ownership changes and act on those, so we
>         don't need to do anything here */
> @@ -1035,6 +1059,8 @@ static void clipboard_release(SpiceMainChannel *main, guint selection,
>
>      if (!s->clipboard_by_guest[selection])
>          return;
> +
> +    s->clipboard_by_guest_released[selection] = TRUE;
>      gtk_clipboard_clear(clipboard);
>      s->clipboard_by_guest[selection] = FALSE;
>  }
> --
> 2.20.1
>

Otherwise seems good to me, but I haven't tested it.

Cheers,
Jakub
Hi,

Thanks for review!

On Mon, Jan 07, 2019 at 09:28:04PM +0100, Jakub Janku wrote:
> Hi,
> 
> On Mon, Jan 7, 2019 at 1:41 PM Victor Toso <victortoso@redhat.com> wrote:
> >
> > From: Victor Toso <me@victortoso.com>
> >
> > If SpiceGtkSession is holding the keyboard, that's huge indication
> > that the client should not be requesting guest's clipboard data yet.
> >
> > This patch adds a check in clipboard_get() callback, to avoid such
> > requests. In Linux, this only happens with X11 backend.
> >
> > This patch helps to handle a possible state race between who owns the
> > grab between client and agent which could lead to agent clipboard
> > failing or getting stuck, see:
> >
> > Linux guest:
> >     https://gitlab.freedesktop.org/spice/linux/vd_agent/issues/9
> >
> > Windows guest:
> >     https://gitlab.freedesktop.org/spice/win32/vd_agent/issues/6
> >
> > The way to reproduce the race might depend on guest system and
> > applications but it is connected to amount of VDAGENTD_CLIPBOARD_GRAB
> > sent by the agent which depends on the amount of clipboard changes in
> > the guest. Simple example is on RHEL 6.10, with Gedit, select a text
> > char by char; Client receives VDAGENTD_CLIPBOARD_GRAB every time a new
> > char is selected instead of once when the full message is selected.
> >
> > v1 -> v2:
> > * Moved the check to the right place, otherwise the patch would not
> >   work on Wayland (Christophe, Jakub)
> > * Improve commit log (Jakub)
> 
> Now I get it, thanks :)
> >
> > 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>
> > ---
> >  src/spice-gtk-session.c | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> >
> > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > index 1ccae07..a78f619 100644
> > --- a/src/spice-gtk-session.c
> > +++ b/src/spice-gtk-session.c
> > @@ -59,6 +59,7 @@ struct _SpiceGtkSessionPrivate {
> >      gboolean                clip_hasdata[CLIPBOARD_LAST];
> >      gboolean                clip_grabbed[CLIPBOARD_LAST];
> >      gboolean                clipboard_by_guest[CLIPBOARD_LAST];
> > +    gboolean                clipboard_by_guest_released[CLIPBOARD_LAST];
> >      /* auto-usbredir related */
> >      gboolean                auto_usbredir_enable;
> >      int                     auto_usbredir_reqs;
> > @@ -634,6 +635,12 @@ static void clipboard_owner_change(GtkClipboard        *clipboard,
> >      if (s->main == NULL)
> >          return;
> >
> > +    if (s->clipboard_by_guest_released[selection]) {
> > +        /* Ignore owner-change event if this is just about agent releasing grab */
> > +        s->clipboard_by_guest_released[selection] = FALSE;
> > +        return;
> > +    }
> > +
> >      if (s->clip_grabbed[selection]) {
> >          s->clip_grabbed[selection] = FALSE;
> >          if (spice_main_channel_agent_test_capability(s->main, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
> > @@ -728,6 +735,14 @@ static void clipboard_get(GtkClipboard *clipboard,
> >      g_return_if_fail(info < SPICE_N_ELEMENTS(atom2agent));
> >      g_return_if_fail(s->main != NULL);
> >
> > +    /* No real need to set clipboard data while no client application will
> > +     * be requested. This is useful for clients on X11 as in Wayland, this
> > +     * callback is only called when SpiceGtkSession:keyboard-focus is false */
> > +    if (spice_gtk_session_get_keyboard_has_focus(self)) {
> > +        SPICE_DEBUG("Do not request clipboard data while holding the keyboard focus");
> > +        return;
> > +    }
> 
> Have you tested this with some clipboard managers? I would
> expect them to request the clipboard data as soon as they
> receive a new owner-change event.

I haven't but considering how Wayland works and the rationale
behind it, I don't think we should care much if that might be a
problem to other applications while our application is holding
the keyboard grab. As the owner of the clipboard still is Spice
when user leaves to another application (keyboard grab is lost)
the request for clipboard data should succeed then.

> So this patch may potentially cripple them, but I might be
> wrong. Not sure whether this is a use-case we need to support
> but it might be good to know the consequences of this patch.

I don't think you are wrong that might affect clipboard managers
but only if they are poking to every change that happens in the
clipboard (like spice-vdagent on X11)

> > +
> >      ri.selection_data = selection_data;
> >      ri.info = info;
> >      ri.loop = g_main_loop_new(NULL, FALSE);
> > @@ -769,6 +784,15 @@ cleanup:
> >
> >  static void clipboard_clear(GtkClipboard *clipboard, gpointer user_data)
> >  {
> > +    SpiceGtkSession *self = user_data;
> > +    SpiceGtkSessionPrivate *s = self->priv;
> > +    gint selection = get_selection_from_clipboard(s, clipboard);
> > +
> > +    g_return_if_fail(selection != -1);
> > +
> > +    if (s->clipboard_by_guest_released[selection])
> > +        return;
> > +
> This gave me a pause for a while. It seems rather weird to me
> that only part of the clipboard code logs debug messages (e.g.
> clipboard_get_targets() prints all the atoms but there's no
> logging in clipboard_grab()). By bypassing the SPICE_DEBUG
> below, we would lose track of the guest's clipboard release
> event as there's no log in clipboard_release() either.
> 
> Maybe it would be useful to add some logging to the other
> functions as well? (clipboard_{grab, request, release})

Yes, I have some clipboard cleanup patches to do, did not want to
mix. I thought a bit about keeping clipboard_clear() log with
this events but it can be quite verbose while user is just
interacting with the guest; also, as that will be ignored by
owner-event, the clipboard_clear log below is likely a non
ignored event, hopefully more useful in the logs.

I can remove the check and leave the log for all, no problem with
that too. Some cleanup is needed with our without it.

> >      SPICE_DEBUG("clipboard_clear");
> >      /* We watch for clipboard ownership changes and act on those, so we
> >         don't need to do anything here */
> > @@ -1035,6 +1059,8 @@ static void clipboard_release(SpiceMainChannel *main, guint selection,
> >
> >      if (!s->clipboard_by_guest[selection])
> >          return;
> > +
> > +    s->clipboard_by_guest_released[selection] = TRUE;
> >      gtk_clipboard_clear(clipboard);
> >      s->clipboard_by_guest[selection] = FALSE;
> >  }
> > --
> > 2.20.1
> >
> 
> Otherwise seems good to me, but I haven't tested it.
> 
> Cheers,
> Jakub

Thanks again,
Victor
On Tue, Jan 8, 2019 at 10:06 AM Victor Toso <victortoso@redhat.com> wrote:
>
> Hi,
>
> Thanks for review!
>
> On Mon, Jan 07, 2019 at 09:28:04PM +0100, Jakub Janku wrote:
> > Hi,
> >
> > On Mon, Jan 7, 2019 at 1:41 PM Victor Toso <victortoso@redhat.com> wrote:
> > >
> > > From: Victor Toso <me@victortoso.com>
> > >
> > > If SpiceGtkSession is holding the keyboard, that's huge indication
> > > that the client should not be requesting guest's clipboard data yet.
> > >
> > > This patch adds a check in clipboard_get() callback, to avoid such
> > > requests. In Linux, this only happens with X11 backend.
> > >
> > > This patch helps to handle a possible state race between who owns the
> > > grab between client and agent which could lead to agent clipboard
> > > failing or getting stuck, see:
> > >
> > > Linux guest:
> > >     https://gitlab.freedesktop.org/spice/linux/vd_agent/issues/9
> > >
> > > Windows guest:
> > >     https://gitlab.freedesktop.org/spice/win32/vd_agent/issues/6
> > >
> > > The way to reproduce the race might depend on guest system and
> > > applications but it is connected to amount of VDAGENTD_CLIPBOARD_GRAB
> > > sent by the agent which depends on the amount of clipboard changes in
> > > the guest. Simple example is on RHEL 6.10, with Gedit, select a text
> > > char by char; Client receives VDAGENTD_CLIPBOARD_GRAB every time a new
> > > char is selected instead of once when the full message is selected.
> > >
> > > v1 -> v2:
> > > * Moved the check to the right place, otherwise the patch would not
> > >   work on Wayland (Christophe, Jakub)
> > > * Improve commit log (Jakub)
> >
> > Now I get it, thanks :)
> > >
> > > 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>
> > > ---
> > >  src/spice-gtk-session.c | 26 ++++++++++++++++++++++++++
> > >  1 file changed, 26 insertions(+)
> > >
> > > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > > index 1ccae07..a78f619 100644
> > > --- a/src/spice-gtk-session.c
> > > +++ b/src/spice-gtk-session.c
> > > @@ -59,6 +59,7 @@ struct _SpiceGtkSessionPrivate {
> > >      gboolean                clip_hasdata[CLIPBOARD_LAST];
> > >      gboolean                clip_grabbed[CLIPBOARD_LAST];
> > >      gboolean                clipboard_by_guest[CLIPBOARD_LAST];
> > > +    gboolean                clipboard_by_guest_released[CLIPBOARD_LAST];
> > >      /* auto-usbredir related */
> > >      gboolean                auto_usbredir_enable;
> > >      int                     auto_usbredir_reqs;
> > > @@ -634,6 +635,12 @@ static void clipboard_owner_change(GtkClipboard        *clipboard,
> > >      if (s->main == NULL)
> > >          return;
> > >
> > > +    if (s->clipboard_by_guest_released[selection]) {
> > > +        /* Ignore owner-change event if this is just about agent releasing grab */
> > > +        s->clipboard_by_guest_released[selection] = FALSE;
> > > +        return;
> > > +    }
> > > +
> > >      if (s->clip_grabbed[selection]) {
> > >          s->clip_grabbed[selection] = FALSE;
> > >          if (spice_main_channel_agent_test_capability(s->main, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
> > > @@ -728,6 +735,14 @@ static void clipboard_get(GtkClipboard *clipboard,
> > >      g_return_if_fail(info < SPICE_N_ELEMENTS(atom2agent));
> > >      g_return_if_fail(s->main != NULL);
> > >
> > > +    /* No real need to set clipboard data while no client application will
> > > +     * be requested. This is useful for clients on X11 as in Wayland, this
> > > +     * callback is only called when SpiceGtkSession:keyboard-focus is false */
> > > +    if (spice_gtk_session_get_keyboard_has_focus(self)) {
> > > +        SPICE_DEBUG("Do not request clipboard data while holding the keyboard focus");
> > > +        return;
> > > +    }
> >
> > Have you tested this with some clipboard managers? I would
> > expect them to request the clipboard data as soon as they
> > receive a new owner-change event.
>
> I haven't but considering how Wayland works and the rationale
> behind it, I don't think we should care much if that might be a
> problem to other applications while our application is holding
> the keyboard grab. As the owner of the clipboard still is Spice
> when user leaves to another application (keyboard grab is lost)
> the request for clipboard data should succeed then.

Makes sense.
>
> > So this patch may potentially cripple them, but I might be
> > wrong. Not sure whether this is a use-case we need to support
> > but it might be good to know the consequences of this patch.
>
> I don't think you are wrong that might affect clipboard managers
> but only if they are poking to every change that happens in the
> clipboard (like spice-vdagent on X11)

Correct.
>
> > > +
> > >      ri.selection_data = selection_data;
> > >      ri.info = info;
> > >      ri.loop = g_main_loop_new(NULL, FALSE);
> > > @@ -769,6 +784,15 @@ cleanup:
> > >
> > >  static void clipboard_clear(GtkClipboard *clipboard, gpointer user_data)
> > >  {
> > > +    SpiceGtkSession *self = user_data;
> > > +    SpiceGtkSessionPrivate *s = self->priv;
> > > +    gint selection = get_selection_from_clipboard(s, clipboard);
> > > +
> > > +    g_return_if_fail(selection != -1);
> > > +
> > > +    if (s->clipboard_by_guest_released[selection])
> > > +        return;
> > > +
> > This gave me a pause for a while. It seems rather weird to me
> > that only part of the clipboard code logs debug messages (e.g.
> > clipboard_get_targets() prints all the atoms but there's no
> > logging in clipboard_grab()). By bypassing the SPICE_DEBUG
> > below, we would lose track of the guest's clipboard release
> > event as there's no log in clipboard_release() either.
> >
> > Maybe it would be useful to add some logging to the other
> > functions as well? (clipboard_{grab, request, release})
>
> Yes, I have some clipboard cleanup patches to do, did not want to
> mix. I thought a bit about keeping clipboard_clear() log with
> this events but it can be quite verbose while user is just
> interacting with the guest; also, as that will be ignored by

This is only a debug message that is logged when the --spice-debug
option is used, so the verbosity shouldn't be a problem, or is it?

> owner-event, the clipboard_clear log below is likely a non
> ignored event, hopefully more useful in the logs.
>
> I can remove the check and leave the log for all, no problem with
> that too. Some cleanup is needed with our without it.

Personally, I would move this change in logging to another patch as it
isn't really related to the fix itself, but I'll leave it up to you.

Acked-by: Jakub Janků <jjanku@redhat.com>

>
> > >      SPICE_DEBUG("clipboard_clear");
> > >      /* We watch for clipboard ownership changes and act on those, so we
> > >         don't need to do anything here */
> > > @@ -1035,6 +1059,8 @@ static void clipboard_release(SpiceMainChannel *main, guint selection,
> > >
> > >      if (!s->clipboard_by_guest[selection])
> > >          return;
> > > +
> > > +    s->clipboard_by_guest_released[selection] = TRUE;
> > >      gtk_clipboard_clear(clipboard);
> > >      s->clipboard_by_guest[selection] = FALSE;
> > >  }
> > > --
> > > 2.20.1
> > >
> >
> > Otherwise seems good to me, but I haven't tested it.
> >
> > Cheers,
> > Jakub
>
> Thanks again,
> Victor
On Mon, Jan 7, 2019 at 7:41 AM Victor Toso <victortoso@redhat.com> wrote:
>
> From: Victor Toso <me@victortoso.com>
>
> If SpiceGtkSession is holding the keyboard, that's huge indication
> that the client should not be requesting guest's clipboard data yet.
>
> This patch adds a check in clipboard_get() callback, to avoid such
> requests. In Linux, this only happens with X11 backend.
>
> This patch helps to handle a possible state race between who owns the
> grab between client and agent which could lead to agent clipboard
> failing or getting stuck, see:
>
> Linux guest:
>     https://gitlab.freedesktop.org/spice/linux/vd_agent/issues/9

(I'm the reporter of this linux issue.)  I appreciate your ongoing work on this.

v1 of your patch works extremely well for me.  I've been running it
since Dec 14.

I just tried v2, on top of git-gtk dbdf692.  I immediately had many
problems.  Attached are explanations and links to "remote-viewer
--spice-debug" logs.

I certainly don't know the internals of spice, but my general feeling
is there's a couple of things going on:
(1) "clipboard get" runs 2-4 times per CTRL+C or CTRL+V which seems odd to me
(2) Sometimes on a paste, "clipboard_clear" is ran.
(3) A spice client open but without focus (i.e. guest booted to Win 7,
and completely thrown off to the side and ignored) is still running
"clipboard_get_targets" when in the other client the clipboard is
changed by CTRL+C or CTRL+V, but without leaving its focus.
(4) Something (maybe v2 clipboard problems) completely crashed one of
my spice clients.  I haven't seen this happen before.


If I have a single spice client open, it works the majority of the
time, but not as well as v1.

When it usually works: https://termbin.com/0m7d
* From a single CTRL+V, 2 sets of "clipboard get" and "Do not
request..."  Not sure why there are 2 rather than 1.

When it sometimes fails: https://termbin.com/dcnc
* From a single CTRL+V, for some reason "clipboard_clear",
"clipboard_get_targets", which sets up the next paste as a ticking
timebomb of lagging the system for about 10 seconds.


If I have more than one spice client open, it acts about as badly as
without any patch.  Both problems I've detailed come back: copying a
cell in Excel, and repeatedly pasting it into different cells; and
copying a cell, pasting it into a different one, re-copying the new
cell, pasting it into a different one, and repeating.  By additional
clients being "open", I mean as soon as it starts, even before the
guest OS boots up and starts spice tools, and it stops the moment the
additional clients fully close.  For these logs, I was running two
instances of "remote-viewer --spice-debug", and I've made a comment
saying what I did, then show the log from that interaction from me,
from the viewer I was using followed by the one I wasn't.  (6659 is
always the one I was interacting with, and 12652/16746 is the one I
wasn't.)

When it rarely works: https://termbin.com/awsw
* From a single CTRL+C:
** On the client being used, 4 sets of "clipboard get", showing "Do
not request..."
** On the client not being used, "inputs", "clipboard_get_targets",
and "Failed to retrieve clipboard text"
* From a single CTRL+V
** On the client being used, 4 sets of "clipboard get", showing "Do
not request..."
** On the client not being used, "inputs", "clipboard_get_targets",
"Retrieving the clipboard data has failed", "Failed to retrieve
clipboard text"

When it usually fails - Failure type "A": https://termbin.com/te1g
* From a single CTRL+C:
** On the client being used, 4 sets of "clipboard get", showing "Do
not request..."
** On the client not being used, "inputs", "clipboard_get_targets",
and "Failed to retrieve clipboard text"
* From a single CTRL+V:
** On the client being used, "clipboard_clear",
"clipboard_get_targets", which makes the next paste to be a ticking
timebomb.
** On the client not being used, "inputs", "clipboard_get_targets",
"Retrieving the clipboard data has failed", "clipboard_get_targets",
"Clipboard is already grabbed, ignoring 8 atoms", "Failed to retrieve
clipboard text"

When it usually fails - Failure type "B": https://termbin.com/hseu
* Sometimes it fails with the client not being used showing "Failed to
retrieve clipboard text" where it above showed "Clipboard is already
grabbed, ignoring 8 atoms"
* In this one I include an extra section for hitting the down arrow
again and pasting, knowing with 100% certainty that will lag the
system and fail.  This could have been shown on the other failure
logs, just this is the only time I did it

As an extra bonus, while I was typing this email (on my host system,
so with both spice clients open but neither actively being used) the
client I've been referring to as "not being used" (even though here
neither were being used) completely crashed.  I don't remember if I
was copying or pasting anything in this email when it happened.  So, I
don't know if it might be related to the v2 patch, or if it might be
due to upgrading spice-git from 94f186e to dbdf692.  In these 39
commits I brought in to compile the v2 patch on top of, I do see some
having to do with "channel-display" and "display".

This crash log: https://termbin.com/9z37

The crash looks like it starts at 22:12:43.460 (line 209), with a
"channel-main.c:1362 main-1:0: Ignoring clipboard grab", then a
"spice-channel.c:801 main-1:0: Send error Error sending data: Broken
pipe" which spirals into a ton of other exits, resets, and errors.  In
this log are periodically 3 libusb lines which I always see happen
every 60 seconds while running "remote-viewer --spice-debug", anytime
with any release, git, or git version with patches.  But, after the
crash has spiraled through a ton of error lines, there's a lot more
libusb debugging information there.

Please let me know if it looks like this crash is related to the
clipboard problems, or if I should create a bug report on it.  My best
guess is it's related to the clipboard problems.  I'm going back to
the v1 patch, but will do so on top of current git (dbdf692) to try to
see if I get any more crashing.
On Tue, Jan 8, 2019 at 11:13 PM james harvey <jamespharvey20@gmail.com> wrote:
> v1 of your patch works extremely well for me.  I've been running it
> since Dec 14.

I'm about to start using the .patch files you emailed today, but
giving a few follow-ups first, still regarding v1.

> If I have a single spice client open, it works the majority of the
> time, but not as well as v1.
>
> When it usually works: https://termbin.com/0m7d
> * From a single CTRL+V, 2 sets of "clipboard get" and "Do not
> request..."  Not sure why there are 2 rather than 1.

Using v1, I see 2 "clipboard get" as well, but of course each with "got data".

> If I have more than one spice client open, it acts about as badly as
> without any patch.  ...
>
> When it rarely works: https://termbin.com/awsw

Using v1 now...

> * From a single CTRL+C:
> ** On the client being used, 4 sets of "clipboard get", showing "Do
> not request..."

2 "clipboard get" with "got data"

> ** On the client not being used, "inputs", "clipboard_get_targets",
> and "Failed to retrieve clipboard text"

"inputs", "clipboard_get_targets", but no "Failed to retrieve clipboard text"

> * From a single CTRL+V
> ** On the client being used, 4 sets of "clipboard get", showing "Do
> not request..."

"clipboard_clear", 3 sets of "clipboard get" with "got data"

> ** On the client not being used, "inputs", "clipboard_get_targets",
> "Retrieving the clipboard data has failed", "Failed to retrieve
> clipboard text"

"inputs", "clipboard_get_targets", "Retrieving the clipboard data has failed"

> Please let me know if it looks like this crash is related to the
> clipboard problems, or if I should create a bug report on it.  My best
> guess is it's related to the clipboard problems.  I'm going back to
> the v1 patch, but will do so on top of current git (dbdf692) to try to
> see if I get any more crashing.

I'm more confused on this now.  After sending the email, I reproduced
a few more identical crashes using v2 of the patch.  I went back to v1
on top of current git (dbdf692), and had gotten 18 hours using that
without any crashing.  I was literally typing this part of this email
and was saying it must be v2 causing the crash.  Then, I had an
identical crash.  So, it's not v2.  No idea how it lasted 18 hours
then now crashed.  I was away for 10 hours, but 8 of the 18 the host
(running the client) was being used, and occasionally the VM's.
Hi,

Only replying to the crash bit for now ;)

On Wed, Jan 09, 2019 at 06:29:51PM -0500, james harvey wrote:
> > Please let me know if it looks like this crash is related to
> > the clipboard problems, or if I should create a bug report on
> > it.  My best guess is it's related to the clipboard problems.
> > I'm going back to the v1 patch, but will do so on top of
> > current git (dbdf692) to try to see if I get any more
> > crashing.
> 
> I'm more confused on this now.  After sending the email, I
> reproduced a few more identical crashes using v2 of the patch.
> I went back to v1 on top of current git (dbdf692), and had
> gotten 18 hours using that without any crashing.  I was
> literally typing this part of this email and was saying it must
> be v2 causing the crash.  Then, I had an identical crash.  So,
> it's not v2.

Yep

> No idea how it lasted 18 hours then now crashed.
> I was away for 10 hours, but 8 of the 18 the host (running the
> client) was being used, and occasionally the VM's.

It is a broken pipe. I can't see any warnings on the log you
provided with the force shutdown before.

Besides what you highlighted before:

 | GSpice-DEBUG: 22:12:43.460: channel-main.c:1362 main-1:0: Ignoring clipboard grab                                                                        
 | GSpice-DEBUG: 22:12:43.460: spice-channel.c:801 main-1:0: Send error Error sending data: Broken pipe                                                     
 | GSpice-DEBUG: 22:12:43.460: spice-channel.c:837 main-1:0: Closing the channel: spice_channel_flush 32                                                    
 | GSpice-DEBUG: 22:12:43.460: spice-channel.c:2680 main-1:0: Coroutine exit main-1:0                                                                       
 | GSpice-DEBUG: 22:12:43.460: spice-channel.c:2871 main-1:0: reset

Several other channels have failures

 | GSpice-DEBUG: 22:12:43.460: spice-channel.c:1066 usbredir-9:1: Closing the connection: spice_channel_read() - ret=0

 | GSpice-DEBUG: 22:12:43.460: spice-channel.c:1066 record-6:0: Closing the connection: spice_channel_read() - ret=0

 | GSpice-DEBUG: 22:12:43.461: spice-channel.c:1066 usbredir-9:0: Closing the connection: spice_channel_read() - ret=0

 | GSpice-DEBUG: 22:12:43.461: spice-channel.c:1066 playback-5:0: Closing the connection: spice_channel_read() - ret=0

...

This leads to a reset on all channels, which eventually emits
SpiceSession::disconnected signal and your Remote-viewer is
calling spice_session_disconnect()

 | GSpice-DEBUG: 22:12:43.471: spice-session.c:1994 session: disconnecting 0  

And the session is disconnected a bit later

 | GSpice-DEBUG: 22:12:43.477: spice-session.c:1994 session: disconnecting 0                                                                                
 | GSpice-DEBUG: 22:12:43.477: spice-channel.c:2888 main-1:0: channel disconnect 0                                                                          
 | GSpice-DEBUG: 22:12:43.477: spice-channel.c:159 main-1:0: spice_channel_dispose 0x565032b1a400                                                           
 | GSpice-DEBUG: 22:12:43.477: spice-channel.c:2888 main-1:0: channel disconnect 12                                                                         
 | GSpice-DEBUG: 22:12:43.477: spice-channel.c:177 main-1:0: spice_channel_finalize 0x565032b1a400                                                          
 | GSpice-DEBUG: 22:12:43.477: spice-session.c:1802 no migration in progress                                                                                
 | GSpice-DEBUG: 22:12:43.477: spice-session.c:324 session dispose

(...)

On broken pipe we might try to reconnect but might be hard if
Remote-viewer calls spice_session_disconnect(). Perhaps some more
checks needs to be done before emitting the
SpiceSession::disconnected ...

Btw, that's pretty new in remote-viewer code

    https://pagure.io/virt-viewer/c/65ef66e42a6db2a9826fffef0db49920a02d358f

Thanks for helping/testing!

Victor