On Mon Jun 12 16:37:53 2023 +0000, Alexandros Frantzis wrote:
Thanks @rbernon, I experimented with what you propose and I think it is a good alternative. I am not sure I would consider it simpler than the current proposal (I guess it's complicated in a different way, the implementation details and locking requirements leak into the affected code a bit more), but since it makes trade-offs that seem to be more appropriate for the driver (no thread sync etc), I am happy to go with it in the next iteration of this MR. I also think (but need to ponder on a bit more) we can avoid changing `wayland_surface_clear_role` at all if we keep the `if (surface->xdg_surface == xdg_surface)` in `xdg_surface_handle_configure` (like we are doing in the current approach). I prefer this compared to introducing the extra xdg user data locking in `wayland_surface_clear_role`, if possible. I haven't experimented with the refcount approach, but I find that the double locking is contained enough to not be a problem, for now at least. If we get more cases that can benefit from a more general refcount based pattern, we can revisit (unless you feel strongly about exploring this more now).
I'm fine with the double locking approach for now.
I see now that `wayland_surface_clear_role` is probably fine and I mixed up the object (which validity is guaranteed by libwayland) and the user data (which is not). The user data is only freed in `wayland_surface_destroy`.