After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 772929 - Doesn't unfocus when disabling seat capabilities
Doesn't unfocus when disabling seat capabilities
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: wayland
unspecified
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2016-10-14 12:32 UTC by Carlos Garnacho
Modified: 2016-10-14 16:23 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
wayland: Update capabilities late in meta_wayland_seat_set_capabilities (1.68 KB, patch)
2016-10-14 12:34 UTC, Carlos Garnacho
reviewed Details | Review
wayland: Cancel touches on meta_wayland_touch_disable() (1.07 KB, patch)
2016-10-14 12:34 UTC, Carlos Garnacho
committed Details | Review

Description Carlos Garnacho 2016-10-14 12:32:25 UTC
It seems commit 87f82d9fc0 introduced some issues I see on VT switch. Whenever I have the pointer on a client, switch away and back, and move the pointer, I get crashes around dispatching of pointer events.

I traced it to the capability checks in set_focus(), at the time the devices are disabled, the capability flags have already changed, so the set_focus (..., NULL) during disable() is ignored. I'm attaching a patch that delays changing the capability flags, so the capability is effectively "on" during disable().

I also found out that touchpoints aren't cancelled properly, as the triggered events will arrive after the capability is disabled, attaching another patch for this.
Comment 1 Carlos Garnacho 2016-10-14 12:34:02 UTC
Created attachment 337712 [details] [review]
wayland: Update capabilities late in meta_wayland_seat_set_capabilities

One of the duties in meta_*_disable is unsetting the current focus if it
was non-NULL previously, however the various set_focus() calls now check
for the respective capability to be enabled, which it no longer is at
the time we call set_focus(..., NULL) from disable().

Set the capability flags later in the function, so the disable() methods
find the capability still enabled (which it is, that's why we're shutting
it down).
Comment 2 Carlos Garnacho 2016-10-14 12:34:10 UTC
Created attachment 337713 [details] [review]
wayland: Cancel touches on meta_wayland_touch_disable()

When disabling the device/capability, we can't rely on cancelled events
being emitted timely, because the capability will be already disabled by
then, all touches must be cancelled immediately then.
Comment 3 Carlos Garnacho 2016-10-14 12:41:47 UTC
FWIW, the crash happens because pointer->focus_client is left pointing to random memory, all MetaWaylandPointerClient are freed at disable() time.
Comment 4 Jonas Ådahl 2016-10-14 15:29:37 UTC
Review of attachment 337712 [details] [review]:

Since https://git.gnome.org/browse/mutter/commit/?id=d5d508415148994905c89320f4f6e655d2368c49 we don't check the presence in set_focus() but expect the caller to know what its trying to do.
Comment 5 Jonas Ådahl 2016-10-14 15:34:33 UTC
Review of attachment 337713 [details] [review]:

Looks good.
Comment 6 Carlos Garnacho 2016-10-14 16:21:48 UTC
(In reply to Jonas Ådahl from comment #4)
> Review of attachment 337712 [details] [review] [review]:
> 
> Since
> https://git.gnome.org/browse/mutter/commit/
> ?id=d5d508415148994905c89320f4f6e655d2368c49 we don't check the presence in
> set_focus() but expect the caller to know what its trying to do.

Oops, right, my current branch didn't apparently have it.
Comment 7 Carlos Garnacho 2016-10-14 16:23:40 UTC
Attachment 337713 [details] pushed as 2abee91 - wayland: Cancel touches on meta_wayland_touch_disable()