[spice-gtk,1/3] clipboard: accept grab only from the side with keyboard focus

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

Details

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

Not browsing as part of any series.

Commit Message

Jakub Janku Feb. 28, 2019, 7:12 p.m.
If two grab messages in opposite directions "meet" on their way
to their destinations, we end up in a state when both spice-gtk
and spice-vdagent think that the other component can provide
clipboard data. As a consequence of this, neither of them can.

This is a bug in the spice-protocol. To mitigate the issue,
accept grab only from the side that currently has keyboard focus,
this means:
(1) spice-gtk has focus ==>
    * accept grabs from vdagent,
    * ignore grabs from other apps running in the host
(2) spice-gtk doesn't have focus ==>
    * accept grabs from other apps running in the host
    * ignore grabs from vdagent

The check in clipboard_owner_change() is X11-specific,
because on Wayland, spice-gtk is first notified about the
owner-change once it gains focus.

The check in clipboard_grab() can be generic.
Note that on Wayland, calling gtk_clipboard_set_with_owner()
while not having focus doesn't have any effect anyway,
only focused clients can set clipboard.

With this patch, the race can still occur around the time
when focus changes (rare, but possible), on X11 as well as Wayland.

Related: https://gitlab.freedesktop.org/spice/spice-gtk/issues/82
Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876

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

Victor, half of this patch is basically yours,
so feel free to add signed-off or something,
in the case this gets upstream :)

---
 src/spice-gtk-session.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index b48f92a..7b7370c 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -680,6 +680,13 @@  static void clipboard_owner_change(GtkClipboard        *clipboard,
         s->clip_hasdata[selection] = FALSE;
         return;
     }
+
+    if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
+        spice_gtk_session_get_keyboard_has_focus(self)) {
+        SPICE_DEBUG("%s: spice-gtk has keyboard focus, "
+                    "ignoring grab from other app", __FUNCTION__);
+        return;
+    }
 #endif
 
     s->clip_hasdata[selection] = TRUE;
@@ -823,6 +830,12 @@  static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
     cb = get_clipboard_from_selection(s, selection);
     g_return_val_if_fail(cb != NULL, FALSE);
 
+    if (!spice_gtk_session_get_keyboard_has_focus(self)) {
+        SPICE_DEBUG("%s: spice-gtk doesn't have keyboard focus, "
+                    "ignoring grab from guest agent", __FUNCTION__);
+        return FALSE;
+    }
+
     for (n = 0; n < ntypes; ++n) {
         found = FALSE;
         for (m = 0; m < SPICE_N_ELEMENTS(atom2agent); m++) {

Comments

Hi

On Thu, Feb 28, 2019 at 8:12 PM Jakub Janků <jjanku@redhat.com> wrote:
>
> If two grab messages in opposite directions "meet" on their way
> to their destinations, we end up in a state when both spice-gtk
> and spice-vdagent think that the other component can provide
> clipboard data. As a consequence of this, neither of them can.
>
> This is a bug in the spice-protocol. To mitigate the issue,

With such as statement, you should provide detailed analysis, and
strong arguments for workarounds.

I think you are describing this conflicting situation: (all messages
are asynchronous here, A & B are either client or remote end
interchangeably)

A->B: grab
B->A: grab
A: handle B grab
B: handle A grab

Both A&B think the other end has the grab...

If we would instead explicitly release the grab, none would have it,
which could be considered an improvement:

A->B: grab
B->A: grab
A: handle B grab
A->B: release
B: handle A grab
B->A: release
B: handle A release
A: handle B release

Instead, we should probably have a priority for who wins. I suppose it
should be the client. But how to distinguish the conflict case with
the normal case? Perhaps an incremental counter (age/generation,
whatever you call it) would be enough.

Did you thought about something along those lines? Would you be able
to come up with protocol changes for that?

At this point, I think we could use some nice sequence diagrams in the spec!


> accept grab only from the side that currently has keyboard focus,
> this means:
> (1) spice-gtk has focus ==>
>     * accept grabs from vdagent,
>     * ignore grabs from other apps running in the host

I have a hard time understanding why keyboard focus is related to
clipboard. clipboard can be interacted with mouse only, or
independently from any user input. Or from multiple inputs. I try to
fit the keyboard input this into clipboard interaction issues, and I
don't think that helps much.

> (2) spice-gtk doesn't have focus ==>
>     * accept grabs from other apps running in the host
>     * ignore grabs from vdagent
>
> The check in clipboard_owner_change() is X11-specific,
> because on Wayland, spice-gtk is first notified about the
> owner-change once it gains focus.
>
> The check in clipboard_grab() can be generic.
> Note that on Wayland, calling gtk_clipboard_set_with_owner()
> while not having focus doesn't have any effect anyway,
> only focused clients can set clipboard.
>
> With this patch, the race can still occur around the time
> when focus changes (rare, but possible), on X11 as well as Wayland.
>
> Related: https://gitlab.freedesktop.org/spice/spice-gtk/issues/82
> Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876
>
> Signed-off-by: Jakub Janků <jjanku@redhat.com>
> ---
>
> Victor, half of this patch is basically yours,
> so feel free to add signed-off or something,
> in the case this gets upstream :)
>
> ---
>  src/spice-gtk-session.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index b48f92a..7b7370c 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -680,6 +680,13 @@ static void clipboard_owner_change(GtkClipboard        *clipboard,
>          s->clip_hasdata[selection] = FALSE;
>          return;
>      }
> +
> +    if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&

Note, gdk_display_get_default() is probably wrong.

gtk_widget_get_display () is probably better. Not a big deal, needs to
be changed over the whole tree I guess.





> +        spice_gtk_session_get_keyboard_has_focus(self)) {
> +        SPICE_DEBUG("%s: spice-gtk has keyboard focus, "
> +                    "ignoring grab from other app", __FUNCTION__);
> +        return;
> +    }
>  #endif
>
>      s->clip_hasdata[selection] = TRUE;
> @@ -823,6 +830,12 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
>      cb = get_clipboard_from_selection(s, selection);
>      g_return_val_if_fail(cb != NULL, FALSE);
>
> +    if (!spice_gtk_session_get_keyboard_has_focus(self)) {
> +        SPICE_DEBUG("%s: spice-gtk doesn't have keyboard focus, "
> +                    "ignoring grab from guest agent", __FUNCTION__);
> +        return FALSE;
> +    }
> +
>      for (n = 0; n < ntypes; ++n) {
>          found = FALSE;
>          for (m = 0; m < SPICE_N_ELEMENTS(atom2agent); m++) {
> --
> 2.20.1
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



--
Marc-André Lureau
Hi,

thanks for having a look!

On Wed, Mar 6, 2019 at 6:42 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:
> >
> > If two grab messages in opposite directions "meet" on their way
> > to their destinations, we end up in a state when both spice-gtk
> > and spice-vdagent think that the other component can provide
> > clipboard data. As a consequence of this, neither of them can.
> >
> > This is a bug in the spice-protocol. To mitigate the issue,
>
> With such as statement, you should provide detailed analysis, and
> strong arguments for workarounds.

Depends on what you consider to be "detailed analysis". Since you
correctly restated the problem with your own words, it seems like my
rather short description served its purpose :)
Also please note that I've tried to provide a more detailed analysis
of the issue reported by James Harvey earlier in [0]

[0] https://lists.freedesktop.org/archives/spice-devel/2019-January/047614.html

As for the arguments, I've actually expressed my main argument in the
cover letter, have you read it?
"Intention of these patches is to make spice-gtk
behave reasonably well with older agents."
I would like to have this patch reviewed with this in mind.
So this patch NOT trying to be a final solution to the problem.

If you look at the race condition from the standpoint of spice-gtk,
the situation looks the following:
1) spice-gtk sends grab
2) spice-gtk receives grab
You can't tell if it's a race or not. The normal behaviour can look the same.

Now, if you consider fix in spice-gtk only (to make older vdagents
play nicely with new spice-gtk), I see these possible mitigations:
a) measure time between 1) and 2) and discard one grab if the elapsed
time is too short:
this solution is rather empirical and seems unreliable to me
b) accept grab only from one side at a moment - that is what this patch does

>
> I think you are describing this conflicting situation: (all messages
> are asynchronous here, A & B are either client or remote end
> interchangeably)
>
> A->B: grab
> B->A: grab
> A: handle B grab
> B: handle A grab
>
> Both A&B think the other end has the grab...
>
> If we would instead explicitly release the grab, none would have it,
> which could be considered an improvement:

Yes, a slight improvement. However, the original grabs both in client
and guest would be lost, which is unwanted.
>
> A->B: grab
> B->A: grab
> A: handle B grab
> A->B: release
> B: handle A grab
> B->A: release
> B: handle A release
> A: handle B release
>
> Instead, we should probably have a priority for who wins. I suppose it
> should be the client.

Not necessarily. Consider James' case: if we make the client win, the
clipboard manager in the client would take over the clipboard in the
guest. The clipboard manager filters out some of the targets and
additionally, the "marching ants" in Excel would disappear.

>But how to distinguish the conflict case with
> the normal case? Perhaps an incremental counter (age/generation,
> whatever you call it) would be enough.

This would need a protocol change.
So given my note in the cover letter, this comment is quite unrelated
to this patch. I would prefer to discuss the protocol change later.
>
> Did you thought about something along those lines? Would you be able
> to come up with protocol changes for that?
>
> At this point, I think we could use some nice sequence diagrams in the spec!
>
>
> > accept grab only from the side that currently has keyboard focus,
> > this means:
> > (1) spice-gtk has focus ==>
> >     * accept grabs from vdagent,
> >     * ignore grabs from other apps running in the host
>
> I have a hard time understanding why keyboard focus is related to
> clipboard. clipboard can be interacted with mouse only, or
> independently from any user input. Or from multiple inputs. I try to
> fit the keyboard input this into clipboard interaction issues, and I
> don't think that helps much.

If spice-gtk has keyboard focus, the user interacts with the guest system.
If it doesn't, user interacts with the client system.

Sure, the clipboard can change without user's input. E.g. with some
automation systems, as you pointer out earlier.
However, this is not the usual use case. And with this patch, I'm
trying to fix the most common scenario to provide a better default
experience.
>
> > (2) spice-gtk doesn't have focus ==>
> >     * accept grabs from other apps running in the host
> >     * ignore grabs from vdagent
> >
> > The check in clipboard_owner_change() is X11-specific,
> > because on Wayland, spice-gtk is first notified about the
> > owner-change once it gains focus.
> >
> > The check in clipboard_grab() can be generic.
> > Note that on Wayland, calling gtk_clipboard_set_with_owner()
> > while not having focus doesn't have any effect anyway,
> > only focused clients can set clipboard.
> >
> > With this patch, the race can still occur around the time
> > when focus changes (rare, but possible), on X11 as well as Wayland.
> >
> > Related: https://gitlab.freedesktop.org/spice/spice-gtk/issues/82
> > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876
> >
> > Signed-off-by: Jakub Janků <jjanku@redhat.com>
> > ---
> >
> > Victor, half of this patch is basically yours,
> > so feel free to add signed-off or something,
> > in the case this gets upstream :)
> >
> > ---
> >  src/spice-gtk-session.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > index b48f92a..7b7370c 100644
> > --- a/src/spice-gtk-session.c
> > +++ b/src/spice-gtk-session.c
> > @@ -680,6 +680,13 @@ static void clipboard_owner_change(GtkClipboard        *clipboard,
> >          s->clip_hasdata[selection] = FALSE;
> >          return;
> >      }
> > +
> > +    if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
>
> Note, gdk_display_get_default() is probably wrong.
>
> gtk_widget_get_display () is probably better. Not a big deal, needs to
> be changed over the whole tree I guess.

True.
>
>
>
>
>
> > +        spice_gtk_session_get_keyboard_has_focus(self)) {
> > +        SPICE_DEBUG("%s: spice-gtk has keyboard focus, "
> > +                    "ignoring grab from other app", __FUNCTION__);
> > +        return;
> > +    }
> >  #endif
> >
> >      s->clip_hasdata[selection] = TRUE;
> > @@ -823,6 +830,12 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
> >      cb = get_clipboard_from_selection(s, selection);
> >      g_return_val_if_fail(cb != NULL, FALSE);
> >
> > +    if (!spice_gtk_session_get_keyboard_has_focus(self)) {
> > +        SPICE_DEBUG("%s: spice-gtk doesn't have keyboard focus, "
> > +                    "ignoring grab from guest agent", __FUNCTION__);
> > +        return FALSE;
> > +    }
> > +
> >      for (n = 0; n < ntypes; ++n) {
> >          found = FALSE;
> >          for (m = 0; m < SPICE_N_ELEMENTS(atom2agent); m++) {
> > --
> > 2.20.1
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
>
> --
> Marc-André Lureau

Cheers,
Jakub
Hi

On Thu, Mar 7, 2019 at 10:36 PM Jakub Janku <jjanku@redhat.com> wrote:
>
> Hi,
>
> thanks for having a look!
>
> On Wed, Mar 6, 2019 at 6:42 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:
> > >
> > > If two grab messages in opposite directions "meet" on their way
> > > to their destinations, we end up in a state when both spice-gtk
> > > and spice-vdagent think that the other component can provide
> > > clipboard data. As a consequence of this, neither of them can.
> > >
> > > This is a bug in the spice-protocol. To mitigate the issue,
> >
> > With such as statement, you should provide detailed analysis, and
> > strong arguments for workarounds.
>
> Depends on what you consider to be "detailed analysis". Since you
> correctly restated the problem with your own words, it seems like my
> rather short description served its purpose :)
> Also please note that I've tried to provide a more detailed analysis
> of the issue reported by James Harvey earlier in [0]
>
> [0] https://lists.freedesktop.org/archives/spice-devel/2019-January/047614.html
>
> As for the arguments, I've actually expressed my main argument in the
> cover letter, have you read it?
> "Intention of these patches is to make spice-gtk
> behave reasonably well with older agents."
> I would like to have this patch reviewed with this in mind.
> So this patch NOT trying to be a final solution to the problem.
>
> If you look at the race condition from the standpoint of spice-gtk,
> the situation looks the following:
> 1) spice-gtk sends grab
> 2) spice-gtk receives grab
> You can't tell if it's a race or not. The normal behaviour can look the same.
>
> Now, if you consider fix in spice-gtk only (to make older vdagents
> play nicely with new spice-gtk), I see these possible mitigations:
> a) measure time between 1) and 2) and discard one grab if the elapsed
> time is too short:
> this solution is rather empirical and seems unreliable to me
> b) accept grab only from one side at a moment - that is what this patch does
>
> >
> > I think you are describing this conflicting situation: (all messages
> > are asynchronous here, A & B are either client or remote end
> > interchangeably)
> >
> > A->B: grab
> > B->A: grab
> > A: handle B grab
> > B: handle A grab
> >
> > Both A&B think the other end has the grab...
> >
> > If we would instead explicitly release the grab, none would have it,
> > which could be considered an improvement:
>
> Yes, a slight improvement. However, the original grabs both in client
> and guest would be lost, which is unwanted.
> >
> > A->B: grab
> > B->A: grab
> > A: handle B grab
> > A->B: release
> > B: handle A grab
> > B->A: release
> > B: handle A release
> > A: handle B release
> >
> > Instead, we should probably have a priority for who wins. I suppose it
> > should be the client.
>
> Not necessarily. Consider James' case: if we make the client win, the
> clipboard manager in the client would take over the clipboard in the
> guest. The clipboard manager filters out some of the targets and
> additionally, the "marching ants" in Excel would disappear.
>
> >But how to distinguish the conflict case with
> > the normal case? Perhaps an incremental counter (age/generation,
> > whatever you call it) would be enough.
>
> This would need a protocol change.
> So given my note in the cover letter, this comment is quite unrelated
> to this patch. I would prefer to discuss the protocol change later.
> >
> > Did you thought about something along those lines? Would you be able
> > to come up with protocol changes for that?
> >
> > At this point, I think we could use some nice sequence diagrams in the spec!
> >
> >
> > > accept grab only from the side that currently has keyboard focus,
> > > this means:
> > > (1) spice-gtk has focus ==>
> > >     * accept grabs from vdagent,
> > >     * ignore grabs from other apps running in the host
> >
> > I have a hard time understanding why keyboard focus is related to
> > clipboard. clipboard can be interacted with mouse only, or
> > independently from any user input. Or from multiple inputs. I try to
> > fit the keyboard input this into clipboard interaction issues, and I
> > don't think that helps much.
>
> If spice-gtk has keyboard focus, the user interacts with the guest system.
> If it doesn't, user interacts with the client system.
>
> Sure, the clipboard can change without user's input. E.g. with some
> automation systems, as you pointer out earlier.
> However, this is not the usual use case. And with this patch, I'm
> trying to fix the most common scenario to provide a better default
> experience.


How reproducible is the problem you are fixing? Isn't this a corner
case? Can you describe a use case in the patch?

I think we shouldn't couple clipboard handling and keyboard focus.

Adding layers of weird tweaks such as this one makes code more
complicated and more prone to strange behaviors that are difficult to
debug.

Imho, we need to fix it properly instead.

> >
> > > (2) spice-gtk doesn't have focus ==>
> > >     * accept grabs from other apps running in the host
> > >     * ignore grabs from vdagent
> > >
> > > The check in clipboard_owner_change() is X11-specific,
> > > because on Wayland, spice-gtk is first notified about the
> > > owner-change once it gains focus.
> > >
> > > The check in clipboard_grab() can be generic.
> > > Note that on Wayland, calling gtk_clipboard_set_with_owner()
> > > while not having focus doesn't have any effect anyway,
> > > only focused clients can set clipboard.
> > >
> > > With this patch, the race can still occur around the time
> > > when focus changes (rare, but possible), on X11 as well as Wayland.
> > >
> > > Related: https://gitlab.freedesktop.org/spice/spice-gtk/issues/82
> > > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876
> > >
> > > Signed-off-by: Jakub Janků <jjanku@redhat.com>
> > > ---
> > >
> > > Victor, half of this patch is basically yours,
> > > so feel free to add signed-off or something,
> > > in the case this gets upstream :)
> > >
> > > ---
> > >  src/spice-gtk-session.c | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > >
> > > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > > index b48f92a..7b7370c 100644
> > > --- a/src/spice-gtk-session.c
> > > +++ b/src/spice-gtk-session.c
> > > @@ -680,6 +680,13 @@ static void clipboard_owner_change(GtkClipboard        *clipboard,
> > >          s->clip_hasdata[selection] = FALSE;
> > >          return;
> > >      }
> > > +
> > > +    if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
> >
> > Note, gdk_display_get_default() is probably wrong.
> >
> > gtk_widget_get_display () is probably better. Not a big deal, needs to
> > be changed over the whole tree I guess.
>
> True.
> >
> >
> >
> >
> >
> > > +        spice_gtk_session_get_keyboard_has_focus(self)) {
> > > +        SPICE_DEBUG("%s: spice-gtk has keyboard focus, "
> > > +                    "ignoring grab from other app", __FUNCTION__);
> > > +        return;
> > > +    }
> > >  #endif
> > >
> > >      s->clip_hasdata[selection] = TRUE;
> > > @@ -823,6 +830,12 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
> > >      cb = get_clipboard_from_selection(s, selection);
> > >      g_return_val_if_fail(cb != NULL, FALSE);
> > >
> > > +    if (!spice_gtk_session_get_keyboard_has_focus(self)) {
> > > +        SPICE_DEBUG("%s: spice-gtk doesn't have keyboard focus, "
> > > +                    "ignoring grab from guest agent", __FUNCTION__);
> > > +        return FALSE;
> > > +    }
> > > +
> > >      for (n = 0; n < ntypes; ++n) {
> > >          found = FALSE;
> > >          for (m = 0; m < SPICE_N_ELEMENTS(atom2agent); m++) {
> > > --
> > > 2.20.1
> > >
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
> >
> >
> > --
> > Marc-André Lureau
>
> Cheers,
> Jakub
Hi,

On Fri, Mar 8, 2019 at 1:15 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Thu, Mar 7, 2019 at 10:36 PM Jakub Janku <jjanku@redhat.com> wrote:
> >
> > Hi,
> >
> > thanks for having a look!
> >
> > On Wed, Mar 6, 2019 at 6:42 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:
> > > >
> > > > If two grab messages in opposite directions "meet" on their way
> > > > to their destinations, we end up in a state when both spice-gtk
> > > > and spice-vdagent think that the other component can provide
> > > > clipboard data. As a consequence of this, neither of them can.
> > > >
> > > > This is a bug in the spice-protocol. To mitigate the issue,
> > >
> > > With such as statement, you should provide detailed analysis, and
> > > strong arguments for workarounds.
> >
> > Depends on what you consider to be "detailed analysis". Since you
> > correctly restated the problem with your own words, it seems like my
> > rather short description served its purpose :)
> > Also please note that I've tried to provide a more detailed analysis
> > of the issue reported by James Harvey earlier in [0]
> >
> > [0] https://lists.freedesktop.org/archives/spice-devel/2019-January/047614.html
> >
> > As for the arguments, I've actually expressed my main argument in the
> > cover letter, have you read it?
> > "Intention of these patches is to make spice-gtk
> > behave reasonably well with older agents."
> > I would like to have this patch reviewed with this in mind.
> > So this patch NOT trying to be a final solution to the problem.
> >
> > If you look at the race condition from the standpoint of spice-gtk,
> > the situation looks the following:
> > 1) spice-gtk sends grab
> > 2) spice-gtk receives grab
> > You can't tell if it's a race or not. The normal behaviour can look the same.
> >
> > Now, if you consider fix in spice-gtk only (to make older vdagents
> > play nicely with new spice-gtk), I see these possible mitigations:
> > a) measure time between 1) and 2) and discard one grab if the elapsed
> > time is too short:
> > this solution is rather empirical and seems unreliable to me
> > b) accept grab only from one side at a moment - that is what this patch does
> >
> > >
> > > I think you are describing this conflicting situation: (all messages
> > > are asynchronous here, A & B are either client or remote end
> > > interchangeably)
> > >
> > > A->B: grab
> > > B->A: grab
> > > A: handle B grab
> > > B: handle A grab
> > >
> > > Both A&B think the other end has the grab...
> > >
> > > If we would instead explicitly release the grab, none would have it,
> > > which could be considered an improvement:
> >
> > Yes, a slight improvement. However, the original grabs both in client
> > and guest would be lost, which is unwanted.
> > >
> > > A->B: grab
> > > B->A: grab
> > > A: handle B grab
> > > A->B: release
> > > B: handle A grab
> > > B->A: release
> > > B: handle A release
> > > A: handle B release
> > >
> > > Instead, we should probably have a priority for who wins. I suppose it
> > > should be the client.
> >
> > Not necessarily. Consider James' case: if we make the client win, the
> > clipboard manager in the client would take over the clipboard in the
> > guest. The clipboard manager filters out some of the targets and
> > additionally, the "marching ants" in Excel would disappear.
> >
> > >But how to distinguish the conflict case with
> > > the normal case? Perhaps an incremental counter (age/generation,
> > > whatever you call it) would be enough.
> >
> > This would need a protocol change.
> > So given my note in the cover letter, this comment is quite unrelated
> > to this patch. I would prefer to discuss the protocol change later.
> > >
> > > Did you thought about something along those lines? Would you be able
> > > to come up with protocol changes for that?
> > >
> > > At this point, I think we could use some nice sequence diagrams in the spec!
> > >
> > >
> > > > accept grab only from the side that currently has keyboard focus,
> > > > this means:
> > > > (1) spice-gtk has focus ==>
> > > >     * accept grabs from vdagent,
> > > >     * ignore grabs from other apps running in the host
> > >
> > > I have a hard time understanding why keyboard focus is related to
> > > clipboard. clipboard can be interacted with mouse only, or
> > > independently from any user input. Or from multiple inputs. I try to
> > > fit the keyboard input this into clipboard interaction issues, and I
> > > don't think that helps much.
> >
> > If spice-gtk has keyboard focus, the user interacts with the guest system.
> > If it doesn't, user interacts with the client system.
> >
> > Sure, the clipboard can change without user's input. E.g. with some
> > automation systems, as you pointer out earlier.
> > However, this is not the usual use case. And with this patch, I'm
> > trying to fix the most common scenario to provide a better default
> > experience.
>
>
> How reproducible is the problem you are fixing? Isn't this a corner
> case? Can you describe a use case in the patch?

That's hard to say. There have been at least 3 related reports so far:
https://gitlab.freedesktop.org/spice/win32/vd_agent/issues/6
https://gitlab.freedesktop.org/spice/spice-gtk/issues/36
https://bugzilla.redhat.com/show_bug.cgi?id=1594876

Wayland is mostly fine. X11 environment with a clipboard manager is
the one most prone to errors.
>
> I think we shouldn't couple clipboard handling and keyboard focus.
>
> Adding layers of weird tweaks such as this one makes code more
> complicated and more prone to strange behaviors that are difficult to
> debug.

I would understand if you nack-ed the 2/3 patch to avoid additional
complexity as it really does handle an edge case.
But this one is mere 13 additions. Once we have a proper fix, we can
just enclose this code with
    |    if (spice_main_channel_agent_test_capability())

So since this patch is so simple, I don't see a reason to leave the
behavior with the old agents broken.
>
> Imho, we need to fix it properly instead.

I agree, but the proper fix can follow this one, imho :)

Cheers,
Jakub
>
> > >
> > > > (2) spice-gtk doesn't have focus ==>
> > > >     * accept grabs from other apps running in the host
> > > >     * ignore grabs from vdagent
> > > >
> > > > The check in clipboard_owner_change() is X11-specific,
> > > > because on Wayland, spice-gtk is first notified about the
> > > > owner-change once it gains focus.
> > > >
> > > > The check in clipboard_grab() can be generic.
> > > > Note that on Wayland, calling gtk_clipboard_set_with_owner()
> > > > while not having focus doesn't have any effect anyway,
> > > > only focused clients can set clipboard.
> > > >
> > > > With this patch, the race can still occur around the time
> > > > when focus changes (rare, but possible), on X11 as well as Wayland.
> > > >
> > > > Related: https://gitlab.freedesktop.org/spice/spice-gtk/issues/82
> > > > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876
> > > >
> > > > Signed-off-by: Jakub Janků <jjanku@redhat.com>
> > > > ---
> > > >
> > > > Victor, half of this patch is basically yours,
> > > > so feel free to add signed-off or something,
> > > > in the case this gets upstream :)
> > > >
> > > > ---
> > > >  src/spice-gtk-session.c | 13 +++++++++++++
> > > >  1 file changed, 13 insertions(+)
> > > >
> > > > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > > > index b48f92a..7b7370c 100644
> > > > --- a/src/spice-gtk-session.c
> > > > +++ b/src/spice-gtk-session.c
> > > > @@ -680,6 +680,13 @@ static void clipboard_owner_change(GtkClipboard        *clipboard,
> > > >          s->clip_hasdata[selection] = FALSE;
> > > >          return;
> > > >      }
> > > > +
> > > > +    if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
> > >
> > > Note, gdk_display_get_default() is probably wrong.
> > >
> > > gtk_widget_get_display () is probably better. Not a big deal, needs to
> > > be changed over the whole tree I guess.
> >
> > True.
> > >
> > >
> > >
> > >
> > >
> > > > +        spice_gtk_session_get_keyboard_has_focus(self)) {
> > > > +        SPICE_DEBUG("%s: spice-gtk has keyboard focus, "
> > > > +                    "ignoring grab from other app", __FUNCTION__);
> > > > +        return;
> > > > +    }
> > > >  #endif
> > > >
> > > >      s->clip_hasdata[selection] = TRUE;
> > > > @@ -823,6 +830,12 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
> > > >      cb = get_clipboard_from_selection(s, selection);
> > > >      g_return_val_if_fail(cb != NULL, FALSE);
> > > >
> > > > +    if (!spice_gtk_session_get_keyboard_has_focus(self)) {
> > > > +        SPICE_DEBUG("%s: spice-gtk doesn't have keyboard focus, "
> > > > +                    "ignoring grab from guest agent", __FUNCTION__);
> > > > +        return FALSE;
> > > > +    }
> > > > +
> > > >      for (n = 0; n < ntypes; ++n) {
> > > >          found = FALSE;
> > > >          for (m = 0; m < SPICE_N_ELEMENTS(atom2agent); m++) {
> > > > --
> > > > 2.20.1
> > > >
> > > > _______________________________________________
> > > > Spice-devel mailing list
> > > > Spice-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > >
> > >
> > >
> > > --
> > > Marc-André Lureau
> >
> > Cheers,
> > Jakub
>
>
>
> --
> Marc-André Lureau
Hi

On Fri, Mar 8, 2019 at 1:41 PM Jakub Janku <jjanku@redhat.com> wrote:
>
> Hi,
>
> On Fri, Mar 8, 2019 at 1:15 PM Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
> >
> > Hi
> >
> > On Thu, Mar 7, 2019 at 10:36 PM Jakub Janku <jjanku@redhat.com> wrote:
> > >
> > > Hi,
> > >
> > > thanks for having a look!
> > >
> > > On Wed, Mar 6, 2019 at 6:42 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:
> > > > >
> > > > > If two grab messages in opposite directions "meet" on their way
> > > > > to their destinations, we end up in a state when both spice-gtk
> > > > > and spice-vdagent think that the other component can provide
> > > > > clipboard data. As a consequence of this, neither of them can.
> > > > >
> > > > > This is a bug in the spice-protocol. To mitigate the issue,
> > > >
> > > > With such as statement, you should provide detailed analysis, and
> > > > strong arguments for workarounds.
> > >
> > > Depends on what you consider to be "detailed analysis". Since you
> > > correctly restated the problem with your own words, it seems like my
> > > rather short description served its purpose :)
> > > Also please note that I've tried to provide a more detailed analysis
> > > of the issue reported by James Harvey earlier in [0]
> > >
> > > [0] https://lists.freedesktop.org/archives/spice-devel/2019-January/047614.html
> > >
> > > As for the arguments, I've actually expressed my main argument in the
> > > cover letter, have you read it?
> > > "Intention of these patches is to make spice-gtk
> > > behave reasonably well with older agents."
> > > I would like to have this patch reviewed with this in mind.
> > > So this patch NOT trying to be a final solution to the problem.
> > >
> > > If you look at the race condition from the standpoint of spice-gtk,
> > > the situation looks the following:
> > > 1) spice-gtk sends grab
> > > 2) spice-gtk receives grab
> > > You can't tell if it's a race or not. The normal behaviour can look the same.
> > >
> > > Now, if you consider fix in spice-gtk only (to make older vdagents
> > > play nicely with new spice-gtk), I see these possible mitigations:
> > > a) measure time between 1) and 2) and discard one grab if the elapsed
> > > time is too short:
> > > this solution is rather empirical and seems unreliable to me
> > > b) accept grab only from one side at a moment - that is what this patch does
> > >
> > > >
> > > > I think you are describing this conflicting situation: (all messages
> > > > are asynchronous here, A & B are either client or remote end
> > > > interchangeably)
> > > >
> > > > A->B: grab
> > > > B->A: grab
> > > > A: handle B grab
> > > > B: handle A grab
> > > >
> > > > Both A&B think the other end has the grab...
> > > >
> > > > If we would instead explicitly release the grab, none would have it,
> > > > which could be considered an improvement:
> > >
> > > Yes, a slight improvement. However, the original grabs both in client
> > > and guest would be lost, which is unwanted.
> > > >
> > > > A->B: grab
> > > > B->A: grab
> > > > A: handle B grab
> > > > A->B: release
> > > > B: handle A grab
> > > > B->A: release
> > > > B: handle A release
> > > > A: handle B release
> > > >
> > > > Instead, we should probably have a priority for who wins. I suppose it
> > > > should be the client.
> > >
> > > Not necessarily. Consider James' case: if we make the client win, the
> > > clipboard manager in the client would take over the clipboard in the
> > > guest. The clipboard manager filters out some of the targets and
> > > additionally, the "marching ants" in Excel would disappear.
> > >
> > > >But how to distinguish the conflict case with
> > > > the normal case? Perhaps an incremental counter (age/generation,
> > > > whatever you call it) would be enough.
> > >
> > > This would need a protocol change.
> > > So given my note in the cover letter, this comment is quite unrelated
> > > to this patch. I would prefer to discuss the protocol change later.
> > > >
> > > > Did you thought about something along those lines? Would you be able
> > > > to come up with protocol changes for that?
> > > >
> > > > At this point, I think we could use some nice sequence diagrams in the spec!
> > > >
> > > >
> > > > > accept grab only from the side that currently has keyboard focus,
> > > > > this means:
> > > > > (1) spice-gtk has focus ==>
> > > > >     * accept grabs from vdagent,
> > > > >     * ignore grabs from other apps running in the host
> > > >
> > > > I have a hard time understanding why keyboard focus is related to
> > > > clipboard. clipboard can be interacted with mouse only, or
> > > > independently from any user input. Or from multiple inputs. I try to
> > > > fit the keyboard input this into clipboard interaction issues, and I
> > > > don't think that helps much.
> > >
> > > If spice-gtk has keyboard focus, the user interacts with the guest system.
> > > If it doesn't, user interacts with the client system.
> > >
> > > Sure, the clipboard can change without user's input. E.g. with some
> > > automation systems, as you pointer out earlier.
> > > However, this is not the usual use case. And with this patch, I'm
> > > trying to fix the most common scenario to provide a better default
> > > experience.
> >
> >
> > How reproducible is the problem you are fixing? Isn't this a corner
> > case? Can you describe a use case in the patch?
>
> That's hard to say. There have been at least 3 related reports so far:
> https://gitlab.freedesktop.org/spice/win32/vd_agent/issues/6
> https://gitlab.freedesktop.org/spice/spice-gtk/issues/36
> https://bugzilla.redhat.com/show_bug.cgi?id=1594876
>

3 reports, and no summary of a use-case to reproduce the problem? I
suppose I will have to read the issues.

> Wayland is mostly fine. X11 environment with a clipboard manager is
> the one most prone to errors.

That's my setup fwiw, I use "clipboard indicator" with gnome & x11
(most of the time, sometime I switch to wayland).

> >
> > I think we shouldn't couple clipboard handling and keyboard focus.
> >
> > Adding layers of weird tweaks such as this one makes code more
> > complicated and more prone to strange behaviors that are difficult to
> > debug.
>
> I would understand if you nack-ed the 2/3 patch to avoid additional
> complexity as it really does handle an edge case.
> But this one is mere 13 additions. Once we have a proper fix, we can
> just enclose this code with
>     |    if (spice_main_channel_agent_test_capability())
>
> So since this patch is so simple, I don't see a reason to leave the
> behavior with the old agents broken.

Ok,
As I said, I find it hard to mix "keyboard focus" with clipboard
handling. Give me a few more days to think through this a bit :)

(if I could have a broken case I can study, that would help me - I'll
eventually dig in the reported issues)

> >
> > Imho, we need to fix it properly instead.
>
> I agree, but the proper fix can follow this one, imho :)
>
> Cheers,
> Jakub
> >
> > > >
> > > > > (2) spice-gtk doesn't have focus ==>
> > > > >     * accept grabs from other apps running in the host
> > > > >     * ignore grabs from vdagent
> > > > >
> > > > > The check in clipboard_owner_change() is X11-specific,
> > > > > because on Wayland, spice-gtk is first notified about the
> > > > > owner-change once it gains focus.
> > > > >
> > > > > The check in clipboard_grab() can be generic.
> > > > > Note that on Wayland, calling gtk_clipboard_set_with_owner()
> > > > > while not having focus doesn't have any effect anyway,
> > > > > only focused clients can set clipboard.
> > > > >
> > > > > With this patch, the race can still occur around the time
> > > > > when focus changes (rare, but possible), on X11 as well as Wayland.
> > > > >
> > > > > Related: https://gitlab.freedesktop.org/spice/spice-gtk/issues/82
> > > > > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876
> > > > >
> > > > > Signed-off-by: Jakub Janků <jjanku@redhat.com>
> > > > > ---
> > > > >
> > > > > Victor, half of this patch is basically yours,
> > > > > so feel free to add signed-off or something,
> > > > > in the case this gets upstream :)
> > > > >
> > > > > ---
> > > > >  src/spice-gtk-session.c | 13 +++++++++++++
> > > > >  1 file changed, 13 insertions(+)
> > > > >
> > > > > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > > > > index b48f92a..7b7370c 100644
> > > > > --- a/src/spice-gtk-session.c
> > > > > +++ b/src/spice-gtk-session.c
> > > > > @@ -680,6 +680,13 @@ static void clipboard_owner_change(GtkClipboard        *clipboard,
> > > > >          s->clip_hasdata[selection] = FALSE;
> > > > >          return;
> > > > >      }
> > > > > +
> > > > > +    if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
> > > >
> > > > Note, gdk_display_get_default() is probably wrong.
> > > >
> > > > gtk_widget_get_display () is probably better. Not a big deal, needs to
> > > > be changed over the whole tree I guess.
> > >
> > > True.
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > > +        spice_gtk_session_get_keyboard_has_focus(self)) {
> > > > > +        SPICE_DEBUG("%s: spice-gtk has keyboard focus, "
> > > > > +                    "ignoring grab from other app", __FUNCTION__);
> > > > > +        return;
> > > > > +    }
> > > > >  #endif
> > > > >
> > > > >      s->clip_hasdata[selection] = TRUE;
> > > > > @@ -823,6 +830,12 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
> > > > >      cb = get_clipboard_from_selection(s, selection);
> > > > >      g_return_val_if_fail(cb != NULL, FALSE);
> > > > >
> > > > > +    if (!spice_gtk_session_get_keyboard_has_focus(self)) {
> > > > > +        SPICE_DEBUG("%s: spice-gtk doesn't have keyboard focus, "
> > > > > +                    "ignoring grab from guest agent", __FUNCTION__);
> > > > > +        return FALSE;
> > > > > +    }
> > > > > +
> > > > >      for (n = 0; n < ntypes; ++n) {
> > > > >          found = FALSE;
> > > > >          for (m = 0; m < SPICE_N_ELEMENTS(atom2agent); m++) {
> > > > > --
> > > > > 2.20.1
> > > > >
> > > > > _______________________________________________
> > > > > Spice-devel mailing list
> > > > > Spice-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > > >
> > > >
> > > >
> > > > --
> > > > Marc-André Lureau
> > >
> > > Cheers,
> > > Jakub
> >
> >
> >
> > --
> > Marc-André Lureau
Hi again,

On Fri, Mar 8, 2019 at 4:00 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Fri, Mar 8, 2019 at 1:41 PM Jakub Janku <jjanku@redhat.com> wrote:
> >
> > Hi,
> >
> > On Fri, Mar 8, 2019 at 1:15 PM Marc-André Lureau
> > <marcandre.lureau@gmail.com> wrote:
> > >
> > > Hi
> > >
> > > On Thu, Mar 7, 2019 at 10:36 PM Jakub Janku <jjanku@redhat.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > thanks for having a look!
> > > >
> > > > On Wed, Mar 6, 2019 at 6:42 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:
> > > > > >
> > > > > > If two grab messages in opposite directions "meet" on their way
> > > > > > to their destinations, we end up in a state when both spice-gtk
> > > > > > and spice-vdagent think that the other component can provide
> > > > > > clipboard data. As a consequence of this, neither of them can.
> > > > > >
> > > > > > This is a bug in the spice-protocol. To mitigate the issue,
> > > > >
> > > > > With such as statement, you should provide detailed analysis, and
> > > > > strong arguments for workarounds.
> > > >
> > > > Depends on what you consider to be "detailed analysis". Since you
> > > > correctly restated the problem with your own words, it seems like my
> > > > rather short description served its purpose :)
> > > > Also please note that I've tried to provide a more detailed analysis
> > > > of the issue reported by James Harvey earlier in [0]
> > > >
> > > > [0] https://lists.freedesktop.org/archives/spice-devel/2019-January/047614.html
> > > >
> > > > As for the arguments, I've actually expressed my main argument in the
> > > > cover letter, have you read it?
> > > > "Intention of these patches is to make spice-gtk
> > > > behave reasonably well with older agents."
> > > > I would like to have this patch reviewed with this in mind.
> > > > So this patch NOT trying to be a final solution to the problem.
> > > >
> > > > If you look at the race condition from the standpoint of spice-gtk,
> > > > the situation looks the following:
> > > > 1) spice-gtk sends grab
> > > > 2) spice-gtk receives grab
> > > > You can't tell if it's a race or not. The normal behaviour can look the same.
> > > >
> > > > Now, if you consider fix in spice-gtk only (to make older vdagents
> > > > play nicely with new spice-gtk), I see these possible mitigations:
> > > > a) measure time between 1) and 2) and discard one grab if the elapsed
> > > > time is too short:
> > > > this solution is rather empirical and seems unreliable to me
> > > > b) accept grab only from one side at a moment - that is what this patch does
> > > >
> > > > >
> > > > > I think you are describing this conflicting situation: (all messages
> > > > > are asynchronous here, A & B are either client or remote end
> > > > > interchangeably)
> > > > >
> > > > > A->B: grab
> > > > > B->A: grab
> > > > > A: handle B grab
> > > > > B: handle A grab
> > > > >
> > > > > Both A&B think the other end has the grab...
> > > > >
> > > > > If we would instead explicitly release the grab, none would have it,
> > > > > which could be considered an improvement:
> > > >
> > > > Yes, a slight improvement. However, the original grabs both in client
> > > > and guest would be lost, which is unwanted.
> > > > >
> > > > > A->B: grab
> > > > > B->A: grab
> > > > > A: handle B grab
> > > > > A->B: release
> > > > > B: handle A grab
> > > > > B->A: release
> > > > > B: handle A release
> > > > > A: handle B release
> > > > >
> > > > > Instead, we should probably have a priority for who wins. I suppose it
> > > > > should be the client.
> > > >
> > > > Not necessarily. Consider James' case: if we make the client win, the
> > > > clipboard manager in the client would take over the clipboard in the
> > > > guest. The clipboard manager filters out some of the targets and
> > > > additionally, the "marching ants" in Excel would disappear.
> > > >
> > > > >But how to distinguish the conflict case with
> > > > > the normal case? Perhaps an incremental counter (age/generation,
> > > > > whatever you call it) would be enough.
> > > >
> > > > This would need a protocol change.
> > > > So given my note in the cover letter, this comment is quite unrelated
> > > > to this patch. I would prefer to discuss the protocol change later.
> > > > >
> > > > > Did you thought about something along those lines? Would you be able
> > > > > to come up with protocol changes for that?
> > > > >
> > > > > At this point, I think we could use some nice sequence diagrams in the spec!
> > > > >
> > > > >
> > > > > > accept grab only from the side that currently has keyboard focus,
> > > > > > this means:
> > > > > > (1) spice-gtk has focus ==>
> > > > > >     * accept grabs from vdagent,
> > > > > >     * ignore grabs from other apps running in the host
> > > > >
> > > > > I have a hard time understanding why keyboard focus is related to
> > > > > clipboard. clipboard can be interacted with mouse only, or
> > > > > independently from any user input. Or from multiple inputs. I try to
> > > > > fit the keyboard input this into clipboard interaction issues, and I
> > > > > don't think that helps much.
> > > >
> > > > If spice-gtk has keyboard focus, the user interacts with the guest system.
> > > > If it doesn't, user interacts with the client system.
> > > >
> > > > Sure, the clipboard can change without user's input. E.g. with some
> > > > automation systems, as you pointer out earlier.
> > > > However, this is not the usual use case. And with this patch, I'm
> > > > trying to fix the most common scenario to provide a better default
> > > > experience.
> > >
> > >
> > > How reproducible is the problem you are fixing? Isn't this a corner
> > > case? Can you describe a use case in the patch?
> >
> > That's hard to say. There have been at least 3 related reports so far:
> > https://gitlab.freedesktop.org/spice/win32/vd_agent/issues/6
> > https://gitlab.freedesktop.org/spice/spice-gtk/issues/36
> > https://bugzilla.redhat.com/show_bug.cgi?id=1594876
> >
>
> 3 reports, and no summary of a use-case to reproduce the problem? I
> suppose I will have to read the issues.

The issue is all about the timing, so it can be hard to find a
specific use-case that always works.
>
> > Wayland is mostly fine. X11 environment with a clipboard manager is
> > the one most prone to errors.
>
> That's my setup fwiw, I use "clipboard indicator" with gnome & x11
> (most of the time, sometime I switch to wayland).

I did some more digging and it seems like not all clipboard managers
are affected.
In two of the reports, KDE was used. KDE has a klipper built-in. By
default, this clipboard manager enables a feature called "prevent
empty clipboard". What this does is that if the clipboard gets
released, it immediately grabs it with the most recent clipboard data.

So I tested the following setup:
client: X11, Gnome
guest: X11, KDE with klipper

with each copying in the client, spice-gtk sends 2 messages (for the
selection "clipboard"):
(1) release -- to indicate that the old contents are no longer available
(2) grab -- saying spice-gtk can provide data

Now, due to the previously mentioned function of klipper, (1) will
cause a grab (3) being sent from the vdagent to spice-gtk. So we have
two grabs, (2) and (3), coming in the opposite directions at about the
same time. That's why this use-case is prone to errors.

I was running the guest on the same machine as the client (on my
laptop) which made it difficult to reproduce since the transfer time
between the two systems was too quick. So I tried to "simulate" a slow
connection by adding a 200 ms sleep before the grab (2) is sent form
spice-gtk to vdagent.

You can give it a try ;)
>
> > >
> > > I think we shouldn't couple clipboard handling and keyboard focus.
> > >
> > > Adding layers of weird tweaks such as this one makes code more
> > > complicated and more prone to strange behaviors that are difficult to
> > > debug.
> >
> > I would understand if you nack-ed the 2/3 patch to avoid additional
> > complexity as it really does handle an edge case.
> > But this one is mere 13 additions. Once we have a proper fix, we can
> > just enclose this code with
> >     |    if (spice_main_channel_agent_test_capability())
> >
> > So since this patch is so simple, I don't see a reason to leave the
> > behavior with the old agents broken.
>
> Ok,
> As I said, I find it hard to mix "keyboard focus" with clipboard
> handling. Give me a few more days to think through this a bit :)

Sure thing.
>
> (if I could have a broken case I can study, that would help me - I'll
> eventually dig in the reported issues)
>
> > >
> > > Imho, we need to fix it properly instead.
> >
> > I agree, but the proper fix can follow this one, imho :)
> >
> > Cheers,
> > Jakub
> > >
> > > > >
> > > > > > (2) spice-gtk doesn't have focus ==>
> > > > > >     * accept grabs from other apps running in the host
> > > > > >     * ignore grabs from vdagent
> > > > > >
> > > > > > The check in clipboard_owner_change() is X11-specific,
> > > > > > because on Wayland, spice-gtk is first notified about the
> > > > > > owner-change once it gains focus.
> > > > > >
> > > > > > The check in clipboard_grab() can be generic.
> > > > > > Note that on Wayland, calling gtk_clipboard_set_with_owner()
> > > > > > while not having focus doesn't have any effect anyway,
> > > > > > only focused clients can set clipboard.
> > > > > >
> > > > > > With this patch, the race can still occur around the time
> > > > > > when focus changes (rare, but possible), on X11 as well as Wayland.
> > > > > >
> > > > > > Related: https://gitlab.freedesktop.org/spice/spice-gtk/issues/82
> > > > > > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876
> > > > > >
> > > > > > Signed-off-by: Jakub Janků <jjanku@redhat.com>
> > > > > > ---
> > > > > >
> > > > > > Victor, half of this patch is basically yours,
> > > > > > so feel free to add signed-off or something,
> > > > > > in the case this gets upstream :)
> > > > > >
> > > > > > ---
> > > > > >  src/spice-gtk-session.c | 13 +++++++++++++
> > > > > >  1 file changed, 13 insertions(+)
> > > > > >
> > > > > > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > > > > > index b48f92a..7b7370c 100644
> > > > > > --- a/src/spice-gtk-session.c
> > > > > > +++ b/src/spice-gtk-session.c
> > > > > > @@ -680,6 +680,13 @@ static void clipboard_owner_change(GtkClipboard        *clipboard,
> > > > > >          s->clip_hasdata[selection] = FALSE;
> > > > > >          return;
> > > > > >      }
> > > > > > +
> > > > > > +    if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
> > > > >
> > > > > Note, gdk_display_get_default() is probably wrong.
> > > > >
> > > > > gtk_widget_get_display () is probably better. Not a big deal, needs to
> > > > > be changed over the whole tree I guess.
> > > >
> > > > True.
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > > +        spice_gtk_session_get_keyboard_has_focus(self)) {
> > > > > > +        SPICE_DEBUG("%s: spice-gtk has keyboard focus, "
> > > > > > +                    "ignoring grab from other app", __FUNCTION__);
> > > > > > +        return;
> > > > > > +    }
> > > > > >  #endif
> > > > > >
> > > > > >      s->clip_hasdata[selection] = TRUE;
> > > > > > @@ -823,6 +830,12 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
> > > > > >      cb = get_clipboard_from_selection(s, selection);
> > > > > >      g_return_val_if_fail(cb != NULL, FALSE);
> > > > > >
> > > > > > +    if (!spice_gtk_session_get_keyboard_has_focus(self)) {
> > > > > > +        SPICE_DEBUG("%s: spice-gtk doesn't have keyboard focus, "
> > > > > > +                    "ignoring grab from guest agent", __FUNCTION__);
> > > > > > +        return FALSE;
> > > > > > +    }
> > > > > > +
> > > > > >      for (n = 0; n < ntypes; ++n) {
> > > > > >          found = FALSE;
> > > > > >          for (m = 0; m < SPICE_N_ELEMENTS(atom2agent); m++) {
> > > > > > --
> > > > > > 2.20.1
> > > > > >
> > > > > > _______________________________________________
> > > > > > Spice-devel mailing list
> > > > > > Spice-devel@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Marc-André Lureau
> > > >
> > > > Cheers,
> > > > Jakub
> > >
> > >
> > >
> > > --
> > > Marc-André Lureau
>
>
>
> --
> Marc-André Lureau

Regards,
Jakub
Hi

On Thu, Feb 28, 2019 at 8:12 PM Jakub Janků <jjanku@redhat.com> wrote:
>
> If two grab messages in opposite directions "meet" on their way
> to their destinations, we end up in a state when both spice-gtk
> and spice-vdagent think that the other component can provide
> clipboard data. As a consequence of this, neither of them can.
>
> This is a bug in the spice-protocol. To mitigate the issue,
> accept grab only from the side that currently has keyboard focus,
> this means:
> (1) spice-gtk has focus ==>
>     * accept grabs from vdagent,
>     * ignore grabs from other apps running in the host
> (2) spice-gtk doesn't have focus ==>
>     * accept grabs from other apps running in the host
>     * ignore grabs from vdagent
>
> The check in clipboard_owner_change() is X11-specific,
> because on Wayland, spice-gtk is first notified about the
> owner-change once it gains focus.
>
> The check in clipboard_grab() can be generic.
> Note that on Wayland, calling gtk_clipboard_set_with_owner()
> while not having focus doesn't have any effect anyway,
> only focused clients can set clipboard.
>
> With this patch, the race can still occur around the time
> when focus changes (rare, but possible), on X11 as well as Wayland.
>
> Related: https://gitlab.freedesktop.org/spice/spice-gtk/issues/82
> Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876
>
> Signed-off-by: Jakub Janků <jjanku@redhat.com>
> ---
>
> Victor, half of this patch is basically yours,
> so feel free to add signed-off or something,
> in the case this gets upstream :)
>
> ---
>  src/spice-gtk-session.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index b48f92a..7b7370c 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -680,6 +680,13 @@ static void clipboard_owner_change(GtkClipboard        *clipboard,
>          s->clip_hasdata[selection] = FALSE;
>          return;
>      }
> +
> +    if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
> +        spice_gtk_session_get_keyboard_has_focus(self)) {
> +        SPICE_DEBUG("%s: spice-gtk has keyboard focus, "
> +                    "ignoring grab from other app", __FUNCTION__);
> +        return;
> +    }
>  #endif

This will break clipboard managers interactions, which may take the
clipboard content, save it and/or modify it.

>
>      s->clip_hasdata[selection] = TRUE;
> @@ -823,6 +830,12 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
>      cb = get_clipboard_from_selection(s, selection);
>      g_return_val_if_fail(cb != NULL, FALSE);
>
> +    if (!spice_gtk_session_get_keyboard_has_focus(self)) {
> +        SPICE_DEBUG("%s: spice-gtk doesn't have keyboard focus, "
> +                    "ignoring grab from guest agent", __FUNCTION__);
> +        return FALSE;
> +    }


Beside automation, the cursor alone may easily create a new clipboard
content which won't be available to the client side (the auto-grab may
fail to follow cursor etc).

It's a bit unclear why it's not X11 specific but for client side
change it is, this could deserve a code comment.

All in all, this feels weak and breaks some legitimate cases.

I am not very strongly against this, as I understand it may help with
some races we discussed, but it feels like the problem is elsewhere
and we need a better solution to prevent the race from happening.

I haven't read the bug reports: this kind of workaround needs a
description of a broken use case (not a theoretical description of a
race that "never" happen in practice).


> +
>      for (n = 0; n < ntypes; ++n) {
>          found = FALSE;
>          for (m = 0; m < SPICE_N_ELEMENTS(atom2agent); m++) {
> --
> 2.20.1
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
Hi,

On Thu, Mar 14, 2019 at 6:18 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:
> >
> > If two grab messages in opposite directions "meet" on their way
> > to their destinations, we end up in a state when both spice-gtk
> > and spice-vdagent think that the other component can provide
> > clipboard data. As a consequence of this, neither of them can.
> >
> > This is a bug in the spice-protocol. To mitigate the issue,
> > accept grab only from the side that currently has keyboard focus,
> > this means:
> > (1) spice-gtk has focus ==>
> >     * accept grabs from vdagent,
> >     * ignore grabs from other apps running in the host
> > (2) spice-gtk doesn't have focus ==>
> >     * accept grabs from other apps running in the host
> >     * ignore grabs from vdagent
> >
> > The check in clipboard_owner_change() is X11-specific,
> > because on Wayland, spice-gtk is first notified about the
> > owner-change once it gains focus.
> >
> > The check in clipboard_grab() can be generic.
> > Note that on Wayland, calling gtk_clipboard_set_with_owner()
> > while not having focus doesn't have any effect anyway,
> > only focused clients can set clipboard.
> >
> > With this patch, the race can still occur around the time
> > when focus changes (rare, but possible), on X11 as well as Wayland.
> >
> > Related: https://gitlab.freedesktop.org/spice/spice-gtk/issues/82
> > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876
> >
> > Signed-off-by: Jakub Janků <jjanku@redhat.com>
> > ---
> >
> > Victor, half of this patch is basically yours,
> > so feel free to add signed-off or something,
> > in the case this gets upstream :)
> >
> > ---
> >  src/spice-gtk-session.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > index b48f92a..7b7370c 100644
> > --- a/src/spice-gtk-session.c
> > +++ b/src/spice-gtk-session.c
> > @@ -680,6 +680,13 @@ static void clipboard_owner_change(GtkClipboard        *clipboard,
> >          s->clip_hasdata[selection] = FALSE;
> >          return;
> >      }
> > +
> > +    if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
> > +        spice_gtk_session_get_keyboard_has_focus(self)) {
> > +        SPICE_DEBUG("%s: spice-gtk has keyboard focus, "
> > +                    "ignoring grab from other app", __FUNCTION__);
> > +        return;
> > +    }
> >  #endif
>
> This will break clipboard managers interactions, which may take the
> clipboard content, save it and/or modify it.

Depends on the implementation of the given clipboard manager, I'd say.
I tried the "clipboard indicator" you're using. Seems like no problem there :)
>
> >
> >      s->clip_hasdata[selection] = TRUE;
> > @@ -823,6 +830,12 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
> >      cb = get_clipboard_from_selection(s, selection);
> >      g_return_val_if_fail(cb != NULL, FALSE);
> >
> > +    if (!spice_gtk_session_get_keyboard_has_focus(self)) {
> > +        SPICE_DEBUG("%s: spice-gtk doesn't have keyboard focus, "
> > +                    "ignoring grab from guest agent", __FUNCTION__);
> > +        return FALSE;
> > +    }
>
>
> Beside automation, the cursor alone may easily create a new clipboard
> content which won't be available to the client side (the auto-grab may
> fail to follow cursor etc).
>
> It's a bit unclear why it's not X11 specific but for client side
> change it is, this could deserve a code comment.

Tried to describe that in the commit log. I could add a comment in the
code as well.
>
> All in all, this feels weak and breaks some legitimate cases.
>
> I am not very strongly against this, as I understand it may help with
> some races we discussed,

Is this an ack or nack?
Seems like we're just going round in circles now...

> but it feels like the problem is elsewhere

sorry, but that's very vague, I have no idea what you're referring to

> and we need a better solution to prevent the race from happening.
>
> I haven't read the bug reports: this kind of workaround needs a
> description of a broken use case (not a theoretical description of a
> race that "never" happen in practice).

I described a broken case in the previous email:
KDE with klipper, "prevent empty clipboard" enabled + slow network

Cheers,
Jakub

>
>
> > +
> >      for (n = 0; n < ntypes; ++n) {
> >          found = FALSE;
> >          for (m = 0; m < SPICE_N_ELEMENTS(atom2agent); m++) {
> > --
> > 2.20.1
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
>
> --
> Marc-André Lureau
Hi

On Fri, Mar 15, 2019 at 4:43 PM Jakub Janku <jjanku@redhat.com> wrote:
>
> Hi,
>
> On Thu, Mar 14, 2019 at 6:18 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:
> > >
> > > If two grab messages in opposite directions "meet" on their way
> > > to their destinations, we end up in a state when both spice-gtk
> > > and spice-vdagent think that the other component can provide
> > > clipboard data. As a consequence of this, neither of them can.
> > >
> > > This is a bug in the spice-protocol. To mitigate the issue,
> > > accept grab only from the side that currently has keyboard focus,
> > > this means:
> > > (1) spice-gtk has focus ==>
> > >     * accept grabs from vdagent,
> > >     * ignore grabs from other apps running in the host
> > > (2) spice-gtk doesn't have focus ==>
> > >     * accept grabs from other apps running in the host
> > >     * ignore grabs from vdagent
> > >
> > > The check in clipboard_owner_change() is X11-specific,
> > > because on Wayland, spice-gtk is first notified about the
> > > owner-change once it gains focus.
> > >
> > > The check in clipboard_grab() can be generic.
> > > Note that on Wayland, calling gtk_clipboard_set_with_owner()
> > > while not having focus doesn't have any effect anyway,
> > > only focused clients can set clipboard.
> > >
> > > With this patch, the race can still occur around the time
> > > when focus changes (rare, but possible), on X11 as well as Wayland.
> > >
> > > Related: https://gitlab.freedesktop.org/spice/spice-gtk/issues/82
> > > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876
> > >
> > > Signed-off-by: Jakub Janků <jjanku@redhat.com>
> > > ---
> > >
> > > Victor, half of this patch is basically yours,
> > > so feel free to add signed-off or something,
> > > in the case this gets upstream :)
> > >
> > > ---
> > >  src/spice-gtk-session.c | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > >
> > > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > > index b48f92a..7b7370c 100644
> > > --- a/src/spice-gtk-session.c
> > > +++ b/src/spice-gtk-session.c
> > > @@ -680,6 +680,13 @@ static void clipboard_owner_change(GtkClipboard        *clipboard,
> > >          s->clip_hasdata[selection] = FALSE;
> > >          return;
> > >      }
> > > +
> > > +    if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
> > > +        spice_gtk_session_get_keyboard_has_focus(self)) {
> > > +        SPICE_DEBUG("%s: spice-gtk has keyboard focus, "
> > > +                    "ignoring grab from other app", __FUNCTION__);
> > > +        return;
> > > +    }
> > >  #endif
> >
> > This will break clipboard managers interactions, which may take the
> > clipboard content, save it and/or modify it.
>
> Depends on the implementation of the given clipboard manager, I'd say.
> I tried the "clipboard indicator" you're using. Seems like no problem there :)

without, and with that patch I suppose

> >
> > >
> > >      s->clip_hasdata[selection] = TRUE;
> > > @@ -823,6 +830,12 @@ static gboolean clipboard_grab(SpiceMainChannel *main, guint selection,
> > >      cb = get_clipboard_from_selection(s, selection);
> > >      g_return_val_if_fail(cb != NULL, FALSE);
> > >
> > > +    if (!spice_gtk_session_get_keyboard_has_focus(self)) {
> > > +        SPICE_DEBUG("%s: spice-gtk doesn't have keyboard focus, "
> > > +                    "ignoring grab from guest agent", __FUNCTION__);
> > > +        return FALSE;
> > > +    }
> >
> >
> > Beside automation, the cursor alone may easily create a new clipboard
> > content which won't be available to the client side (the auto-grab may
> > fail to follow cursor etc).
> >
> > It's a bit unclear why it's not X11 specific but for client side
> > change it is, this could deserve a code comment.
>
> Tried to describe that in the commit log. I could add a comment in the
> code as well.

yes, please

> >
> > All in all, this feels weak and breaks some legitimate cases.
> >
> > I am not very strongly against this, as I understand it may help with
> > some races we discussed,
>
> Is this an ack or nack?

If you ask me, it's a nack at this point, as I don't have a way to
reproduce the problem at this point and I clearly see problematic case
created by the patch. Iow, it looks like a regression to me.

> Seems like we're just going round in circles now...

Hopefully I am not the only one reviewing and questioning this patch

> > but it feels like the problem is elsewhere
>
> sorry, but that's very vague, I have no idea what you're referring to

Some clipboard managers seem to work fine. So what is klipper doing?

>
> > and we need a better solution to prevent the race from happening.
> >
> > I haven't read the bug reports: this kind of workaround needs a
> > description of a broken use case (not a theoretical description of a
> > race that "never" happen in practice).
>
> I described a broken case in the previous email:
> KDE with klipper, "prevent empty clipboard" enabled + slow network

I guess I will have to install KDE to understand and reproduce the issue.

Rgardless if this patch is accepted or not, we should work on a real
solution to prevent the race.