Re: [Spice-devel] [PATCH spice-gtk 1/3] clipboard: accept grab only from the side with keyboard focus
Hey,
On Fri, Mar 15, 2019 at 8:02 PM Marc-André Lureau
wrote:
>
> Hi
>
> On Fri, Mar 15, 2019 at 4:43 PM Jakub Janku wrote:
> >
> > Hi,
> >
> > On Thu, Mar 14, 2019 at 6:18 PM Marc-André Lureau
> > wrote:
> > >
> > > Hi
> > >
> > > On Thu, Feb 28, 2019 at 8:12 PM Jakub Janků 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ů
> > > > ---
> > > >
> > > > 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
Sure
>
> > >
> > > >
> > > > 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?
I've already tried to describe that in [0].
[0]
Re: [Spice-devel] [PATCH spice-gtk 1/3] clipboard: accept grab only from the side with keyboard focus
Hi
On Fri, Mar 15, 2019 at 4:43 PM Jakub Janku wrote:
>
> Hi,
>
> On Thu, Mar 14, 2019 at 6:18 PM Marc-André Lureau
> wrote:
> >
> > Hi
> >
> > On Thu, Feb 28, 2019 at 8:12 PM Jakub Janků 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ů
> > > ---
> > >
> > > 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
Re: [Spice-devel] [PATCH spice-gtk 1/3] clipboard: accept grab only from the side with keyboard focus
Hi,
On Thu, Mar 14, 2019 at 6:18 PM Marc-André Lureau
wrote:
>
> Hi
>
> On Thu, Feb 28, 2019 at 8:12 PM Jakub Janků 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ů
> > ---
> >
> > 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
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk 1/3] clipboard: accept grab only from the side with keyboard focus
Hi
On Thu, Feb 28, 2019 at 8:12 PM Jakub Janků 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ů
> ---
>
> 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
--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk 1/3] clipboard: accept grab only from the side with keyboard focus
Hi again, On Fri, Mar 8, 2019 at 4:00 PM Marc-André Lureau wrote: > > Hi > > On Fri, Mar 8, 2019 at 1:41 PM Jakub Janku wrote: > > > > Hi, > > > > On Fri, Mar 8, 2019 at 1:15 PM Marc-André Lureau > > wrote: > > > > > > Hi > > > > > > On Thu, Mar 7, 2019 at 10:36 PM Jakub Janku wrote: > > > > > > > > Hi, > > > > > > > > thanks for having a look! > > > > > > > > On Wed, Mar 6, 2019 at 6:42 PM Marc-André Lureau > > > > wrote: > > > > > > > > > > Hi > > > > > > > > > > On Thu, Feb 28, 2019 at 8:12 PM Jakub Janků 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 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
Re: [Spice-devel] [PATCH spice-gtk 1/3] clipboard: accept grab only from the side with keyboard focus
Hi On Fri, Mar 8, 2019 at 1:41 PM Jakub Janku wrote: > > Hi, > > On Fri, Mar 8, 2019 at 1:15 PM Marc-André Lureau > wrote: > > > > Hi > > > > On Thu, Mar 7, 2019 at 10:36 PM Jakub Janku wrote: > > > > > > Hi, > > > > > > thanks for having a look! > > > > > > On Wed, Mar 6, 2019 at 6:42 PM Marc-André Lureau > > > wrote: > > > > > > > > Hi > > > > > > > > On Thu, Feb 28, 2019 at 8:12 PM Jakub Janků 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 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,
Re: [Spice-devel] [PATCH spice-gtk 1/3] clipboard: accept grab only from the side with keyboard focus
Hi, On Fri, Mar 8, 2019 at 1:15 PM Marc-André Lureau wrote: > > Hi > > On Thu, Mar 7, 2019 at 10:36 PM Jakub Janku wrote: > > > > Hi, > > > > thanks for having a look! > > > > On Wed, Mar 6, 2019 at 6:42 PM Marc-André Lureau > > wrote: > > > > > > Hi > > > > > > On Thu, Feb 28, 2019 at 8:12 PM Jakub Janků 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 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
Re: [Spice-devel] [PATCH spice-gtk 1/3] clipboard: accept grab only from the side with keyboard focus
Hi On Thu, Mar 7, 2019 at 10:36 PM Jakub Janku wrote: > > Hi, > > thanks for having a look! > > On Wed, Mar 6, 2019 at 6:42 PM Marc-André Lureau > wrote: > > > > Hi > > > > On Thu, Feb 28, 2019 at 8:12 PM Jakub Janků 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 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
Re: [Spice-devel] [PATCH spice-gtk 1/3] clipboard: accept grab only from the side with keyboard focus
Hi, thanks for having a look! On Wed, Mar 6, 2019 at 6:42 PM Marc-André Lureau wrote: > > Hi > > On Thu, Feb 28, 2019 at 8:12 PM Jakub Janků 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 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
Re: [Spice-devel] [PATCH spice-gtk 1/3] clipboard: accept grab only from the side with keyboard focus
Hi
On Thu, Feb 28, 2019 at 8:12 PM Jakub Janků 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 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ů
> ---
>
> 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
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-gtk 1/3] clipboard: accept grab only from the side with keyboard focus
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ů
---
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
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
