GNOME Bugzilla – Bug 719819
Wayland backend depends on ordering of globals in the display server
Last modified: 2015-05-20 03:51:17 UTC
Currently, when the wl_seat global is received, it assumes that the wl_data_device_manager global has already been seen, and uses it to create a data device. If the wl_seat occurs before the wl_data_device_manager, a crash occurs. Similarly, when the wl_seat global is received, it is assumed that the wl_compositor global has already been seen and tries to create a pointer surface with it. Both of these actions should be moved until after the first round trip to ensure that the required globals have already been bound.
Created attachment 303509 [details] [review] wayland: Don't round trip recursively during initialization Instead use asynchronous round trips that is synchronized in the end of the initialization. This makes it easier to track state, as we won't dispatch arbitrary Wayland messages while processing globals.
Created attachment 303510 [details] [review] wayland: Add global object depedency tracking Some features need certain globals to initialize. In order to deal with these dependencies, add a way to postpone closures that depend on a certain set of globals, that later will be invoked when required globals are all received.
Created attachment 303511 [details] [review] wayland: Use on globals closure for loading cursor themes Instead of putting a 'load cursor themes' call when receiving an wl_shm global, make it a closure that is prepared during initialization.
Created attachment 303512 [details] [review] libwayland-server patch shuffling globals order Attaching a patch that can be applied to the upstream wayland repository. It'll cause globals to be sent in a random order every time a wl_registry is retrieved by the client. With these patches GTK+ seems to be able to start without a problem even with this patch applied. The question though is what globals should be required for GTK+ to succeed. Now it'll just continue even if it didn't receive any globals (even wl_shm), instead probably failing in other places. While wl_compositor, wl_subcompositor, wl_shm, wl_seat, wl_data_device and xdg_shell are more or less required, wl_output may not be there during startup due to hot plugging or other reasons. So, what globals should be required for initialization to succeed?
Review of attachment 303510 [details] [review]: ::: gdk/wayland/gdkdisplay-wayland.c @@ +366,3 @@ + if (handled) + g_hash_table_insert (display_wayland->known_globals, + GUINT_TO_POINTER (id), g_strdup (interface)); Given that the one use for this hash table is to look up globals by their interface, wouldn't it make more sense to flip this around, and make the interface name the key ? Then you can actually use the hash table as it was meant to be used: by doing lookups, instead of iterating over it...
(In reply to Matthias Clasen from comment #5) > Review of attachment 303510 [details] [review] [review]: > > ::: gdk/wayland/gdkdisplay-wayland.c > @@ +366,3 @@ > + if (handled) > + g_hash_table_insert (display_wayland->known_globals, > + GUINT_TO_POINTER (id), g_strdup (interface)); > > Given that the one use for this hash table is to look up globals by their > interface, wouldn't it make more sense to flip this around, and make the > interface name the key ? Then you can actually use the hash table as it was > meant to be used: by doing lookups, instead of iterating over it... There may be more than one global per interface name (multiple seats, multiple outputs), so we wouldn't know how to delete if we didn't use the id as key. We could, I suppose, change it to ignore that globals may be removed, since we only use this during initialization anyway.
Review of attachment 303509 [details] [review]: ok
(In reply to Jonas Ådahl from comment #6) > (In reply to Matthias Clasen from comment #5) > > Review of attachment 303510 [details] [review] [review] [review]: > > > > ::: gdk/wayland/gdkdisplay-wayland.c > > @@ +366,3 @@ > > + if (handled) > > + g_hash_table_insert (display_wayland->known_globals, > > + GUINT_TO_POINTER (id), g_strdup (interface)); > > > > Given that the one use for this hash table is to look up globals by their > > interface, wouldn't it make more sense to flip this around, and make the > > interface name the key ? Then you can actually use the hash table as it was > > meant to be used: by doing lookups, instead of iterating over it... > > There may be more than one global per interface name (multiple seats, > multiple outputs), so we wouldn't know how to delete if we didn't use the id > as key. We could, I suppose, change it to ignore that globals may be > removed, since we only use this during initialization anyway. I see, so it is really just a list of globals. I guess we could store it as a plain list of struct { global; interface} or make the interface name the key and store a list of globals. But given that the list of globals is small and essentially fixed, we could also just leave it as it is. A comment would be useful, though
Review of attachment 303510 [details] [review]: ok, then
Review of attachment 303511 [details] [review]: ok
Attachment 303509 [details] pushed as 181e439 - wayland: Don't round trip recursively during initialization Attachment 303510 [details] pushed as 4e9be39 - wayland: Add global object depedency tracking Attachment 303511 [details] pushed as 7fef713 - wayland: Use on globals closure for loading cursor themes