[spice-gtk,3/3] clipboard: invalidate targets request when needed

Submitted by Jakub Janku on Feb. 28, 2019, 7:12 p.m.

Details

Message ID 20190228191244.19464-4-jjanku@redhat.com
State New
Headers show
Series "clipboard grab race" ( rev: 2 1 ) in Spice

Not browsing as part of any series.

Commit Message

Jakub Janku Feb. 28, 2019, 7:12 p.m.
Targets request is no longer relevant when
clipboard owner changes since the retrieved targets
will be outdated.

When the request is no longer relevant,
invalidate it by pointing its weak ref to NULL.
As a consequence, free_weak_ref() call returns NULL
and clipboard_get_targets() exits.

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

This addresses the same issue that
was present in spice-vdagent and is already fixed.

---
 src/spice-gtk-session.c | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index 018cb4b..037547a 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];
+    GWeakRef                *last_targets_request[CLIPBOARD_LAST];
     /* auto-usbredir related */
     gboolean                auto_usbredir_enable;
     int                     auto_usbredir_reqs;
@@ -537,6 +538,16 @@  static gpointer free_weak_ref(gpointer data)
     return object;
 }
 
+static void nullify_weak_ref(GWeakRef **weak_ref_p)
+{
+    gpointer null_object = NULL;
+
+    if (*weak_ref_p) {
+        g_weak_ref_set(*weak_ref_p, null_object);
+        *weak_ref_p = NULL;
+    }
+}
+
 static void clipboard_get_targets(GtkClipboard *clipboard,
                                   GdkAtom *atoms,
                                   gint n_atoms,
@@ -551,23 +562,25 @@  static void clipboard_get_targets(GtkClipboard *clipboard,
 
     g_return_if_fail(SPICE_IS_GTK_SESSION(self));
 
-    if (atoms == NULL) {
-        SPICE_DEBUG("Retrieving the clipboard data has failed");
-        return;
-    }
-
     SpiceGtkSessionPrivate *s = self->priv;
     guint32 types[SPICE_N_ELEMENTS(atom2agent)] = { 0 };
     gint num_types;
     int a;
     int selection;
 
-    if (s->main == NULL)
-        return;
-
     selection = get_selection_from_clipboard(s, clipboard);
     g_return_if_fail(selection != -1);
 
+    s->last_targets_request[selection] = NULL;
+
+    if (atoms == NULL) {
+        SPICE_DEBUG("Retrieving the clipboard data has failed");
+        return;
+    }
+
+    if (s->main == NULL)
+        return;
+
     if (s->clip_grabbed[selection]) {
         SPICE_DEBUG("Clipboard is already grabbed, ignoring %d atoms", n_atoms);
         return;
@@ -652,6 +665,8 @@  static void clipboard_owner_change(GtkClipboard        *clipboard,
         return;
     }
 
+    nullify_weak_ref(&s->last_targets_request[selection]);
+
     /* In case we sent a grab to the agent, we need to release it now as
      * previous clipboard data should not be reachable anymore */
     if (s->clip_grabbed[selection]) {
@@ -690,9 +705,11 @@  static void clipboard_owner_change(GtkClipboard        *clipboard,
 #endif
 
     s->clip_hasdata[selection] = TRUE;
-    if (s->auto_clipboard_enable && !read_only(self))
+    if (s->auto_clipboard_enable && !read_only(self)) {
+        s->last_targets_request[selection] = get_weak_ref(self);
         gtk_clipboard_request_targets(clipboard, clipboard_get_targets,
-                                      get_weak_ref(self));
+                                      s->last_targets_request[selection]);
+    }
 }
 
 typedef struct
@@ -866,6 +883,8 @@  static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
         return TRUE;
     }
 
+    nullify_weak_ref(&s->last_targets_request[selection]);
+
     if (!gtk_clipboard_set_with_owner(cb,
                                       targets,
                                       num_targets,

Comments

Hi

On Thu, Feb 28, 2019 at 8:12 PM Jakub Janků <jjanku@redhat.com> wrote:
>
> Targets request is no longer relevant when
> clipboard owner changes since the retrieved targets
> will be outdated.
>
> When the request is no longer relevant,
> invalidate it by pointing its weak ref to NULL.
> As a consequence, free_weak_ref() call returns NULL
> and clipboard_get_targets() exits.

Why not simply check if user_data == last_targets_request?

What does nullify_weak_ref() really helps with?

>
> Signed-off-by: Jakub Janků <jjanku@redhat.com>
> ---
>
> This addresses the same issue that
> was present in spice-vdagent and is already fixed.
>
> ---
>  src/spice-gtk-session.c | 39 +++++++++++++++++++++++++++++----------
>  1 file changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index 018cb4b..037547a 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];
> +    GWeakRef                *last_targets_request[CLIPBOARD_LAST];
>      /* auto-usbredir related */
>      gboolean                auto_usbredir_enable;
>      int                     auto_usbredir_reqs;
> @@ -537,6 +538,16 @@ static gpointer free_weak_ref(gpointer data)
>      return object;
>  }
>
> +static void nullify_weak_ref(GWeakRef **weak_ref_p)
> +{
> +    gpointer null_object = NULL;
> +
> +    if (*weak_ref_p) {
> +        g_weak_ref_set(*weak_ref_p, null_object);
> +        *weak_ref_p = NULL;
> +    }
> +}
> +
>  static void clipboard_get_targets(GtkClipboard *clipboard,
>                                    GdkAtom *atoms,
>                                    gint n_atoms,
> @@ -551,23 +562,25 @@ static void clipboard_get_targets(GtkClipboard *clipboard,
>
>      g_return_if_fail(SPICE_IS_GTK_SESSION(self));
>
> -    if (atoms == NULL) {
> -        SPICE_DEBUG("Retrieving the clipboard data has failed");
> -        return;
> -    }
> -
>      SpiceGtkSessionPrivate *s = self->priv;
>      guint32 types[SPICE_N_ELEMENTS(atom2agent)] = { 0 };
>      gint num_types;
>      int a;
>      int selection;
>
> -    if (s->main == NULL)
> -        return;
> -
>      selection = get_selection_from_clipboard(s, clipboard);
>      g_return_if_fail(selection != -1);
>
> +    s->last_targets_request[selection] = NULL;
> +
> +    if (atoms == NULL) {
> +        SPICE_DEBUG("Retrieving the clipboard data has failed");
> +        return;
> +    }
> +
> +    if (s->main == NULL)
> +        return;
> +
>      if (s->clip_grabbed[selection]) {
>          SPICE_DEBUG("Clipboard is already grabbed, ignoring %d atoms", n_atoms);
>          return;
> @@ -652,6 +665,8 @@ static void clipboard_owner_change(GtkClipboard        *clipboard,
>          return;
>      }
>
> +    nullify_weak_ref(&s->last_targets_request[selection]);
> +
>      /* In case we sent a grab to the agent, we need to release it now as
>       * previous clipboard data should not be reachable anymore */
>      if (s->clip_grabbed[selection]) {
> @@ -690,9 +705,11 @@ static void clipboard_owner_change(GtkClipboard        *clipboard,
>  #endif
>
>      s->clip_hasdata[selection] = TRUE;
> -    if (s->auto_clipboard_enable && !read_only(self))
> +    if (s->auto_clipboard_enable && !read_only(self)) {
> +        s->last_targets_request[selection] = get_weak_ref(self);
>          gtk_clipboard_request_targets(clipboard, clipboard_get_targets,
> -                                      get_weak_ref(self));
> +                                      s->last_targets_request[selection]);
> +    }
>  }
>
>  typedef struct
> @@ -866,6 +883,8 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
>          return TRUE;
>      }
>
> +    nullify_weak_ref(&s->last_targets_request[selection]);
> +
>      if (!gtk_clipboard_set_with_owner(cb,
>                                        targets,
>                                        num_targets,
> --
> 2.20.1
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
Hi,

On Wed, Mar 6, 2019 at 7:11 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Thu, Feb 28, 2019 at 8:12 PM Jakub Janků <jjanku@redhat.com> wrote:
> >
> > Targets request is no longer relevant when
> > clipboard owner changes since the retrieved targets
> > will be outdated.
> >
> > When the request is no longer relevant,
> > invalidate it by pointing its weak ref to NULL.
> > As a consequence, free_weak_ref() call returns NULL
> > and clipboard_get_targets() exits.
>
> Why not simply check if user_data == last_targets_request?

This surely looks simpler,
thanks :)

Jakub
>
> What does nullify_weak_ref() really helps with?
>
> >
> > Signed-off-by: Jakub Janků <jjanku@redhat.com>
> > ---
> >
> > This addresses the same issue that
> > was present in spice-vdagent and is already fixed.
> >
> > ---
> >  src/spice-gtk-session.c | 39 +++++++++++++++++++++++++++++----------
> >  1 file changed, 29 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > index 018cb4b..037547a 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];
> > +    GWeakRef                *last_targets_request[CLIPBOARD_LAST];
> >      /* auto-usbredir related */
> >      gboolean                auto_usbredir_enable;
> >      int                     auto_usbredir_reqs;
> > @@ -537,6 +538,16 @@ static gpointer free_weak_ref(gpointer data)
> >      return object;
> >  }
> >
> > +static void nullify_weak_ref(GWeakRef **weak_ref_p)
> > +{
> > +    gpointer null_object = NULL;
> > +
> > +    if (*weak_ref_p) {
> > +        g_weak_ref_set(*weak_ref_p, null_object);
> > +        *weak_ref_p = NULL;
> > +    }
> > +}
> > +
> >  static void clipboard_get_targets(GtkClipboard *clipboard,
> >                                    GdkAtom *atoms,
> >                                    gint n_atoms,
> > @@ -551,23 +562,25 @@ static void clipboard_get_targets(GtkClipboard *clipboard,
> >
> >      g_return_if_fail(SPICE_IS_GTK_SESSION(self));
> >
> > -    if (atoms == NULL) {
> > -        SPICE_DEBUG("Retrieving the clipboard data has failed");
> > -        return;
> > -    }
> > -
> >      SpiceGtkSessionPrivate *s = self->priv;
> >      guint32 types[SPICE_N_ELEMENTS(atom2agent)] = { 0 };
> >      gint num_types;
> >      int a;
> >      int selection;
> >
> > -    if (s->main == NULL)
> > -        return;
> > -
> >      selection = get_selection_from_clipboard(s, clipboard);
> >      g_return_if_fail(selection != -1);
> >
> > +    s->last_targets_request[selection] = NULL;
> > +
> > +    if (atoms == NULL) {
> > +        SPICE_DEBUG("Retrieving the clipboard data has failed");
> > +        return;
> > +    }
> > +
> > +    if (s->main == NULL)
> > +        return;
> > +
> >      if (s->clip_grabbed[selection]) {
> >          SPICE_DEBUG("Clipboard is already grabbed, ignoring %d atoms", n_atoms);
> >          return;
> > @@ -652,6 +665,8 @@ static void clipboard_owner_change(GtkClipboard        *clipboard,
> >          return;
> >      }
> >
> > +    nullify_weak_ref(&s->last_targets_request[selection]);
> > +
> >      /* In case we sent a grab to the agent, we need to release it now as
> >       * previous clipboard data should not be reachable anymore */
> >      if (s->clip_grabbed[selection]) {
> > @@ -690,9 +705,11 @@ static void clipboard_owner_change(GtkClipboard        *clipboard,
> >  #endif
> >
> >      s->clip_hasdata[selection] = TRUE;
> > -    if (s->auto_clipboard_enable && !read_only(self))
> > +    if (s->auto_clipboard_enable && !read_only(self)) {
> > +        s->last_targets_request[selection] = get_weak_ref(self);
> >          gtk_clipboard_request_targets(clipboard, clipboard_get_targets,
> > -                                      get_weak_ref(self));
> > +                                      s->last_targets_request[selection]);
> > +    }
> >  }
> >
> >  typedef struct
> > @@ -866,6 +883,8 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
> >          return TRUE;
> >      }
> >
> > +    nullify_weak_ref(&s->last_targets_request[selection]);
> > +
> >      if (!gtk_clipboard_set_with_owner(cb,
> >                                        targets,
> >                                        num_targets,
> > --
> > 2.20.1
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
>
> --
> Marc-André Lureau