[Spice-devel] [PATCH spice-gtk 1/3] clipboard: accept grab only from the side with keyboard focus
Marc-André Lureau
marcandre.lureau at gmail.com
Fri Mar 8 14:59:51 UTC 2019
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.
> 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 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
More information about the Spice-devel
mailing list