GNOME Bugzilla – Bug 744640
wayland: don't try to use pointer on seat before it's initialized
Last modified: 2015-02-18 16:52:47 UTC
Before commit ac448bd42be8cf8e46cadd7e1a209491e33b1693 the pointer object was initialized when the seat was created, now it's only initialized when the device manager finds and loads a pointer device. This commit makes sure we don't try to access the pointer object in meta_wayland_seat_repick if no pointer capability is (yet) found.
Created attachment 296983 [details] [review] wayland: don't try to use pointer on seat before it's initialized
Note, i didn't investigate the code thoroughly, I was just getting a crash on startup and this fixed it
Review of attachment 296983 [details] [review]: uhm, good catch :). Doing this at the seat level indeed looks like the best approach to me, other places we should be probably protecting are: - resource creation on the wl_seat_interface functions - meta_wayland_keyboard_set_focus() AFAICS, most other API entries to meta_wayland_[pointer/keyboard/touch] seem driven by events from the devices themselves, or already handle NULLs properly.
Hi, (In reply to Carlos Garnacho from comment #3) > uhm, good catch :). Doing this at the seat level indeed looks like the best > approach to me, other places we should be probably protecting are: > - resource creation on the wl_seat_interface functions what should we be doing here? post an error? I see in wayland.xml: The ID provided will be initialized to the wl_pointer interface for this seat. This request only takes effect if the seat has the pointer capability. So I guess the answer is "ignore the call" rather than post an error. > AFAICS, most other API entries to meta_wayland_[pointer/keyboard/touch] seem > driven by events from the devices themselves, or already handle NULLs > properly. So oddly, I just got a similar kind of crash from one of the event driven functions. I wonder if we're getting events from devices we're ignoring or something like that. Will investigate tomorrow.
i'll go ahead and attach a more complete patch. It's more aggresive, and checks capabilities any time one of the objects is used even if the function that uses it can get away with using the object uninitialized. I still want to investigate why it's needed for the clutter event handlers, though.
Created attachment 297074 [details] [review] wayland: don't try to use seat devices that aren't present Before commit ac448bd42be8cf8e46cadd7e1a209491e33b1693 the pointer, keyboard, and touch objects were initialized when the seat was created. Now they're initialized later, when the clutter device manager finds and loads them. This commit makes sure we don't try to access those objects if they around initialized (yet).
> I still want to investigate why it's needed for the clutter event handlers, > though. Okay I think I have a handle on what's going on now. The seat code is processing events from master devices that it probably shouldn't be. This also masked a bug where touchpad devices were getting ignored. I'll attach a small patchset that gets things working.
Created attachment 297101 [details] [review] wayland: treat touchpads like mouse devices They both serve the same purpose of moving the pointer around, so they both should be considered pointer devices on the seat.
Created attachment 297102 [details] [review] wayland: discard non-seat events sent to the seat The wayland seat event handlers get sent events that aren't strictly interesting to them (such as events for hardware devices the seat doesn't support and events for virtual devices that the seat needs to ignore). This commit makes sure all uninteresting events get ignored.
Created attachment 297103 [details] [review] wayland: don't try to use seat devices that aren't (yet) present Before commit ac448bd42be8cf8e46cadd7e1a209491e33b1693 the pointer, keyboard, and touch objects were initialized when the seat was created. Now they're initialized later, when the clutter device manager finds and loads them. This commit makes sure we don't try to access those objects if they aren't initialized.
Review of attachment 297102 [details] [review]: ::: src/wayland/meta-wayland-seat.c @@ +273,3 @@ + case CLUTTER_KEYBOARD_DEVICE: + case CLUTTER_TOUCHSCREEN_DEVICE: + supported_device = TRUE; just noticed a whitespace issue here (too much indent)
Review of attachment 297101 [details] [review]: Eek, good catch :)
Review of attachment 297102 [details] [review]: Indeed, I'm not sure Clutter does synthesize events with master=slave on its own, but sounds like a good sanity check for injected events at any other level. Would be lovely though if we could stop seeing misc devices like "integrated camera" as keyboard devices on clutter though...
Review of attachment 297103 [details] [review]: Good catch with get_grab_info(), that might indeed be triggered from (probably misbehaving though) clients. The g_assert()s got me slightly uneasy though, we probably don't want to abort if some extension manages to trick this through clutter_do_event(). Besides that, the patch looks fine to commit to me.
Attachment 297103 [details] pushed as 2aa6dcd - wayland: don't try to use seat devices that aren't (yet) present