Re: [Spice-devel] [PATCH spice-gtk 2/2] clipboard: do not release between remote grabs
Hi,
On Sun, Mar 24, 2019 at 7:45 PM Marc-André Lureau <
marcandre.lur...@gmail.com> wrote:
>
> Hi
>
> On Sun, Mar 24, 2019 at 6:49 PM Jakub Janku wrote:
> >
> > Hi,
> >
> > On Thu, Mar 21, 2019 at 1:21 PM wrote:
> > >
> > > From: Marc-André Lureau
> > >
> > > 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
> >
> > 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 {
> > > gbooleanclip_hasdata[CLIPBOARD_LAST];
> > > gbooleanclip_grabbed[CLIPBOARD_LAST];
> > > gbooleanclipboard_by_guest[CLIPBOARD_LAST];
> > > +guint clipboard_release_delay[CLIPBOARD_LAST];
> > > /* auto-usbredir related */
> > > gbooleanauto_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(>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;
> > > +
> > >
Re: [Spice-devel] [PATCH spice-gtk 2/2] clipboard: do not release between remote grabs
Hi
On Sun, Mar 24, 2019 at 6:49 PM Jakub Janku wrote:
>
> Hi,
>
> On Thu, Mar 21, 2019 at 1:21 PM wrote:
> >
> > From: Marc-André Lureau
> >
> > 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
>
> 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 {
> > gbooleanclip_hasdata[CLIPBOARD_LAST];
> > gbooleanclip_grabbed[CLIPBOARD_LAST];
> > gbooleanclipboard_by_guest[CLIPBOARD_LAST];
> > +guint clipboard_release_delay[CLIPBOARD_LAST];
> > /* auto-usbredir related */
> > gbooleanauto_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(>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
Re: [Spice-devel] [PATCH spice-gtk 2/2] clipboard: do not release between remote grabs
Hi,
On Thu, Mar 21, 2019 at 1:21 PM wrote:
>
> From: Marc-André Lureau
>
> 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
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 {
> gbooleanclip_hasdata[CLIPBOARD_LAST];
> gbooleanclip_grabbed[CLIPBOARD_LAST];
> gbooleanclipboard_by_guest[CLIPBOARD_LAST];
> +guint clipboard_release_delay[CLIPBOARD_LAST];
> /* auto-usbredir related */
> gbooleanauto_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(>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
[Spice-devel] [PATCH spice-gtk 2/2] clipboard: do not release between remote grabs
From: Marc-André Lureau
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
---
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 {
gbooleanclip_hasdata[CLIPBOARD_LAST];
gbooleanclip_grabbed[CLIPBOARD_LAST];
gbooleanclipboard_by_guest[CLIPBOARD_LAST];
+guint clipboard_release_delay[CLIPBOARD_LAST];
/* auto-usbredir related */
gbooleanauto_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(>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;
+
