[spice-gtk,2/2] clipboard: do not release between remote grabs

Submitted by marcandre.lureau@redhat.com on March 21, 2019, 12:21 p.m.

Details

Message ID 20190321122134.17466-3-marcandre.lureau@redhat.com
State New
Headers show
Series "clipboard: skip release between grabs" ( rev: 2 1 ) in Spice

Not browsing as part of any series.

Commit Message

marcandre.lureau@redhat.com March 21, 2019, 12:21 p.m.
From: Marc-André Lureau <marcandre.lureau@redhat.com>

Delay the release events for 0.5 sec. If no further grab comes in,
then release the grab. Otherwise, let's skip the release. This avoids
some races with clipboard managers.

Related to:
https://gitlab.freedesktop.org/spice/spice-gtk/issues/82

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 src/spice-gtk-session.c | 80 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 72 insertions(+), 8 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index 0e748b6..d73a44b 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];
+    guint                   clipboard_release_delay[CLIPBOARD_LAST];
     /* auto-usbredir related */
     gboolean                auto_usbredir_enable;
     int                     auto_usbredir_reqs;
@@ -95,6 +96,7 @@  struct _SpiceGtkSessionPrivate {
 
 /* ------------------------------------------------------------------ */
 /* Prototypes for private functions */
+static void clipboard_release(SpiceGtkSession *self, guint selection);
 static void clipboard_owner_change(GtkClipboard *clipboard,
                                    GdkEventOwnerChange *event,
                                    gpointer user_data);
@@ -247,6 +249,23 @@  static void spice_gtk_session_dispose(GObject *gobject)
         G_OBJECT_CLASS(spice_gtk_session_parent_class)->dispose(gobject);
 }
 
+static void clipboard_release_delay_remove(SpiceGtkSession *self, guint selection,
+                                           gboolean release_if_delayed)
+{
+    SpiceGtkSessionPrivate *s = self->priv;
+
+    if (!s->clipboard_release_delay[selection])
+        return;
+
+    if (release_if_delayed) {
+        SPICE_DEBUG("delayed clipboard release, sel:%u", selection);
+        clipboard_release(self, selection);
+    }
+
+    g_source_remove(s->clipboard_release_delay[selection]);
+    s->clipboard_release_delay[selection] = 0;
+}
+
 static void spice_gtk_session_finalize(GObject *gobject)
 {
     SpiceGtkSession *self = SPICE_GTK_SESSION(gobject);
@@ -256,6 +275,7 @@  static void spice_gtk_session_finalize(GObject *gobject)
     /* release stuff */
     for (i = 0; i < CLIPBOARD_LAST; ++i) {
         g_clear_pointer(&s->clip_targets[i], g_free);
+        clipboard_release_delay_remove(self, i, true);
     }
 
     /* Chain up to the parent class */
@@ -815,6 +835,8 @@  static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
     int m, n;
     int num_targets = 0;
 
+    clipboard_release_delay_remove(self, selection, false);
+
     cb = get_clipboard_from_selection(s, selection);
     g_return_val_if_fail(cb != NULL, FALSE);
 
@@ -1044,17 +1066,12 @@  static gboolean clipboard_request(SpiceMainChannel *main, guint selection,
     return TRUE;
 }
 
-static void clipboard_release(SpiceMainChannel *main, guint selection,
-                              gpointer user_data)
+static void clipboard_release(SpiceGtkSession *self, guint selection)
 {
-    g_return_if_fail(SPICE_IS_GTK_SESSION(user_data));
-
-    SpiceGtkSession *self = user_data;
     SpiceGtkSessionPrivate *s = self->priv;
     GtkClipboard* clipboard = get_clipboard_from_selection(s, selection);
 
-    if (!clipboard)
-        return;
+    g_return_if_fail(clipboard != NULL);
 
     s->nclip_targets[selection] = 0;
 
@@ -1064,6 +1081,53 @@  static void clipboard_release(SpiceMainChannel *main, guint selection,
     s->clipboard_by_guest[selection] = FALSE;
 }
 
+typedef struct SpiceGtkClipboardRelease {
+    SpiceGtkSession *self;
+    guint selection;
+} SpiceGtkClipboardRelease;
+
+static gboolean clipboard_release_timeout(gpointer user_data)
+{
+    SpiceGtkClipboardRelease *rel = user_data;
+
+    clipboard_release_delay_remove(rel->self, rel->selection, true);
+
+    return G_SOURCE_REMOVE;
+}
+
+/*
+ * The agents send release between two grabs. This may trigger
+ * clipboard managers trying to grab the clipboard. We end up with two
+ * sides, client and remote, racing for the clipboard grab, and
+ * believing each other is the owner.
+ *
+ * Workaround this problem by delaying the release event by 0.5 sec.
+ * FIXME: protocol change to solve the conflict and set client priority.
+ */
+#define CLIPBOARD_RELEASE_DELAY 500 /* ms */
+
+static void clipboard_release_delay(SpiceMainChannel *main, guint selection,
+                                    gpointer user_data)
+{
+    SpiceGtkSession *self = SPICE_GTK_SESSION(user_data);
+    SpiceGtkSessionPrivate *s = self->priv;
+    GtkClipboard* clipboard = get_clipboard_from_selection(s, selection);
+    SpiceGtkClipboardRelease *rel;
+
+    if (!clipboard)
+        return;
+
+    clipboard_release_delay_remove(self, selection, true);
+
+    rel = g_new0(SpiceGtkClipboardRelease, 1);
+    rel->self = self;
+    rel->selection = selection;
+    s->clipboard_release_delay[selection] =
+        g_timeout_add_full(G_PRIORITY_DEFAULT, CLIPBOARD_RELEASE_DELAY,
+                           clipboard_release_timeout, rel, g_free);
+
+}
+
 static void channel_new(SpiceSession *session, SpiceChannel *channel,
                         gpointer user_data)
 {
@@ -1080,7 +1144,7 @@  static void channel_new(SpiceSession *session, SpiceChannel *channel,
         g_signal_connect(channel, "main-clipboard-selection-request",
                          G_CALLBACK(clipboard_request), self);
         g_signal_connect(channel, "main-clipboard-selection-release",
-                         G_CALLBACK(clipboard_release), self);
+                         G_CALLBACK(clipboard_release_delay), self);
     }
     if (SPICE_IS_INPUTS_CHANNEL(channel)) {
         spice_g_signal_connect_object(channel, "inputs-modifiers",

Comments

Hi,

On Thu, Mar 21, 2019 at 1:21 PM <marcandre.lureau@redhat.com> wrote:
>
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Delay the release events for 0.5 sec. If no further grab comes in,
> then release the grab. Otherwise, let's skip the release. This avoids
> some races with clipboard managers.
>
> Related to:
> https://gitlab.freedesktop.org/spice/spice-gtk/issues/82
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

In the 0.5 second period, any requests from apps in the client for
clipboard data should be ignored since the vdagent can't provide the
data any more, so we shouldn't request it.
So I would add a condition to clipboard_get():
    if (s->clipboard_release_delay[selection]) {
        SPICE_DEBUG("...");
        return;
    }
> ---
>  src/spice-gtk-session.c | 80 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 72 insertions(+), 8 deletions(-)
>
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index 0e748b6..d73a44b 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];
> +    guint                   clipboard_release_delay[CLIPBOARD_LAST];
>      /* auto-usbredir related */
>      gboolean                auto_usbredir_enable;
>      int                     auto_usbredir_reqs;
> @@ -95,6 +96,7 @@ struct _SpiceGtkSessionPrivate {
>
>  /* ------------------------------------------------------------------ */
>  /* Prototypes for private functions */
> +static void clipboard_release(SpiceGtkSession *self, guint selection);
>  static void clipboard_owner_change(GtkClipboard *clipboard,
>                                     GdkEventOwnerChange *event,
>                                     gpointer user_data);
> @@ -247,6 +249,23 @@ static void spice_gtk_session_dispose(GObject *gobject)
>          G_OBJECT_CLASS(spice_gtk_session_parent_class)->dispose(gobject);
>  }
>
> +static void clipboard_release_delay_remove(SpiceGtkSession *self, guint selection,
> +                                           gboolean release_if_delayed)
> +{
> +    SpiceGtkSessionPrivate *s = self->priv;
> +
> +    if (!s->clipboard_release_delay[selection])
> +        return;
> +
> +    if (release_if_delayed) {
> +        SPICE_DEBUG("delayed clipboard release, sel:%u", selection);
> +        clipboard_release(self, selection);
> +    }
> +
> +    g_source_remove(s->clipboard_release_delay[selection]);
> +    s->clipboard_release_delay[selection] = 0;
> +}
> +
>  static void spice_gtk_session_finalize(GObject *gobject)
>  {
>      SpiceGtkSession *self = SPICE_GTK_SESSION(gobject);
> @@ -256,6 +275,7 @@ static void spice_gtk_session_finalize(GObject *gobject)
>      /* release stuff */
>      for (i = 0; i < CLIPBOARD_LAST; ++i) {
>          g_clear_pointer(&s->clip_targets[i], g_free);
> +        clipboard_release_delay_remove(self, i, true);
>      }
>
>      /* Chain up to the parent class */
> @@ -815,6 +835,8 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
>      int m, n;
>      int num_targets = 0;
>
> +    clipboard_release_delay_remove(self, selection, false);
> +
>      cb = get_clipboard_from_selection(s, selection);
>      g_return_val_if_fail(cb != NULL, FALSE);
>
> @@ -1044,17 +1066,12 @@ static gboolean clipboard_request(SpiceMainChannel *main, guint selection,
>      return TRUE;
>  }
>
> -static void clipboard_release(SpiceMainChannel *main, guint selection,
> -                              gpointer user_data)
> +static void clipboard_release(SpiceGtkSession *self, guint selection)
>  {
> -    g_return_if_fail(SPICE_IS_GTK_SESSION(user_data));
> -
> -    SpiceGtkSession *self = user_data;
>      SpiceGtkSessionPrivate *s = self->priv;
>      GtkClipboard* clipboard = get_clipboard_from_selection(s, selection);
>
> -    if (!clipboard)
> -        return;
> +    g_return_if_fail(clipboard != NULL);
>
>      s->nclip_targets[selection] = 0;
>
> @@ -1064,6 +1081,53 @@ static void clipboard_release(SpiceMainChannel *main, guint selection,
>      s->clipboard_by_guest[selection] = FALSE;
>  }
>
> +typedef struct SpiceGtkClipboardRelease {
> +    SpiceGtkSession *self;
> +    guint selection;
> +} SpiceGtkClipboardRelease;
> +
> +static gboolean clipboard_release_timeout(gpointer user_data)
> +{
> +    SpiceGtkClipboardRelease *rel = user_data;
> +
> +    clipboard_release_delay_remove(rel->self, rel->selection, true);
> +
> +    return G_SOURCE_REMOVE;
> +}
> +
> +/*
> + * The agents send release between two grabs. This may trigger
> + * clipboard managers trying to grab the clipboard. We end up with two
> + * sides, client and remote, racing for the clipboard grab, and
> + * believing each other is the owner.
> + *
> + * Workaround this problem by delaying the release event by 0.5 sec.
> + * FIXME: protocol change to solve the conflict and set client priority.
> + */
> +#define CLIPBOARD_RELEASE_DELAY 500 /* ms */
> +
> +static void clipboard_release_delay(SpiceMainChannel *main, guint selection,
> +                                    gpointer user_data)
> +{
> +    SpiceGtkSession *self = SPICE_GTK_SESSION(user_data);
> +    SpiceGtkSessionPrivate *s = self->priv;
> +    GtkClipboard* clipboard = get_clipboard_from_selection(s, selection);
> +    SpiceGtkClipboardRelease *rel;
> +
> +    if (!clipboard)
> +        return;
> +
> +    clipboard_release_delay_remove(self, selection, true);
> +
> +    rel = g_new0(SpiceGtkClipboardRelease, 1);
> +    rel->self = self;
> +    rel->selection = selection;
> +    s->clipboard_release_delay[selection] =
> +        g_timeout_add_full(G_PRIORITY_DEFAULT, CLIPBOARD_RELEASE_DELAY,
> +                           clipboard_release_timeout, rel, g_free);
> +
> +}
> +
>  static void channel_new(SpiceSession *session, SpiceChannel *channel,
>                          gpointer user_data)
>  {
> @@ -1080,7 +1144,7 @@ static void channel_new(SpiceSession *session, SpiceChannel *channel,
>          g_signal_connect(channel, "main-clipboard-selection-request",
>                           G_CALLBACK(clipboard_request), self);
>          g_signal_connect(channel, "main-clipboard-selection-release",
> -                         G_CALLBACK(clipboard_release), self);
> +                         G_CALLBACK(clipboard_release_delay), self);

I find the naming you're introducing here rather confusing.
I wouldn't change the signal handler name to "clipboard_release_delay".

What about using the word "pending" instead of "delay"?
So:
clipboard_release_delay --> clipboard_release_pending
clipboard_release_delay_remove --> clipboard_release_pending_clear

>      }
>      if (SPICE_IS_INPUTS_CHANNEL(channel)) {
>          spice_g_signal_connect_object(channel, "inputs-modifiers",
> --
> 2.21.0.4.g36eb1cb9cf
>
Hi

On Sun, Mar 24, 2019 at 6:49 PM Jakub Janku <jjanku@redhat.com> wrote:
>
> Hi,
>
> On Thu, Mar 21, 2019 at 1:21 PM <marcandre.lureau@redhat.com> wrote:
> >
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Delay the release events for 0.5 sec. If no further grab comes in,
> > then release the grab. Otherwise, let's skip the release. This avoids
> > some races with clipboard managers.
> >
> > Related to:
> > https://gitlab.freedesktop.org/spice/spice-gtk/issues/82
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> In the 0.5 second period, any requests from apps in the client for
> clipboard data should be ignored since the vdagent can't provide the
> data any more, so we shouldn't request it.
> So I would add a condition to clipboard_get():
>     if (s->clipboard_release_delay[selection]) {
>         SPICE_DEBUG("...");
>         return;
>     }

yes, thanks

> > ---
> >  src/spice-gtk-session.c | 80 ++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 72 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > index 0e748b6..d73a44b 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];
> > +    guint                   clipboard_release_delay[CLIPBOARD_LAST];
> >      /* auto-usbredir related */
> >      gboolean                auto_usbredir_enable;
> >      int                     auto_usbredir_reqs;
> > @@ -95,6 +96,7 @@ struct _SpiceGtkSessionPrivate {
> >
> >  /* ------------------------------------------------------------------ */
> >  /* Prototypes for private functions */
> > +static void clipboard_release(SpiceGtkSession *self, guint selection);
> >  static void clipboard_owner_change(GtkClipboard *clipboard,
> >                                     GdkEventOwnerChange *event,
> >                                     gpointer user_data);
> > @@ -247,6 +249,23 @@ static void spice_gtk_session_dispose(GObject *gobject)
> >          G_OBJECT_CLASS(spice_gtk_session_parent_class)->dispose(gobject);
> >  }
> >
> > +static void clipboard_release_delay_remove(SpiceGtkSession *self, guint selection,
> > +                                           gboolean release_if_delayed)
> > +{
> > +    SpiceGtkSessionPrivate *s = self->priv;
> > +
> > +    if (!s->clipboard_release_delay[selection])
> > +        return;
> > +
> > +    if (release_if_delayed) {
> > +        SPICE_DEBUG("delayed clipboard release, sel:%u", selection);
> > +        clipboard_release(self, selection);
> > +    }
> > +
> > +    g_source_remove(s->clipboard_release_delay[selection]);
> > +    s->clipboard_release_delay[selection] = 0;
> > +}
> > +
> >  static void spice_gtk_session_finalize(GObject *gobject)
> >  {
> >      SpiceGtkSession *self = SPICE_GTK_SESSION(gobject);
> > @@ -256,6 +275,7 @@ static void spice_gtk_session_finalize(GObject *gobject)
> >      /* release stuff */
> >      for (i = 0; i < CLIPBOARD_LAST; ++i) {
> >          g_clear_pointer(&s->clip_targets[i], g_free);
> > +        clipboard_release_delay_remove(self, i, true);
> >      }
> >
> >      /* Chain up to the parent class */
> > @@ -815,6 +835,8 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
> >      int m, n;
> >      int num_targets = 0;
> >
> > +    clipboard_release_delay_remove(self, selection, false);
> > +
> >      cb = get_clipboard_from_selection(s, selection);
> >      g_return_val_if_fail(cb != NULL, FALSE);
> >
> > @@ -1044,17 +1066,12 @@ static gboolean clipboard_request(SpiceMainChannel *main, guint selection,
> >      return TRUE;
> >  }
> >
> > -static void clipboard_release(SpiceMainChannel *main, guint selection,
> > -                              gpointer user_data)
> > +static void clipboard_release(SpiceGtkSession *self, guint selection)
> >  {
> > -    g_return_if_fail(SPICE_IS_GTK_SESSION(user_data));
> > -
> > -    SpiceGtkSession *self = user_data;
> >      SpiceGtkSessionPrivate *s = self->priv;
> >      GtkClipboard* clipboard = get_clipboard_from_selection(s, selection);
> >
> > -    if (!clipboard)
> > -        return;
> > +    g_return_if_fail(clipboard != NULL);
> >
> >      s->nclip_targets[selection] = 0;
> >
> > @@ -1064,6 +1081,53 @@ static void clipboard_release(SpiceMainChannel *main, guint selection,
> >      s->clipboard_by_guest[selection] = FALSE;
> >  }
> >
> > +typedef struct SpiceGtkClipboardRelease {
> > +    SpiceGtkSession *self;
> > +    guint selection;
> > +} SpiceGtkClipboardRelease;
> > +
> > +static gboolean clipboard_release_timeout(gpointer user_data)
> > +{
> > +    SpiceGtkClipboardRelease *rel = user_data;
> > +
> > +    clipboard_release_delay_remove(rel->self, rel->selection, true);
> > +
> > +    return G_SOURCE_REMOVE;
> > +}
> > +
> > +/*
> > + * The agents send release between two grabs. This may trigger
> > + * clipboard managers trying to grab the clipboard. We end up with two
> > + * sides, client and remote, racing for the clipboard grab, and
> > + * believing each other is the owner.
> > + *
> > + * Workaround this problem by delaying the release event by 0.5 sec.
> > + * FIXME: protocol change to solve the conflict and set client priority.
> > + */
> > +#define CLIPBOARD_RELEASE_DELAY 500 /* ms */
> > +
> > +static void clipboard_release_delay(SpiceMainChannel *main, guint selection,
> > +                                    gpointer user_data)
> > +{
> > +    SpiceGtkSession *self = SPICE_GTK_SESSION(user_data);
> > +    SpiceGtkSessionPrivate *s = self->priv;
> > +    GtkClipboard* clipboard = get_clipboard_from_selection(s, selection);
> > +    SpiceGtkClipboardRelease *rel;
> > +
> > +    if (!clipboard)
> > +        return;
> > +
> > +    clipboard_release_delay_remove(self, selection, true);
> > +
> > +    rel = g_new0(SpiceGtkClipboardRelease, 1);
> > +    rel->self = self;
> > +    rel->selection = selection;
> > +    s->clipboard_release_delay[selection] =
> > +        g_timeout_add_full(G_PRIORITY_DEFAULT, CLIPBOARD_RELEASE_DELAY,
> > +                           clipboard_release_timeout, rel, g_free);
> > +
> > +}
> > +
> >  static void channel_new(SpiceSession *session, SpiceChannel *channel,
> >                          gpointer user_data)
> >  {
> > @@ -1080,7 +1144,7 @@ static void channel_new(SpiceSession *session, SpiceChannel *channel,
> >          g_signal_connect(channel, "main-clipboard-selection-request",
> >                           G_CALLBACK(clipboard_request), self);
> >          g_signal_connect(channel, "main-clipboard-selection-release",
> > -                         G_CALLBACK(clipboard_release), self);
> > +                         G_CALLBACK(clipboard_release_delay), self);
>
> I find the naming you're introducing here rather confusing.
> I wouldn't change the signal handler name to "clipboard_release_delay".

yes, let's not rename it. With
VD_AGENT_CAP_CLIPBOARD_NO_RELEASE_ON_REGRAB implementation, it won't
be delayed either.

> What about using the word "pending" instead of "delay"?
> So:
> clipboard_release_delay --> clipboard_release_pending

Delay or pending doesn't make much difference to me.

> clipboard_release_delay_remove --> clipboard_release_pending_clear

Again I don't care much. I prefer "remove", since that's the term used
for GSources. But if you feel strongly about "clear", that works for
me too.


>
> >      }
> >      if (SPICE_IS_INPUTS_CHANNEL(channel)) {
> >          spice_g_signal_connect_object(channel, "inputs-modifiers",
> > --
> > 2.21.0.4.g36eb1cb9cf
> >
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel