[Spice-devel] [PATCH spice-gtk 1/3] clipboard: accept grab only from the side with keyboard focus
Jakub Janku
jjanku at redhat.com
Fri Mar 8 12:41:37 UTC 2019
Hi,
On Fri, Mar 8, 2019 at 1:15 PM Marc-André Lureau
<marcandre.lureau at gmail.com> wrote:
>
> Hi
>
> On Thu, Mar 7, 2019 at 10:36 PM Jakub Janku <jjanku at redhat.com> wrote:
> >
> > Hi,
> >
> > thanks for having a look!
> >
> > On Wed, Mar 6, 2019 at 6:42 PM Marc-André Lureau
> > <marcandre.lureau at gmail.com> wrote:
> > >
> > > Hi
> > >
> > > On Thu, Feb 28, 2019 at 8:12 PM Jakub Janků <jjanku at 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 at 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 at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > >
> > >
> > >
> > > --
> > > Marc-André Lureau
> >
> > Cheers,
> > Jakub
>
>
>
> --
> Marc-André Lureau
More information about the Spice-devel
mailing list