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

Mon Feb 11 10:19:02 UTC 2019

On Mon, Feb 11, 2019 at 10:30 AM Victor Toso <victortoso at redhat.com> wrote:
>
> Hi,
>
> On Mon, Feb 11, 2019 at 10:12:42AM +0100, Jakub Janku wrote:
> > ping?
>
> Should be fixed by:

You can double-check with James to be sure, but I don't think that's true.

If you look at the logs ( https://termbin.com/40un ), there's the
following line:
    |   (virt-viewer:25767): GSpice-DEBUG: 21:29:06.743:
spice-gtk-session.c:556 Retrieving the clipboard data has failed
And the commit you mention below fixes exactly that and that's all,
the race is still there.

Cheers,
Jakub
>
> 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
> > >>