Re: [Spice-devel] [PATCH spice-gtk 1/2] clipboard: do not release between client grabs
Hi,
On Sun, Mar 24, 2019 at 7:26 PM Marc-André Lureau
wrote:
>
> Hi
>
> On Sun, Mar 24, 2019 at 6:50 PM Jakub Janku wrote:
> >
> > Hi,
> >
> > On Thu, Mar 21, 2019 at 1:21 PM wrote:
> > >
> > > From: Marc-André Lureau
> > >
> > > On the client side, whenever the grab owner changes (and the clipboard
> > > was previously grabbed), spice-gtk sends a clipboard release followed
> > > immediately by a new grab. But some clipboard managers on the remote
> > > side react to clipboard release events by taking a clipboard grab,
> > > presumably to avoid empty clipboards.
> > >
> > > The two grabs, coming from the client and from the remote sides, will
> > > race in both directions, which may confuse the client & remote side,
> > > as both believe the other side is the current grab owner, and thus
> > > further clipboard data requests are likely to fail.
> > >
> > > Let's avoid sending a release event when re-grabing.
> > >
> > > The race described above may still happen in other rare circunstances,
> > > and will require a protocol change. To avoid the conflict, a discussed
> > > solution could use a clipboard serial number.
> > >
> > > Tested with current linux & windows vdagent. Looking at earlier
> > > version of the code, it doesn't seem like subsequent grabs will be
> > > treated as an error.
> > >
> > > Signed-off-by: Marc-André Lureau
> > > ---
> > > src/spice-gtk-session.c | 21 -
> > > 1 file changed, 8 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > > index b48f92a..0e748b6 100644
> > > --- a/src/spice-gtk-session.c
> > > +++ b/src/spice-gtk-session.c
> > > @@ -569,8 +569,7 @@ static void clipboard_get_targets(GtkClipboard
> > > *clipboard,
> > > g_return_if_fail(selection != -1);
> > >
> > > if (s->clip_grabbed[selection]) {
> > > -SPICE_DEBUG("Clipboard is already grabbed, ignoring %d atoms",
> > > n_atoms);
> > > -return;
> > > +SPICE_DEBUG("Clipboard is already grabbed, re-grab: %d atoms",
> > > n_atoms);
> > > }
> > >
> > > /* Set all Atoms that matches our current protocol implementation */
> > > @@ -652,18 +651,14 @@ static void clipboard_owner_change(GtkClipboard
> > >*clipboard,
> > > return;
> > > }
> > >
> > > -/* In case we sent a grab to the agent, we need to release it now as
> > > - * previous clipboard data should not be reachable anymore */
> > > -if (s->clip_grabbed[selection]) {
> > > -s->clip_grabbed[selection] = FALSE;
> > > -if (spice_main_channel_agent_test_capability(s->main,
> > > VD_AGENT_CAP_CLIPBOARD_BY_DEMAND)) {
> > > -spice_main_channel_clipboard_selection_release(s->main,
> > > selection);
> > > -}
> > > -}
> >
> > If event->owner is NULL, the clipboard is empty at the moment and this
> > function exits without requesting the new targets. So in this case, we
> > should still send release to the agent, otherwise the guest might
> > think that clipboard data can be provided while the clipboard in the
> > client is empty for a long time. Pasting data in the guest would
> > result into an invalid request being sent to spice-gtk.
>
> This is going into undocumented territories. But if what you say is
> true in commit 9af2c481b74077ab7d6cb9d4bf589f9855a302f5, then yes,
> client should probably release the clipboard.
>
> I don't quite understand why gtk+ would allow NEW_OWNER, with owner ==
> NULL and empty clipboard. It sounds more like a bug to me.
Well, in vdagent we release the clipboard with the following call:
XSetSelectionOwner(x11->display, clip, None, CurrentTime);
So it seems completely fine to me that we get a GdkEventOwnerChange
with reason NEW_OWNER and owner set to NULL. I don't think it's a bug.
>
> Would this be enough ?
Sure.
Cheers,
Jakub
>
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index 5b2c27c..3dbcae6 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -671,7 +671,11 @@ static void clipboard_owner_change(GtkClipboard
> *clipboard,
> return;
> }
>
> -if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER) {
> +if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER ||
> +#ifdef GDK_WINDOWING_X11
> +(!event->owner && GDK_IS_X11_DISPLAY(gdk_display_get_default()))
> +#endif
> +) {
> if (s->clip_grabbed[selection]) {
> /* grab was sent to the agent, so release it */
> s->clip_grabbed[selection] = FALSE;
> @@ -690,13 +694,6 @@ static void clipboard_owner_change(GtkClipboard
> *clipboard,
>
> s->clipboard_by_guest[selection] = FALSE;
>
> -#ifdef GDK_WINDOWING_X11
> -if (!event->owner && GDK_IS_X11_DISPLAY(gdk_display_get_default())) {
> -s->clip_hasdata[selection] = FALSE;
> -return;
> -}
> -#endif
> -
>
>
> > > -
> > > -/* We are mostly interested when owner has changed in which case
> > > - * we w
Re: [Spice-devel] [PATCH spice-gtk 1/2] clipboard: do not release between client grabs
Hi
On Sun, Mar 24, 2019 at 6:50 PM Jakub Janku wrote:
>
> Hi,
>
> On Thu, Mar 21, 2019 at 1:21 PM wrote:
> >
> > From: Marc-André Lureau
> >
> > On the client side, whenever the grab owner changes (and the clipboard
> > was previously grabbed), spice-gtk sends a clipboard release followed
> > immediately by a new grab. But some clipboard managers on the remote
> > side react to clipboard release events by taking a clipboard grab,
> > presumably to avoid empty clipboards.
> >
> > The two grabs, coming from the client and from the remote sides, will
> > race in both directions, which may confuse the client & remote side,
> > as both believe the other side is the current grab owner, and thus
> > further clipboard data requests are likely to fail.
> >
> > Let's avoid sending a release event when re-grabing.
> >
> > The race described above may still happen in other rare circunstances,
> > and will require a protocol change. To avoid the conflict, a discussed
> > solution could use a clipboard serial number.
> >
> > Tested with current linux & windows vdagent. Looking at earlier
> > version of the code, it doesn't seem like subsequent grabs will be
> > treated as an error.
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > src/spice-gtk-session.c | 21 -
> > 1 file changed, 8 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > index b48f92a..0e748b6 100644
> > --- a/src/spice-gtk-session.c
> > +++ b/src/spice-gtk-session.c
> > @@ -569,8 +569,7 @@ static void clipboard_get_targets(GtkClipboard
> > *clipboard,
> > g_return_if_fail(selection != -1);
> >
> > if (s->clip_grabbed[selection]) {
> > -SPICE_DEBUG("Clipboard is already grabbed, ignoring %d atoms",
> > n_atoms);
> > -return;
> > +SPICE_DEBUG("Clipboard is already grabbed, re-grab: %d atoms",
> > n_atoms);
> > }
> >
> > /* Set all Atoms that matches our current protocol implementation */
> > @@ -652,18 +651,14 @@ static void clipboard_owner_change(GtkClipboard
> > *clipboard,
> > return;
> > }
> >
> > -/* In case we sent a grab to the agent, we need to release it now as
> > - * previous clipboard data should not be reachable anymore */
> > -if (s->clip_grabbed[selection]) {
> > -s->clip_grabbed[selection] = FALSE;
> > -if (spice_main_channel_agent_test_capability(s->main,
> > VD_AGENT_CAP_CLIPBOARD_BY_DEMAND)) {
> > -spice_main_channel_clipboard_selection_release(s->main,
> > selection);
> > -}
> > -}
>
> If event->owner is NULL, the clipboard is empty at the moment and this
> function exits without requesting the new targets. So in this case, we
> should still send release to the agent, otherwise the guest might
> think that clipboard data can be provided while the clipboard in the
> client is empty for a long time. Pasting data in the guest would
> result into an invalid request being sent to spice-gtk.
This is going into undocumented territories. But if what you say is
true in commit 9af2c481b74077ab7d6cb9d4bf589f9855a302f5, then yes,
client should probably release the clipboard.
I don't quite understand why gtk+ would allow NEW_OWNER, with owner ==
NULL and empty clipboard. It sounds more like a bug to me.
Would this be enough ?
diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index 5b2c27c..3dbcae6 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -671,7 +671,11 @@ static void clipboard_owner_change(GtkClipboard
*clipboard,
return;
}
-if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER) {
+if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER ||
+#ifdef GDK_WINDOWING_X11
+(!event->owner && GDK_IS_X11_DISPLAY(gdk_display_get_default()))
+#endif
+) {
if (s->clip_grabbed[selection]) {
/* grab was sent to the agent, so release it */
s->clip_grabbed[selection] = FALSE;
@@ -690,13 +694,6 @@ static void clipboard_owner_change(GtkClipboard
*clipboard,
s->clipboard_by_guest[selection] = FALSE;
-#ifdef GDK_WINDOWING_X11
-if (!event->owner && GDK_IS_X11_DISPLAY(gdk_display_get_default())) {
-s->clip_hasdata[selection] = FALSE;
-return;
-}
-#endif
-
> > -
> > -/* We are mostly interested when owner has changed in which case
> > - * we would like to let agent know about new clipboard data. */
> > if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER) {
> > +if (s->clip_grabbed[selection]) {
> > +/* grab was sent to the agent, so release it */
> > +s->clip_grabbed[selection] = FALSE;
> > +if (spice_main_channel_agent_test_capability(s->main,
> > VD_AGENT_CAP_CLIPBOARD_BY_DEMAND)) {
> > +spice_main_channel_clipboard_selection_release(s->main,
> > selection);
> > +}
> > +}
> > s->clip_hasdata[selection] = FALSE;
> > re
Re: [Spice-devel] [PATCH spice-gtk 1/2] clipboard: do not release between client grabs
Hi,
On Thu, Mar 21, 2019 at 1:21 PM wrote:
>
> From: Marc-André Lureau
>
> On the client side, whenever the grab owner changes (and the clipboard
> was previously grabbed), spice-gtk sends a clipboard release followed
> immediately by a new grab. But some clipboard managers on the remote
> side react to clipboard release events by taking a clipboard grab,
> presumably to avoid empty clipboards.
>
> The two grabs, coming from the client and from the remote sides, will
> race in both directions, which may confuse the client & remote side,
> as both believe the other side is the current grab owner, and thus
> further clipboard data requests are likely to fail.
>
> Let's avoid sending a release event when re-grabing.
>
> The race described above may still happen in other rare circunstances,
> and will require a protocol change. To avoid the conflict, a discussed
> solution could use a clipboard serial number.
>
> Tested with current linux & windows vdagent. Looking at earlier
> version of the code, it doesn't seem like subsequent grabs will be
> treated as an error.
>
> Signed-off-by: Marc-André Lureau
> ---
> src/spice-gtk-session.c | 21 -
> 1 file changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index b48f92a..0e748b6 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -569,8 +569,7 @@ static void clipboard_get_targets(GtkClipboard *clipboard,
> g_return_if_fail(selection != -1);
>
> if (s->clip_grabbed[selection]) {
> -SPICE_DEBUG("Clipboard is already grabbed, ignoring %d atoms",
> n_atoms);
> -return;
> +SPICE_DEBUG("Clipboard is already grabbed, re-grab: %d atoms",
> n_atoms);
> }
>
> /* Set all Atoms that matches our current protocol implementation */
> @@ -652,18 +651,14 @@ static void clipboard_owner_change(GtkClipboard
> *clipboard,
> return;
> }
>
> -/* In case we sent a grab to the agent, we need to release it now as
> - * previous clipboard data should not be reachable anymore */
> -if (s->clip_grabbed[selection]) {
> -s->clip_grabbed[selection] = FALSE;
> -if (spice_main_channel_agent_test_capability(s->main,
> VD_AGENT_CAP_CLIPBOARD_BY_DEMAND)) {
> -spice_main_channel_clipboard_selection_release(s->main,
> selection);
> -}
> -}
If event->owner is NULL, the clipboard is empty at the moment and this
function exits without requesting the new targets. So in this case, we
should still send release to the agent, otherwise the guest might
think that clipboard data can be provided while the clipboard in the
client is empty for a long time. Pasting data in the guest would
result into an invalid request being sent to spice-gtk.
> -
> -/* We are mostly interested when owner has changed in which case
> - * we would like to let agent know about new clipboard data. */
> if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER) {
> +if (s->clip_grabbed[selection]) {
> +/* grab was sent to the agent, so release it */
> +s->clip_grabbed[selection] = FALSE;
> +if (spice_main_channel_agent_test_capability(s->main,
> VD_AGENT_CAP_CLIPBOARD_BY_DEMAND)) {
> +spice_main_channel_clipboard_selection_release(s->main,
> selection);
> +}
> +}
> s->clip_hasdata[selection] = FALSE;
> return;
> }
> --
> 2.21.0.4.g36eb1cb9cf
>
Regards,
Jakub
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-gtk 1/2] clipboard: do not release between client grabs
From: Marc-André Lureau
On the client side, whenever the grab owner changes (and the clipboard
was previously grabbed), spice-gtk sends a clipboard release followed
immediately by a new grab. But some clipboard managers on the remote
side react to clipboard release events by taking a clipboard grab,
presumably to avoid empty clipboards.
The two grabs, coming from the client and from the remote sides, will
race in both directions, which may confuse the client & remote side,
as both believe the other side is the current grab owner, and thus
further clipboard data requests are likely to fail.
Let's avoid sending a release event when re-grabing.
The race described above may still happen in other rare circunstances,
and will require a protocol change. To avoid the conflict, a discussed
solution could use a clipboard serial number.
Tested with current linux & windows vdagent. Looking at earlier
version of the code, it doesn't seem like subsequent grabs will be
treated as an error.
Signed-off-by: Marc-André Lureau
---
src/spice-gtk-session.c | 21 -
1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index b48f92a..0e748b6 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -569,8 +569,7 @@ static void clipboard_get_targets(GtkClipboard *clipboard,
g_return_if_fail(selection != -1);
if (s->clip_grabbed[selection]) {
-SPICE_DEBUG("Clipboard is already grabbed, ignoring %d atoms",
n_atoms);
-return;
+SPICE_DEBUG("Clipboard is already grabbed, re-grab: %d atoms",
n_atoms);
}
/* Set all Atoms that matches our current protocol implementation */
@@ -652,18 +651,14 @@ static void clipboard_owner_change(GtkClipboard
*clipboard,
return;
}
-/* In case we sent a grab to the agent, we need to release it now as
- * previous clipboard data should not be reachable anymore */
-if (s->clip_grabbed[selection]) {
-s->clip_grabbed[selection] = FALSE;
-if (spice_main_channel_agent_test_capability(s->main,
VD_AGENT_CAP_CLIPBOARD_BY_DEMAND)) {
-spice_main_channel_clipboard_selection_release(s->main, selection);
-}
-}
-
-/* We are mostly interested when owner has changed in which case
- * we would like to let agent know about new clipboard data. */
if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER) {
+if (s->clip_grabbed[selection]) {
+/* grab was sent to the agent, so release it */
+s->clip_grabbed[selection] = FALSE;
+if (spice_main_channel_agent_test_capability(s->main,
VD_AGENT_CAP_CLIPBOARD_BY_DEMAND)) {
+spice_main_channel_clipboard_selection_release(s->main,
selection);
+}
+}
s->clip_hasdata[selection] = FALSE;
return;
}
--
2.21.0.4.g36eb1cb9cf
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
