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

[Marc-André Lureau]

Hi

On Tue, Jan 15, 2019 at 12:40 AM Jakub Janku <jjanku at redhat.com> wrote:
>
> Hi,
>
> On Mon, Jan 14, 2019 at 1:41 PM Marc-André Lureau
> <marcandre.lureau at gmail.com> wrote:
> >
> > Hi
> >
> > On Mon, Jan 14, 2019 at 4: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.
> >
> > Two things that bug me about this:
> > - it's x11 specific, weird
>
> What's so weird about that?
> It's also X11-specific that we can receive these "owner-change" events
> while spice-gtk is holding the focus since on Wayland, apps need to
> gain focus first to be able to set the clipboard, afaik.
> The clipboard protocols on X11 and Wayland have simply been designed
> in different ways. So although GTK tries to unify the behavior and
> provide the same experience in both environments, some limitations
> just cannot be overcome.

The patch is not about integration with X11, the way for example we
have overlay, key locks, or mouse acceleration code that gtk doesn't
provide. It's about an API that gtk+ provides that should be uniform.
Sometime it fails to provide the require abstraction. In this case, I
don't undestand *yet* why the code should be specific to X11, it could
be equally "valid" on wayland or windows etc...

>
> > - the condition seems wrong: if an application has the keyboard grab,
> > that doesn't mean that another cannot update the clipboard. For
> > example, I suppose this can easily happen with multiple
> > pointers/inputs or some automation.
>
> Sure, another app can update the clipboard, but imagine the following situation:
> 1) user copies something in the guest
> 2) an app in the client's system grabs the clipboard without having
> keyboard focus -- this means that the grab likely wasn't directly
> initiated by the user

I don't know, you could have some automation framework changing the
clipboard. For testing, for example, or to remove swear words, or
other kind of protected content..

And I think you could have multiple keyboard/pointers on the same
seat, although this may be esoteric, I agree.

> 3) spice-gtk receives "owner-change" event and subsequently sends grab
> to the agent
> 4) based on the grab message from spice-gtk in step 3), vdagent grabs
> the clipboard in the guest system -- and by that, it overwrites the
> clipboard contents copied in step 1)

That would be what I expect indeed.

>
> So the clipboard is suddenly changed, although the user didn't take any action.

And this may also be what the user wanted.

> I think this is unwanted and this patch solves it, so I'm all for
> ack-ing this just for this reason. If this also solves the bug, it's
> even better.
>
> Nevertheless, I would be really happy if we could track down what
> exactly causes the issues that have been reported and describe it in
> more detail.

Yes, that sounds like a better course to me.

> >
> > Other than that, if it fixes or workaround real clipboard issues, I am
> > not against it, but I think we should add more comments about the
> > "hack".
> >
> >
> > >
> > > 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
>
> Does this mean it fixes the issues for him?
> > > ---
> > >  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.  */
>
> For completeness, it might be good to mention that the event can be
> caused by other applications too (as Marc-André pointed out), even
> though spice-gtk is holding the keyboard-grab.
>
> > > +    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
> > >
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
> >
> Cheers,
> Jakub



-- 
Marc-André Lureau