GNOME Bugzilla – Bug 771646
wayland: Clean up left over state when input class is disabled
Last modified: 2016-10-12 02:02:40 UTC
This fixes a crash that happens on for example VT switching. It was introduced by the GObject:ification of the input classes, except that for keyboards, it worked by (statistically good) chance.
Created attachment 335842 [details] [review] wayland: Clean up left over state when input class disabled Don't leave left-over focus client, focus surface, and resource links etc over when disabing an input class. Leaving it around may cause segmentation fault when trying to update focus, when re-enabling.
Review of attachment 335842 [details] [review]: ::: src/wayland/meta-wayland-keyboard.c @@ +675,3 @@ + wl_list_remove (&keyboard->resource_list); + wl_list_remove (&keyboard->focus_resource_list); Doesn't this only remove whatever is the first element on these lists? And, shouldn't these lists get cleaned up automatically when the seat loses the keyboard capability because clients should unbind the resource and thus unbind_resource() getting called for each client? Except we can't rely on clients here but then perhaps we shouldn't wl_list_init() on keyboard_enable and instead should initialize these lists on gobject init instead? In fact, quite a few things we're doing on keyboard_enable/disable should be moved to gobject init/finish handlers. @@ +676,3 @@ + wl_list_remove (&keyboard->resource_list); + wl_list_remove (&keyboard->focus_resource_list); + keyboard->focus_surface = NULL; setting ->focus_surface to NULL is already done above in meta_wayland_keyboard_set_focus(NULL) ::: src/wayland/meta-wayland-pointer.c @@ +504,3 @@ + pointer->focus_surface = NULL; + pointer->focus_client = NULL; meta_wayland_pointer_set_focus(NULL) ?
(In reply to Rui Matos from comment #2) > Review of attachment 335842 [details] [review] [review]: > > ::: src/wayland/meta-wayland-keyboard.c > @@ +675,3 @@ > > + wl_list_remove (&keyboard->resource_list); > + wl_list_remove (&keyboard->focus_resource_list); > > Doesn't this only remove whatever is the first element on these lists? No, it unlinks the list from list elements, i.e. wl_list_remove() of the resource links will still work. > > And, shouldn't these lists get cleaned up automatically when the seat loses > the keyboard capability because clients should unbind the resource and thus > unbind_resource() getting called for each client? > > Except we can't rely on clients here but then perhaps we shouldn't > wl_list_init() on keyboard_enable and instead should initialize these lists > on gobject init instead? The point is to make wl_keyboard (as well as wl_pointer and wl_touch) objects defunct if they were retrieved before the most recent appearance of the capability. > > In fact, quite a few things we're doing on keyboard_enable/disable should be > moved to gobject init/finish handlers. > > @@ +676,3 @@ > + wl_list_remove (&keyboard->resource_list); > + wl_list_remove (&keyboard->focus_resource_list); > + keyboard->focus_surface = NULL; > > setting ->focus_surface to NULL is already done above in > meta_wayland_keyboard_set_focus(NULL) True. Removed. > > ::: src/wayland/meta-wayland-pointer.c > @@ +504,3 @@ > > + pointer->focus_surface = NULL; > + pointer->focus_client = NULL; > > meta_wayland_pointer_set_focus(NULL) ? Yes, should have waited a bit with upload this patch, since just setting those as NULL causes a crash later due to the destroy listener not being detached.
Created attachment 336117 [details] [review] wayland/keyboard: Simplify getting the serial serial Use the same method everywhere for getting the next event serial number.
Created attachment 336118 [details] [review] wayland/keyboard: Stop using temporary wl_list 'l' The variable name 'l' usually refers to a GList iterator, but here it's just a short hand for a specific list. Stop using this shorthand, since it just makes it harder to read what list is used.
Created attachment 336119 [details] [review] wayland/keyboard: Scope variable correctly
Created attachment 336120 [details] [review] wayland/keyboard: Naming and coding style fixes Some very long lines that stood out were shortened, and an old naming convention from weston was removed.
Created attachment 336121 [details] [review] wayland/keyboard: Initialize static state in GObject init func
Created attachment 336122 [details] [review] wayland/keyboard: Cleanup xkb state managing Initialize and cleanup properly in a _init()/_destroy() function pair.
Created attachment 336123 [details] [review] wayland/keyboard: Cleanup grab state managing Initialize on init() and just end grab on disable().
Created attachment 336124 [details] [review] wayland/keyboard: Cleanup resource list management Initialize on init(), unlink and reinitialize the list headers on disable() so that any delayed resource destruction doesn't affect future state.
Created attachment 336125 [details] [review] wayland/seat: Use seat capability checking helper
Created attachment 336126 [details] [review] wayland/keyboard: Check keyboard presence at set focus call site Make the caller of focus setting and grab starting check whether there is a keyboard to update the focus state or start grabbing. It makes it more obvious what to expect, as the call would be a no-op in when no keyboard is present.
Created attachment 336127 [details] [review] wayland/pointer: Use helper for getting the next event serial Unify next event serial retrieval using a helper function.
Created attachment 336128 [details] [review] wayland/pointer: Check pointer presence at set focus call site Make the caller of focus setting check whether there is a pointer to update the focus state of. It makes it more obvious what to expect, as the call would be a no-op in when no pointer is present. Grabbing is still allowed without the presence of a pointer because it is used by popups even on touch-only systems.
Created attachment 336129 [details] [review] wayland/pointer: Unset pointer focus when disabled Previously the focus was reset implicitly by a memset() on the whole MetaWaylandPointer struct. When MetaWaylandPointer was turned into a GObject, this was not possible any more, and the focus was not updated properly.
Created attachment 336130 [details] [review] wayland/pointer: Naming and coding style fixes Some very long lines that stood out were shortened, and an old naming convention from weston was removed.
Created attachment 336131 [details] [review] wayland/pointer: Use grab helper that doesn't focus when disabling Instead of using meta_wayland_pointer_end_grab() which focuses the new grab, add a new helper mean to be used to reset the grab state without changing the pointer focus. When using this function, the call site is supposed to explicitly manage focus.
Comment on attachment 335842 [details] [review] wayland: Clean up left over state when input class disabled Went a head and did some general cleanup while rewriting the first patch.
Review of attachment 336117 [details] [review]: Maybe this should go into the common MetaWaylandInputDevice class now?
Review of attachment 336127 [details] [review]: Move to MetaWaylandInputDevice?
Review of attachment 336118 [details] [review]: lgtm
Review of attachment 336119 [details] [review]: You can do the same on the if (surface != NULL) block below
(In reply to Rui Matos from comment #20) > Review of attachment 336117 [details] [review] [review]: > > Maybe this should go into the common MetaWaylandInputDevice class now? Or MetaWaylandSeat, really. Mostly just wanted a helper function that takes the same input class type as common in the file.
Review of attachment 336120 [details] [review]: sure
Review of attachment 336121 [details] [review]: ++
Review of attachment 336122 [details] [review]: looks good
Review of attachment 336123 [details] [review]: ++
Review of attachment 336124 [details] [review]: looks fine
Review of attachment 336125 [details] [review]: sure
Review of attachment 336126 [details] [review]: Debatable, but ok
Review of attachment 336128 [details] [review]: What about other places calling meta_wayland_pointer_set_focus() ?
Review of attachment 336129 [details] [review]: Oops, looks good ::: src/wayland/meta-wayland-pointer.c @@ +508,2 @@ g_clear_pointer (&pointer->pointer_clients, g_hash_table_unref); pointer->cursor_surface = NULL; Shouldn't this line be replaced with meta_wayland_pointer_set_cursor_surface (pointer, NULL) ?
Review of attachment 336130 [details] [review]: sure
(In reply to Rui Matos from comment #32) > Review of attachment 336128 [details] [review] [review]: > > What about other places calling meta_wayland_pointer_set_focus() ? data-device: its in response to a click, so it's guaranteed to be enabled disable(): has_pointer() already returns FALSE here, but we still need to unset focus focus surface destroy handler: would only be called when there is a focus - with the pointer disabled, no focus sync_focus_surface(): only sets NULL, would be a no-op when disabled anyway. The main reason is that if we do the check inside set_focus(), then we can't call to unset focus after the pointer is disabled.
Review of attachment 336131 [details] [review]: Ok
Review of attachment 336128 [details] [review]: ok, makes sense
*** Bug 772413 has been marked as a duplicate of this bug. ***
Comment on attachment 336129 [details] [review] wayland/pointer: Unset pointer focus when disabled Attachment 336129 [details] pushed as f89162e - wayland/pointer: Unset pointer focus when disabled (Pushing that one, so we can stop carrying it downstream in Fedora)
Attachment 336117 [details] pushed as 911a838 - wayland/keyboard: Simplify getting the serial serial Attachment 336118 [details] pushed as 4a3781d - wayland/keyboard: Stop using temporary wl_list 'l' Attachment 336119 [details] pushed as c221737 - wayland/keyboard: Scope variable correctly Attachment 336120 [details] pushed as 56e8f98 - wayland/keyboard: Naming and coding style fixes Attachment 336121 [details] pushed as c3f7259 - wayland/keyboard: Initialize static state in GObject init func Attachment 336122 [details] pushed as d639c28 - wayland/keyboard: Cleanup xkb state managing Attachment 336123 [details] pushed as 312f215 - wayland/keyboard: Cleanup grab state managing Attachment 336124 [details] pushed as 578e527 - wayland/keyboard: Cleanup resource list management Attachment 336125 [details] pushed as a8c3470 - wayland/seat: Use seat capability checking helper Attachment 336126 [details] pushed as 133bbdf - wayland/keyboard: Check keyboard presence at set focus call site Attachment 336127 [details] pushed as d3cff9a - wayland/pointer: Use helper for getting the next event serial Attachment 336128 [details] pushed as d5d5084 - wayland/pointer: Check pointer presence at set focus call site Attachment 336130 [details] pushed as 93a6be0 - wayland/pointer: Naming and coding style fixes Attachment 336131 [details] pushed as 7990182 - wayland/pointer: Use grab helper that doesn't focus when disabling