[Spice-devel] [spice-gtk v4 2/2] gtk-session: clipboard: x11: owner-change improvement

[Victor Toso]

Hi,

On Mon, Feb 11, 2019 at 10:12:42AM +0100, Jakub Janku wrote:
> ping?

Should be fixed by:

commit 214f8fd969127c41a7d53def196118ee0549a411
Author: Jakub Janků <jjankuatredhat.com>
Date:   Sun Jan 27 18:14:20 2019 +0100

    clipboard: don't request targets without owner on X11

    On X11, if the owner in GdkEventOwnerChange is set to NULL,
    it means there's no data in the clipboard, so it's pointless to
    request targets as the request will fail anyway.

    On Wayland, owner is always NULL, so don't do anything there.

    Signed-off-by: Jakub Janků <jjanku at redhat.com>
    Acked-by: Victor Toso <victortoso at redhat.com>

The fact that the patch in this mail thread is related to keyboard-grab
was the reason to be nacked. If you want to discuss that, we might move
along to that thread on clibpoard-managers, sent as RFC

    https://lists.freedesktop.org/archives/spice-devel/2019-January/047259.html

> On Mon, Jan 28, 2019 at 10:29 AM Jakub Janku <jjanku at redhat.com> wrote:
> >
> > Hi,
> >
> > I tried to fix this bug in a less radical way, but my patch unfortunately did not cover all the cases.
> >
> > I obtained some logs from James Harvey which make the situation clearer - it can be found here:
> > https://termbin.com/40un
> > So I'll try to explain what's happening:
> >
> > James uses KDE which has a clipboard manager integrated in (klipper).
> >
> > (1) user copies something in the guest, grab is sent to the
> > spice-gtk
> > (2) clipboard manager immediately requests the data, data is
> > retrieved from the vdagent
> > (3) user pastes the copied data in guest, this causes a quick
> > re-grab in the guest = a clipboard release message is sent to
> > spice-gtk and it is immediately followed by a new grab
> > (4) spice-gtk receives clipboard release, so it clears the clipboard
> > (5) clipboard manager detects that the clipboard has no owner, so it
> > grabs it itself, advertising the data it originally obtained from us
> > in step (2) (this normally enables the user to paste the data even
> > after the source app has been closed)
> > (6) spice-gtk receives "owner-change" signal caused by the grab in
> > step (5), requests clipboard targets and sends a grab to vdagent
> > (7) spice-gtk finally receives the grab from step (3), so it sets
> > the clipboard using gtk_clipboard_set_with_owner()
> > (8) vdagent receives grab from step (6), so it too sets the
> > clipboard using gtk_clipboard_set_with_owner()
> >
> > This is clearly a race. We ended up in a state where both spice-gtk
> > and vdagent thinks the other component can provide the data, but in
> > reality none of them can.
> >
> > (This does not happen always, as can be seen in the log, the first
> > paste succeeds.)
> >
> > Given current spice protocol, I don't see a way to detect the
> > race on either side, but I'd love to be wrong. So I'd update
> > the commit log and comment in the code to explain the
> > situation and then it's ack from me.

I think we can still improve documentation to clarify the race and
behaviors. There are a few things that I thought were obvious and in
fact Marc-André pointed out I was wrong.

Either way, let me know if I'm missing something.

Cheers!

> > Cheers,
> > Jakub
> >
> > On Mon, Jan 14, 2019, 1:34 PM Victor Toso <victortoso at redhat.com wrote:
> >>
> >> From: Victor Toso <me at victortoso.com>
> >>
> >> On X11, the release-grab message might end up clearing the
> >> GtkClipboard which triggers the owner-changed callback having the
> >> event owner as NULL.
> >>
> >> We should not be calling gtk_clipboard_request_targets() in this
> >> situation as is prone to errors as the intention here is request
> >> clipboard information from changes made by client OS.
> >>
> >> The fix is to avoid any such request while spice client is holding the
> >> keyboard grab.
> >>
> >> Related: https://gitlab.freedesktop.org/spice/win32/vd_agent/issues/6
> >> Related: https://gitlab.freedesktop.org/spice/linux/vd_agent/issues/9
> >> Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876
> >>
> >> Changed in v4:
> >> - Updated commit log
> >>
> >> Signed-off-by: Victor Toso <victortoso at redhat.com>
> >> Tested-by: James Harvey @jamespharvey20
> >> ---
> >>  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 abce43f..20df70d 100644
> >> --- a/src/spice-gtk-session.c
> >> +++ b/src/spice-gtk-session.c
> >> @@ -674,6 +674,19 @@ static void clipboard_owner_change(GtkClipboard        *clipboard,
> >>          return;
> >>      }
> >>
> >> +#ifdef GDK_WINDOWING_X11
> >> +    /* In X11, while holding the keyboard-grab we are not interested in this
> >> +     * event as it either came by grab or release messages from agent.  */
> >> +    if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
> >> +        spice_gtk_session_get_keyboard_has_focus(self)) {
> >> +        SPICE_DEBUG("clipboard: owner-changed event: not requesting client's target "
> >> +                    "while holding focus");
> >> +        return;
> >> +    }
> >> +#endif
> >> +    SPICE_DEBUG("clipboard: owner-changed event: has-focus=%d",
> >> +                spice_gtk_session_get_keyboard_has_focus(self));
> >> +
> >>      s->clipboard_by_guest[selection] = FALSE;
> >>      s->clip_hasdata[selection] = TRUE;
> >>      if (s->auto_clipboard_enable && !read_only(self))
> >> --
> >> 2.20.1
> >>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190211/95dc2bdc/attachment.sig>