[Spice-devel] [PATCH spice-gtk 1/3] clipboard: accept grab only from the side with keyboard focus

[Marc-André Lureau]

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.

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:

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. But how to distinguish the conflict case with
the normal case? Perhaps an incremental counter (age/generation,
whatever you call it) would be enough.

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.

> (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.





> +        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