[spice-gtk] widget: fix request_auto_usbredir() critical

Submitted by marcandre.lureau@redhat.com on June 6, 2018, 3:23 p.m.

Details

Message ID 20180606152324.16640-1-marcandre.lureau@redhat.com
State New
Headers show
Series "widget: fix request_auto_usbredir() critical" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

marcandre.lureau@redhat.com June 6, 2018, 3:23 p.m.
From: Marc-André Lureau <marcandre.lureau@redhat.com>

On f28, when focusing out of the display, we get the following
critical:

(spicy:15388): GSpice-CRITICAL **: 17:20:07.710:
spice_gtk_session_request_auto_usbredir: assertion
's->auto_usbredir_reqs > 0' failed

This is due to unbalanced gtk+ focus-in and focus-out events (one more
focus-out). This may be fixable on the gtk+ side, but it's also easy
to prevent on our side when the last focus state is unchanged.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 src/spice-widget.c | 3 +++
 1 file changed, 3 insertions(+)


base-commit: 7b67ed4eb6ab530bf58afac8ff4ed190cc951dfb

Patch hide | download patch | download mbox

diff --git a/src/spice-widget.c b/src/spice-widget.c
index 72f5334..2adcf30 100644
--- a/src/spice-widget.c
+++ b/src/spice-widget.c
@@ -207,6 +207,9 @@  static void update_keyboard_focus(SpiceDisplay *display, gboolean state)
 {
     SpiceDisplayPrivate *d = display->priv;
 
+    if (d->keyboard_have_focus == state)
+        return;
+
     d->keyboard_have_focus = state;
     spice_gtk_session_set_keyboard_has_focus(d->gtk_session, state);
 

Comments

On Wed, Jun 06, 2018 at 05:23:24PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> On f28, when focusing out of the display, we get the following
> critical:
> 
> (spicy:15388): GSpice-CRITICAL **: 17:20:07.710:
> spice_gtk_session_request_auto_usbredir: assertion
> 's->auto_usbredir_reqs > 0' failed
> 
> This is due to unbalanced gtk+ focus-in and focus-out events (one more
> focus-out). This may be fixable on the gtk+ side, but it's also easy
> to prevent on our side when the last focus state is unchanged.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Have you seen https://gitlab.gnome.org/GNOME/gtk/issues/792 ?

IMHO it can be prevented in spice-gtk but some warning should be
kept as this is unexpected.

> ---
>  src/spice-widget.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/spice-widget.c b/src/spice-widget.c
> index 72f5334..2adcf30 100644
> --- a/src/spice-widget.c
> +++ b/src/spice-widget.c
> @@ -207,6 +207,9 @@ static void update_keyboard_focus(SpiceDisplay *display, gboolean state)
>  {
>      SpiceDisplayPrivate *d = display->priv;
>  
> +    if (d->keyboard_have_focus == state)
> +        return;
> +
>      d->keyboard_have_focus = state;
>      spice_gtk_session_set_keyboard_has_focus(d->gtk_session, state);
>  
> 
> base-commit: 7b67ed4eb6ab530bf58afac8ff4ed190cc951dfb
> -- 
> 2.18.0.rc1.1.gae296d1cf5
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
On Wed, Jun 6, 2018 at 6:03 PM, Victor Toso <victortoso@redhat.com> wrote:
> On Wed, Jun 06, 2018 at 05:23:24PM +0200, marcandre.lureau@redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> On f28, when focusing out of the display, we get the following
>> critical:
>>
>> (spicy:15388): GSpice-CRITICAL **: 17:20:07.710:
>> spice_gtk_session_request_auto_usbredir: assertion
>> 's->auto_usbredir_reqs > 0' failed
>>
>> This is due to unbalanced gtk+ focus-in and focus-out events (one more
>> focus-out). This may be fixable on the gtk+ side, but it's also easy
>> to prevent on our side when the last focus state is unchanged.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Have you seen https://gitlab.gnome.org/GNOME/gtk/issues/792 ?

Nope, thanks for the link.

>
> IMHO it can be prevented in spice-gtk but some warning should be
> kept as this is unexpected.

The function looks generic enough to prevent doing unnecessary work if
the state is not changed.

I am not sure it's spice-gtk job to report gtk+ bugs :)

How long before the gtk+ bug is fixed (if it's ever fixed)?

I'd just go with this simple patch, and let the gtk+ team decide how
and when to fix the gtk+ behaviour.

>
>> ---
>>  src/spice-widget.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/src/spice-widget.c b/src/spice-widget.c
>> index 72f5334..2adcf30 100644
>> --- a/src/spice-widget.c
>> +++ b/src/spice-widget.c
>> @@ -207,6 +207,9 @@ static void update_keyboard_focus(SpiceDisplay *display, gboolean state)
>>  {
>>      SpiceDisplayPrivate *d = display->priv;
>>
>> +    if (d->keyboard_have_focus == state)
>> +        return;
>> +
>>      d->keyboard_have_focus = state;
>>      spice_gtk_session_set_keyboard_has_focus(d->gtk_session, state);
>>
>>
>> base-commit: 7b67ed4eb6ab530bf58afac8ff4ed190cc951dfb
>> --
>> 2.18.0.rc1.1.gae296d1cf5
>>
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
Hi,

On Wed, Jun 06, 2018 at 06:19:27PM +0200, Marc-André Lureau wrote:
> On Wed, Jun 6, 2018 at 6:03 PM, Victor Toso <victortoso@redhat.com> wrote:
> > On Wed, Jun 06, 2018 at 05:23:24PM +0200, marcandre.lureau@redhat.com wrote:
> >> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>
> >> On f28, when focusing out of the display, we get the following
> >> critical:
> >>
> >> (spicy:15388): GSpice-CRITICAL **: 17:20:07.710:
> >> spice_gtk_session_request_auto_usbredir: assertion
> >> 's->auto_usbredir_reqs > 0' failed
> >>
> >> This is due to unbalanced gtk+ focus-in and focus-out events (one more
> >> focus-out). This may be fixable on the gtk+ side, but it's also easy
> >> to prevent on our side when the last focus state is unchanged.
> >>
> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Have you seen https://gitlab.gnome.org/GNOME/gtk/issues/792 ?

> Nope, thanks for the link.
>
> >
> > IMHO it can be prevented in spice-gtk but some warning should be
> > kept as this is unexpected.
>
> The function looks generic enough to prevent doing unnecessary
> work if the state is not changed.
>
> I am not sure it's spice-gtk job to report gtk+ bugs :)

Sadly we rely on gtk and gtk bugs are easier to track if we have
some info on it. The critical that you are fixing is bad, could
be avoided by this patch but this patch workaround a bug in gtk+
so we should know about it somehow with some debug/warning
message, imho.

> How long before the gtk+ bug is fixed (if it's ever fixed)?

The fix was pushed [0] but need to double check if it was pushed
to the stable branches, etc.

[0] https://gitlab.gnome.org/GNOME/gtk/commit/d3885e92a7db130d96865

> I'd just go with this simple patch, and let the gtk+ team
> decide how and when to fix the gtk+ behaviour.

I'm not saying no for your patch, I prefer warning/debug message
due gtk+ event bug instead of critical message later on.

        toso

> >
> >> ---
> >>  src/spice-widget.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/src/spice-widget.c b/src/spice-widget.c
> >> index 72f5334..2adcf30 100644
> >> --- a/src/spice-widget.c
> >> +++ b/src/spice-widget.c
> >> @@ -207,6 +207,9 @@ static void update_keyboard_focus(SpiceDisplay *display, gboolean state)
> >>  {
> >>      SpiceDisplayPrivate *d = display->priv;
> >>
> >> +    if (d->keyboard_have_focus == state)
> >> +        return;
> >> +
> >>      d->keyboard_have_focus = state;
> >>      spice_gtk_session_set_keyboard_has_focus(d->gtk_session, state);
> >>
> >>
> >> base-commit: 7b67ed4eb6ab530bf58afac8ff4ed190cc951dfb
> >> --
> >> 2.18.0.rc1.1.gae296d1cf5
> >>
> >> _______________________________________________
> >> Spice-devel mailing list
> >> Spice-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
> 
> 
> 
> -- 
> Marc-André Lureau
> 
> Hi,
> 
> On Wed, Jun 06, 2018 at 06:19:27PM +0200, Marc-André Lureau wrote:
> > On Wed, Jun 6, 2018 at 6:03 PM, Victor Toso <victortoso@redhat.com> wrote:
> > > On Wed, Jun 06, 2018 at 05:23:24PM +0200, marcandre.lureau@redhat.com
> > > wrote:
> > >> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >>
> > >> On f28, when focusing out of the display, we get the following
> > >> critical:
> > >>
> > >> (spicy:15388): GSpice-CRITICAL **: 17:20:07.710:
> > >> spice_gtk_session_request_auto_usbredir: assertion
> > >> 's->auto_usbredir_reqs > 0' failed
> > >>
> > >> This is due to unbalanced gtk+ focus-in and focus-out events (one more
> > >> focus-out). This may be fixable on the gtk+ side, but it's also easy
> > >> to prevent on our side when the last focus state is unchanged.
> > >>
> > >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> > > Have you seen https://gitlab.gnome.org/GNOME/gtk/issues/792 ?
> 
> > Nope, thanks for the link.
> >
> > >
> > > IMHO it can be prevented in spice-gtk but some warning should be
> > > kept as this is unexpected.
> >
> > The function looks generic enough to prevent doing unnecessary
> > work if the state is not changed.
> >
> > I am not sure it's spice-gtk job to report gtk+ bugs :)
> 
> Sadly we rely on gtk and gtk bugs are easier to track if we have
> some info on it. The critical that you are fixing is bad, could
> be avoided by this patch but this patch workaround a bug in gtk+
> so we should know about it somehow with some debug/warning
> message, imho.
> 
> > How long before the gtk+ bug is fixed (if it's ever fixed)?
> 
> The fix was pushed [0] but need to double check if it was pushed
> to the stable branches, etc.
> 
> [0] https://gitlab.gnome.org/GNOME/gtk/commit/d3885e92a7db130d96865
> 
> > I'd just go with this simple patch, and let the gtk+ team
> > decide how and when to fix the gtk+ behaviour.
> 
> I'm not saying no for your patch, I prefer warning/debug message
> due gtk+ event bug instead of critical message later on.
> 
>         toso
> 

Is it only me or this patch is fixing an usb error changing
focus handling?
Won't be better to have also a similar check in 
spice_gtk_session_request_auto_usbredir ?

like:

    if (state == s->auto_usbredir_reqs) {
        return;
    }
    s->auto_usbredir_reqs = state;

so on first TRUE will change, on second/third/so on will ignore,
first disable will disable, other disable ignore?

> > >
> > >> ---
> > >>  src/spice-widget.c | 3 +++
> > >>  1 file changed, 3 insertions(+)
> > >>
> > >> diff --git a/src/spice-widget.c b/src/spice-widget.c
> > >> index 72f5334..2adcf30 100644
> > >> --- a/src/spice-widget.c
> > >> +++ b/src/spice-widget.c
> > >> @@ -207,6 +207,9 @@ static void update_keyboard_focus(SpiceDisplay
> > >> *display, gboolean state)
> > >>  {
> > >>      SpiceDisplayPrivate *d = display->priv;
> > >>
> > >> +    if (d->keyboard_have_focus == state)
> > >> +        return;
> > >> +
> > >>      d->keyboard_have_focus = state;
> > >>      spice_gtk_session_set_keyboard_has_focus(d->gtk_session, state);
> > >>
> > >>
> > >> base-commit: 7b67ed4eb6ab530bf58afac8ff4ed190cc951dfb