[Spice-devel] [PATCH spice-gtk 1/3] clipboard: accept grab only from the side with keyboard focus
Jakub Janku
jjanku at redhat.com
Sat Mar 9 12:18:35 UTC 2019
Hi again,
On Fri, Mar 8, 2019 at 4:00 PM Marc-André Lureau
<marcandre.lureau at gmail.com> wrote:
>
> Hi
>
> On Fri, Mar 8, 2019 at 1:41 PM Jakub Janku <jjanku at redhat.com> wrote:
> >
> > 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
> >
>
> 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 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
>
>
>
> --
> Marc-André Lureau
Regards,
Jakub
More information about the Spice-devel
mailing list