[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 12:15:28 UTC 2019
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?
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 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