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

[Jakub Janku]

ping?

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