Hi On Fri, Mar 8, 2019 at 1:41 PM Jakub Janku <jjanku@xxxxxxxxxx> wrote: > > Hi, > > On Fri, Mar 8, 2019 at 1:15 PM Marc-André Lureau > <marcandre.lureau@xxxxxxxxx> wrote: > > > > Hi > > > > On Thu, Mar 7, 2019 at 10:36 PM Jakub Janku <jjanku@xxxxxxxxxx> wrote: > > > > > > Hi, > > > > > > thanks for having a look! > > > > > > On Wed, Mar 6, 2019 at 6:42 PM Marc-André Lureau > > > <marcandre.lureau@xxxxxxxxx> wrote: > > > > > > > > Hi > > > > > > > > On Thu, Feb 28, 2019 at 8:12 PM Jakub Janků <jjanku@xxxxxxxxxx> 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@xxxxxxxxxx> > > > > > --- > > > > > > > > > > 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@xxxxxxxxxxxxxxxxxxxxx > > > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel > > > > > > > > > > > > > > > > -- > > > > Marc-André Lureau > > > > > > Cheers, > > > Jakub > > > > > > > > -- > > Marc-André Lureau -- Marc-André Lureau _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel