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

Submitted by Marc-André Lureau on March 24, 2019, 6:25 p.m.

Details

Message ID CAJ+F1CJgRkdEiyPasvMd=J4P=az4p86UHrb62NwPjcKmJwMe+A@mail.gmail.com
State New
Headers show
Series "clipboard: skip release between grabs" ( rev: 2 ) in Spice

Not browsing as part of any series.

Commit Message

Marc-André Lureau March 24, 2019, 6:25 p.m.
Hi

On Sun, Mar 24, 2019 at 6:50 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>
> >
> > On the client side, whenever the grab owner changes (and the clipboard
> > was previously grabbed), spice-gtk sends a clipboard release followed
> > immediately by a new grab. But some clipboard managers on the remote
> > side react to clipboard release events by taking a clipboard grab,
> > presumably to avoid empty clipboards.
> >
> > The two grabs, coming from the client and from the remote sides, will
> > race in both directions, which may confuse the client & remote side,
> > as both believe the other side is the current grab owner, and thus
> > further clipboard data requests are likely to fail.
> >
> > Let's avoid sending a release event when re-grabing.
> >
> > The race described above may still happen in other rare circunstances,
> > and will require a protocol change. To avoid the conflict, a discussed
> > solution could use a clipboard serial number.
> >
> > Tested with current linux & windows vdagent. Looking at earlier
> > version of the code, it doesn't seem like subsequent grabs will be
> > treated as an error.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  src/spice-gtk-session.c | 21 ++++++++-------------
> >  1 file changed, 8 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > index b48f92a..0e748b6 100644
> > --- a/src/spice-gtk-session.c
> > +++ b/src/spice-gtk-session.c
> > @@ -569,8 +569,7 @@ static void clipboard_get_targets(GtkClipboard *clipboard,
> >      g_return_if_fail(selection != -1);
> >
> >      if (s->clip_grabbed[selection]) {
> > -        SPICE_DEBUG("Clipboard is already grabbed, ignoring %d atoms", n_atoms);
> > -        return;
> > +        SPICE_DEBUG("Clipboard is already grabbed, re-grab: %d atoms", n_atoms);
> >      }
> >
> >      /* Set all Atoms that matches our current protocol implementation */
> > @@ -652,18 +651,14 @@ static void clipboard_owner_change(GtkClipboard        *clipboard,
> >          return;
> >      }
> >
> > -    /* 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]) {
> > -        s->clip_grabbed[selection] = FALSE;
> > -        if (spice_main_channel_agent_test_capability(s->main, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND)) {
> > -            spice_main_channel_clipboard_selection_release(s->main, selection);
> > -        }
> > -    }
>
> If event->owner is NULL, the clipboard is empty at the moment and this
> function exits without requesting the new targets. So in this case, we
> should still send release to the agent, otherwise the guest might
> think that clipboard data can be provided while the clipboard in the
> client is empty for a long time. Pasting data in the guest would
> result into an invalid request being sent to spice-gtk.

This is going into undocumented territories. But if what you say is
true in commit 9af2c481b74077ab7d6cb9d4bf589f9855a302f5, then yes,
client should probably release the clipboard.

I don't quite understand why gtk+ would allow NEW_OWNER, with owner ==
NULL and empty clipboard. It sounds more like a bug to me.

Would this be enough ?

             s->clip_grabbed[selection] = FALSE;
@@ -690,13 +694,6 @@ static void clipboard_owner_change(GtkClipboard
     *clipboard,

     s->clipboard_by_guest[selection] = FALSE;

-#ifdef GDK_WINDOWING_X11
-    if (!event->owner && GDK_IS_X11_DISPLAY(gdk_display_get_default())) {
-        s->clip_hasdata[selection] = FALSE;
-        return;
-    }
-#endif
-


> > -
> > -    /* We are mostly interested when owner has changed in which case
> > -     * we would like to let agent know about new clipboard data. */
> >      if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER) {
> > +        if (s->clip_grabbed[selection]) {
> > +            /* grab was sent to the agent, so release it */
> > +            s->clip_grabbed[selection] = FALSE;
> > +            if (spice_main_channel_agent_test_capability(s->main, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND)) {
> > +                spice_main_channel_clipboard_selection_release(s->main, selection);
> > +            }
> > +        }
> >          s->clip_hasdata[selection] = FALSE;
> >          return;
> >      }
> > --
> > 2.21.0.4.g36eb1cb9cf
> >
>
> Regards,
> Jakub
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

Patch hide | download patch | download mbox

diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index 5b2c27c..3dbcae6 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -671,7 +671,11 @@  static void clipboard_owner_change(GtkClipboard
     *clipboard,
         return;
     }

-    if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER) {
+    if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER ||
+#ifdef GDK_WINDOWING_X11
+        (!event->owner && GDK_IS_X11_DISPLAY(gdk_display_get_default()))
+#endif
+        ) {
         if (s->clip_grabbed[selection]) {
             /* grab was sent to the agent, so release it */

Comments

Hi,

On Sun, Mar 24, 2019 at 7:26 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Sun, Mar 24, 2019 at 6:50 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>
> > >
> > > On the client side, whenever the grab owner changes (and the clipboard
> > > was previously grabbed), spice-gtk sends a clipboard release followed
> > > immediately by a new grab. But some clipboard managers on the remote
> > > side react to clipboard release events by taking a clipboard grab,
> > > presumably to avoid empty clipboards.
> > >
> > > The two grabs, coming from the client and from the remote sides, will
> > > race in both directions, which may confuse the client & remote side,
> > > as both believe the other side is the current grab owner, and thus
> > > further clipboard data requests are likely to fail.
> > >
> > > Let's avoid sending a release event when re-grabing.
> > >
> > > The race described above may still happen in other rare circunstances,
> > > and will require a protocol change. To avoid the conflict, a discussed
> > > solution could use a clipboard serial number.
> > >
> > > Tested with current linux & windows vdagent. Looking at earlier
> > > version of the code, it doesn't seem like subsequent grabs will be
> > > treated as an error.
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > >  src/spice-gtk-session.c | 21 ++++++++-------------
> > >  1 file changed, 8 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > > index b48f92a..0e748b6 100644
> > > --- a/src/spice-gtk-session.c
> > > +++ b/src/spice-gtk-session.c
> > > @@ -569,8 +569,7 @@ static void clipboard_get_targets(GtkClipboard *clipboard,
> > >      g_return_if_fail(selection != -1);
> > >
> > >      if (s->clip_grabbed[selection]) {
> > > -        SPICE_DEBUG("Clipboard is already grabbed, ignoring %d atoms", n_atoms);
> > > -        return;
> > > +        SPICE_DEBUG("Clipboard is already grabbed, re-grab: %d atoms", n_atoms);
> > >      }
> > >
> > >      /* Set all Atoms that matches our current protocol implementation */
> > > @@ -652,18 +651,14 @@ static void clipboard_owner_change(GtkClipboard        *clipboard,
> > >          return;
> > >      }
> > >
> > > -    /* 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]) {
> > > -        s->clip_grabbed[selection] = FALSE;
> > > -        if (spice_main_channel_agent_test_capability(s->main, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND)) {
> > > -            spice_main_channel_clipboard_selection_release(s->main, selection);
> > > -        }
> > > -    }
> >
> > If event->owner is NULL, the clipboard is empty at the moment and this
> > function exits without requesting the new targets. So in this case, we
> > should still send release to the agent, otherwise the guest might
> > think that clipboard data can be provided while the clipboard in the
> > client is empty for a long time. Pasting data in the guest would
> > result into an invalid request being sent to spice-gtk.
>
> This is going into undocumented territories. But if what you say is
> true in commit 9af2c481b74077ab7d6cb9d4bf589f9855a302f5, then yes,
> client should probably release the clipboard.
>
> I don't quite understand why gtk+ would allow NEW_OWNER, with owner ==
> NULL and empty clipboard. It sounds more like a bug to me.

Well, in vdagent we release the clipboard with the following call:
    XSetSelectionOwner(x11->display, clip, None, CurrentTime);
So it seems completely fine to me that we get a GdkEventOwnerChange
with reason NEW_OWNER and owner set to NULL. I don't think it's a bug.
>
> Would this be enough ?

Sure.

Cheers,
Jakub
>
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index 5b2c27c..3dbcae6 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -671,7 +671,11 @@ static void clipboard_owner_change(GtkClipboard
>      *clipboard,
>          return;
>      }
>
> -    if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER) {
> +    if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER ||
> +#ifdef GDK_WINDOWING_X11
> +        (!event->owner && GDK_IS_X11_DISPLAY(gdk_display_get_default()))
> +#endif
> +        ) {
>          if (s->clip_grabbed[selection]) {
>              /* grab was sent to the agent, so release it */
>              s->clip_grabbed[selection] = FALSE;
> @@ -690,13 +694,6 @@ static void clipboard_owner_change(GtkClipboard
>      *clipboard,
>
>      s->clipboard_by_guest[selection] = FALSE;
>
> -#ifdef GDK_WINDOWING_X11
> -    if (!event->owner && GDK_IS_X11_DISPLAY(gdk_display_get_default())) {
> -        s->clip_hasdata[selection] = FALSE;
> -        return;
> -    }
> -#endif
> -
>
>
> > > -
> > > -    /* We are mostly interested when owner has changed in which case
> > > -     * we would like to let agent know about new clipboard data. */
> > >      if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER) {
> > > +        if (s->clip_grabbed[selection]) {
> > > +            /* grab was sent to the agent, so release it */
> > > +            s->clip_grabbed[selection] = FALSE;
> > > +            if (spice_main_channel_agent_test_capability(s->main, VD_AGENT_CAP_CLIPBOARD_BY_DEMAND)) {
> > > +                spice_main_channel_clipboard_selection_release(s->main, selection);
> > > +            }
> > > +        }
> > >          s->clip_hasdata[selection] = FALSE;
> > >          return;
> > >      }
> > > --
> > > 2.21.0.4.g36eb1cb9cf
> > >
> >
> > Regards,
> > Jakub
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
>
> --
> Marc-André Lureau