[spice-gtk,v2] gtk-session: clipboard request: notify agent on failure

Submitted by Victor Toso on Jan. 16, 2019, 9:10 a.m.

Details

Message ID 20190116091035.26205-1-victortoso@redhat.com
State New
Headers show
Series "Series without cover letter" ( rev: 2 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso Jan. 16, 2019, 9:10 a.m.
From: Victor Toso <me@victortoso.com>

Similar to 172c521271a3d - if we don't, the agent might be waiting for
data till some timeout happens in the system, blocking copy-paste
feature and possibly freezing applications.

A way to reproduce is copy-paste big clipboard data between two spice
clients.

Also add some documentation to the checks, in order to quickly
understand what they are about.

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

v1 -> v2:
- Fixed comment on s->clipboard_by_guest[selection] check (Marc-André)
- Removed the reply on read only. (Marc-André)

Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 src/spice-gtk-session.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index 1a19bca..82a5b35 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -1015,12 +1015,29 @@  static gboolean clipboard_request(SpiceMainChannel *main, guint selection,
     int m;
 
     cb = get_clipboard_from_selection(s, selection);
-    g_return_val_if_fail(cb != NULL, FALSE);
-    g_return_val_if_fail(s->clipboard_by_guest[selection] == FALSE, FALSE);
-    g_return_val_if_fail(s->clip_grabbed[selection], FALSE);
+    if (cb == NULL) {
+        goto notify_agent;
+    }
+
+    /* As we are holding clipboard data sent by selection-grab from guest, the
+     * selection-request of clipboard data from guest must fail. We either sent
+     * a bad selection-grab to the guest or the agent is buggy. */
+    if (s->clipboard_by_guest[selection]) {
+        SPICE_DEBUG("clipboard: agent request: grab on hold by agent, possible race");
+        goto notify_agent;
+    }
 
-    if (read_only(self))
+    /* The selection-request by agent should happen only if the clipboard data is set
+     * by client */
+    if (!s->clip_grabbed[selection]) {
+        SPICE_DEBUG("clipboard: agent request: data set by agent, possible race");
+        goto notify_agent;
+    }
+
+    /* On read only, should not reply to the agent. */
+    if (read_only(self)) {
         return FALSE;
+    }
 
     if (type == VD_AGENT_CLIPBOARD_UTF8_TEXT) {
         gtk_clipboard_request_text(cb, clipboard_received_text_cb,
@@ -1039,6 +1056,10 @@  static gboolean clipboard_request(SpiceMainChannel *main, guint selection,
     }
 
     return TRUE;
+
+notify_agent:
+    spice_main_channel_clipboard_selection_notify(s->main, selection, type, NULL, 0);
+    return FALSE;
 }
 
 static void clipboard_release(SpiceMainChannel *main, guint selection,

Comments

On Wed, Jan 16, 2019 at 4:10 AM Victor Toso <victortoso@redhat.com> wrote:
>
> From: Victor Toso <me@victortoso.com>
>
> Similar to 172c521271a3d - if we don't, the agent might be waiting for
> data till some timeout happens in the system, blocking copy-paste
> feature and possibly freezing applications.
>
> A way to reproduce is copy-paste big clipboard data between two spice
> clients.
>
> Also add some documentation to the checks, in order to quickly
> understand what they are about.
>
> 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
>
> v1 -> v2:
> - Fixed comment on s->clipboard_by_guest[selection] check (Marc-André)
> - Removed the reply on read only. (Marc-André)
>
> Signed-off-by: Victor Toso <victortoso@redhat.com>

Am I understanding correctly that the Jan 10 patches which completely
fix spice for me and others experiencing clipboard problems aren't
being added in any form, and that instead the clipboard is being left
broken, just less broken so it doesn't freeze the guest?  I don't know
exactly when it broke, but spice didn't used to act this way.

This patch (v2) alone, on top of current git (ecf9358) at least fixes
the freezing.  But, if that's all that's added, it's not something I
can stand using.  It doesn't help the clipboard problems.  It makes me
want to break my keyboard.  Paste randomly failing this often is
extremely frustrating and distracting.

Log of it breaking after a single paste, so the second one fails is
here: https://termbin.com/5d69

The last spice-git I ran was f18589b with the Jan 10 patches:
* [spice-gtk v3 1/3] gtk-session: clipboard: document owner-changed event
* [spice-gtk v3 2/3] gtk-session: clipboard: x11: owner-change improvement
* [spice-gtk v3 3/3] gtk-session: clipboard: x11: do not request data
while on focus
* [Spice-devel] [spice-gtk] gtk-session: prefer early check to agent
connectivity

That was rock solid.  I have no clipboard problems.

That's going to leave me and others perpetually patching spice
releases, and someday when they don't apply cleanly, being at a loss
and being forced to look at other protocols.

Apologies if I'm misunderstanding.
Hi,

On Wed, Jan 16, 2019 at 05:46:58AM -0500, james harvey wrote:
> On Wed, Jan 16, 2019 at 4:10 AM Victor Toso <victortoso@redhat.com> wrote:
> >
> > From: Victor Toso <me@victortoso.com>
> >
> > Similar to 172c521271a3d - if we don't, the agent might be waiting for
> > data till some timeout happens in the system, blocking copy-paste
> > feature and possibly freezing applications.
> >
> > A way to reproduce is copy-paste big clipboard data between two spice
> > clients.
> >
> > Also add some documentation to the checks, in order to quickly
> > understand what they are about.
> >
> > 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
> >
> > v1 -> v2:
> > - Fixed comment on s->clipboard_by_guest[selection] check (Marc-André)
> > - Removed the reply on read only. (Marc-André)
> >
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> 
> Am I understanding correctly that the Jan 10 patches which completely
> fix spice for me and others experiencing clipboard problems aren't
> being added in any form, and that instead the clipboard is being left
> broken, just less broken so it doesn't freeze the guest?  I don't know
> exactly when it broke, but spice didn't used to act this way.

No, it'll probably be kept broken broken, not just broken, it
seems.

> This patch (v2) alone, on top of current git (ecf9358) at least fixes
> the freezing.  But, if that's all that's added, it's not something I
> can stand using.  It doesn't help the clipboard problems.  It makes me
> want to break my keyboard.  Paste randomly failing this often is
> extremely frustrating and distracting.
> 
> Log of it breaking after a single paste, so the second one fails is
> here: https://termbin.com/5d69
> 
> The last spice-git I ran was f18589b with the Jan 10 patches:
> * [spice-gtk v3 1/3] gtk-session: clipboard: document owner-changed event
> * [spice-gtk v3 2/3] gtk-session: clipboard: x11: owner-change improvement
> * [spice-gtk v3 3/3] gtk-session: clipboard: x11: do not request data
> while on focus
> * [Spice-devel] [spice-gtk] gtk-session: prefer early check to agent
> connectivity
> 
> That was rock solid.  I have no clipboard problems.
> 
> That's going to leave me and others perpetually patching spice
> releases, and someday when they don't apply cleanly, being at a
> loss and being forced to look at other protocols.
> 
> Apologies if I'm misunderstanding.
Hi

On Wed, Jan 16, 2019 at 2:47 PM james harvey <jamespharvey20@gmail.com> wrote:
>
> On Wed, Jan 16, 2019 at 4:10 AM Victor Toso <victortoso@redhat.com> wrote:
> >
> > From: Victor Toso <me@victortoso.com>
> >
> > Similar to 172c521271a3d - if we don't, the agent might be waiting for
> > data till some timeout happens in the system, blocking copy-paste
> > feature and possibly freezing applications.
> >
> > A way to reproduce is copy-paste big clipboard data between two spice
> > clients.
> >
> > Also add some documentation to the checks, in order to quickly
> > understand what they are about.
> >
> > 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
> >
> > v1 -> v2:
> > - Fixed comment on s->clipboard_by_guest[selection] check (Marc-André)
> > - Removed the reply on read only. (Marc-André)
> >
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
>
> Am I understanding correctly that the Jan 10 patches which completely
> fix spice for me and others experiencing clipboard problems aren't
> being added in any form, and that instead the clipboard is being left
> broken, just less broken so it doesn't freeze the guest?  I don't know
> exactly when it broke, but spice didn't used to act this way.

That's important information, because spice-gtk clipboard code didn't
change that much afaik.

>
> This patch (v2) alone, on top of current git (ecf9358) at least fixes
> the freezing.  But, if that's all that's added, it's not something I
> can stand using.  It doesn't help the clipboard problems.  It makes me
> want to break my keyboard.  Paste randomly failing this often is
> extremely frustrating and distracting.
>
> Log of it breaking after a single paste, so the second one fails is
> here: https://termbin.com/5d69
>
> The last spice-git I ran was f18589b with the Jan 10 patches:
> * [spice-gtk v3 1/3] gtk-session: clipboard: document owner-changed event
> * [spice-gtk v3 2/3] gtk-session: clipboard: x11: owner-change improvement
> * [spice-gtk v3 3/3] gtk-session: clipboard: x11: do not request data
> while on focus
> * [Spice-devel] [spice-gtk] gtk-session: prefer early check to agent
> connectivity
>
> That was rock solid.  I have no clipboard problems.

Cool! now we need to have clear understanding for the changes to
accept them upstream, that's what we are working on.

>
> That's going to leave me and others perpetually patching spice
> releases, and someday when they don't apply cleanly, being at a loss
> and being forced to look at other protocols.
>
> Apologies if I'm misunderstanding.

You misunderstand, there is a lot of activity on this problem atm and
it is currently scheduled for 0.36. However, since it is not clear
whether this is a regression and the fixes are *not* trivial, it
should be fine to schedule for 0.37. Either early release after that,
and fixes can be backported too most probably.
On Wed, Jan 16, 2019 at 6:08 AM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
> On Wed, Jan 16, 2019 at 2:47 PM james harvey <jamespharvey20@gmail.com> wrote:
> > Am I understanding correctly that the Jan 10 patches which completely
> > fix spice for me and others experiencing clipboard problems aren't
> > being added in any form, and that instead the clipboard is being left
> > broken, just less broken so it doesn't freeze the guest?  I don't know
> > exactly when it broke, but spice didn't used to act this way.
>
> That's important information, because spice-gtk clipboard code didn't
> change that much afaik.

I can definitely understand when it broke would be helpful.
Unfortunately, there is a couple of years gap in my spice usage, so I
don't think it's something I can pin down.  I don't know if it would
be possible to run years old spice, or if dependencies from or to
spice would get tripped up and spin the project into basically
bisecting an entire system as a whole, based on packages available on
given dates.  Even if it was, it's of course possible it might be
another package's change that made this stop working.  While that
would be possible to do given Arch's historical repos, it would be a
massive undertaking.  With the amount of other project bisecting I've
done lately, it's not something I'd be able to do anytime soon.

> > That's going to leave me and others perpetually patching spice
> > releases, and someday when they don't apply cleanly, being at a loss
> > and being forced to look at other protocols.
> >
> > Apologies if I'm misunderstanding.
>
> You misunderstand, there is a lot of activity on this problem atm and
> it is currently scheduled for 0.36. However, since it is not clear
> whether this is a regression and the fixes are *not* trivial, it
> should be fine to schedule for 0.37. Either early release after that,
> and fixes can be backported too most probably.

Glad to hear!
Hi,

On Wed, Jan 16, 2019 at 10:11 AM Victor Toso <victortoso@redhat.com> wrote:
>
> From: Victor Toso <me@victortoso.com>
>
> Similar to 172c521271a3d - if we don't, the agent might be waiting for
> data till some timeout happens in the system, blocking copy-paste
> feature and possibly freezing applications.
>
> A way to reproduce is copy-paste big clipboard data between two spice
> clients.
>
> Also add some documentation to the checks, in order to quickly
> understand what they are about.
>
> 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
>
> v1 -> v2:
> - Fixed comment on s->clipboard_by_guest[selection] check (Marc-André)
> - Removed the reply on read only. (Marc-André)
>
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  src/spice-gtk-session.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index 1a19bca..82a5b35 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -1015,12 +1015,29 @@ static gboolean clipboard_request(SpiceMainChannel *main, guint selection,
>      int m;
>
>      cb = get_clipboard_from_selection(s, selection);
> -    g_return_val_if_fail(cb != NULL, FALSE);
> -    g_return_val_if_fail(s->clipboard_by_guest[selection] == FALSE, FALSE);
> -    g_return_val_if_fail(s->clip_grabbed[selection], FALSE);
> +    if (cb == NULL) {
> +        goto notify_agent;
> +    }

cb == NULL means that the requested selection can't have been
advertised by spice-gtk, so the vdagent shouldn't request the
selection's data.
This seems like a clear bug in vdagent to me. Should we respond? I'm
afraid it might potentially result into some bugs staying unnoticed in
the vdagent.

> +
> +    /* As we are holding clipboard data sent by selection-grab from guest, the
> +     * selection-request of clipboard data from guest must fail. We either sent
> +     * a bad selection-grab to the guest or the agent is buggy. */
> +    if (s->clipboard_by_guest[selection]) {
> +        SPICE_DEBUG("clipboard: agent request: grab on hold by agent, possible race");
> +        goto notify_agent;
> +    }

Yes, this can happen, this is the bug we're trying to fix. But this
change doesn't really fix it, it just mitigates it.
As you're saying in the comment, it's a bug either in the client or in
the agent. I'd rather fix the bug than try to alleviate its
"symptoms".

>
> -    if (read_only(self))
> +    /* The selection-request by agent should happen only if the clipboard data is set
> +     * by client */
> +    if (!s->clip_grabbed[selection]) {
> +        SPICE_DEBUG("clipboard: agent request: data set by agent, possible race");
> +        goto notify_agent;
> +    }

This can surely happen and it wouldn't be a programmer error, so the
g_return_val_if_fail() should indeed be replaced by a less severe log.

But again, I have some doubts whether we should respond. The new
clipboard code in linux vdagent clears all pending requests upon
release message. So sending the data message would actually produce a
warning on the guest side as the corresponding request wouldn't be
found. I'm not sure how the Windows agent handles this.

> +
> +    /* On read only, should not reply to the agent. */
> +    if (read_only(self)) {
>          return FALSE;
> +    }
>
>      if (type == VD_AGENT_CLIPBOARD_UTF8_TEXT) {
>          gtk_clipboard_request_text(cb, clipboard_received_text_cb,
> @@ -1039,6 +1056,10 @@ static gboolean clipboard_request(SpiceMainChannel *main, guint selection,
>      }
>
>      return TRUE;
> +
> +notify_agent:
> +    spice_main_channel_clipboard_selection_notify(s->main, selection, type, NULL, 0);
> +    return FALSE;
>  }
>
>  static void clipboard_release(SpiceMainChannel *main, guint selection,
> --
> 2.20.1
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

Cheers,
Jakub