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

[Jakub Janku <jjanku@xxxxxxxxxx>]

Hi,

On Sun, Mar 24, 2019 at 7:45 PM Marc-André Lureau <marcandre.lureau@xxxxxxxxx> wrote:
>
> Hi
>
> On Sun, Mar 24, 2019 at 6:49 PM Jakub Janku <jjanku@xxxxxxxxxx> wrote:
> >
> > Hi,
> >
> > On Thu, Mar 21, 2019 at 1:21 PM <marcandre.lureau@xxxxxxxxxx> wrote:
> > >
> > > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>
> >
> > 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.

I prefer "pending" a bit more. Both "clear" and "remove" seem fine, up to you which one you use.

Thanks,

Jakub
>
>
> >
> > >      }
> > >      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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
>
> --
> Marc-André Lureau

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel