On Fri Jun 2 16:29:58 2023 +0000, Rémi Bernon wrote:
Okay, I will need some time to look and think about that, I don't really have a strong opinion anyway but at the same time, like for the previous MR, I'm afraid that my eventual approval won't be enough.
What about something like that?
```C static pthread_mutex_t xdg_data_mutex = PTHREAD_MUTEX_INITIALIZER;
static struct wayland_surface *wayland_surface_lock_xdg(struct xdg_surface *xdg_surface) { struct wayland_surface *surface;
pthread_mutex_lock(&xdg_data_mutex); surface = xdg_surface_get_user_data(xdg_surface); if (surface) pthread_mutex_lock(&surface->mutex); pthread_mutex_unlock(&xdg_data_mutex);
return surface; }
static void xdg_surface_handle_configure(void *data, struct xdg_surface *xdg_surface, uint32_t serial) { struct wayland_surface *surface; BOOL initial_configure;
TRACE("serial=%u\n", serial);
if (!(surface = wayland_surface_lock_xdg(xdg_surface))) return;
initial_configure = surface->pending_serial == 0 && surface->current_serial == 0; surface->pending_serial = serial; if (initial_configure) pthread_cond_signal(&surface->configured_cond);
pthread_mutex_unlock(&surface->mutex); }
/* [...] */
void wayland_surface_destroy(struct wayland_surface *surface) { pthread_mutex_lock(&xdg_data_mutex);
pthread_mutex_lock(&surface->mutex);
if (surface->xdg_toplevel) { xdg_toplevel_destroy(surface->xdg_toplevel); surface->xdg_toplevel = NULL; }
if (surface->xdg_surface) { xdg_surface_set_user_data(surface->xdg_surface, NULL); xdg_surface_destroy(surface->xdg_surface); surface->xdg_surface = NULL; }
if (surface->wl_surface) { wl_surface_destroy(surface->wl_surface); surface->wl_surface = NULL; }
pthread_mutex_unlock(&surface->mutex);
pthread_mutex_unlock(&xdg_data_mutex);
wl_display_flush(process_wayland.wl_display);
pthread_cond_destroy(&surface->configured_cond); pthread_mutex_destroy(&surface->mutex);
free(surface); }
/* And something similar in wayland_surface_clear_role */
```
I can also imagine something more generic, with refcounted objects for anything that is used as wl_proxy_user_data, and references held as long as they are in user_data, or used in a callback, instead of this double locking which is slightly ugly.
This approach would also avoid unnecessary synchronization between the dispatcher and threads destroying objects. And like I mentioned above, I believe thread synchronization is best avoided in the drivers.