[PATCH wayland] client: Check that the proxy object exists before invoking closure

[Kristian Høgsberg]

On Sun, Oct 28, 2012 at 12:45:40AM +0200, Jonas Ådahl wrote:
> Even when a closure got queued in the queue of a proxy, the proxy object
> can be destroyed during a closure invoke earlier in the queue. Therefore
> check that the proxy object still exists before invoking a closure.
> 
> Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> ---
> 
> Hi,
> 
> I've been seeing crashes in dispatch_event() when destroying the closure,
> and it seems to be because of how frame callbacks were handled in mesa in
> combination with surfaces being destroyed. The cause seems to be a race
> condition occurring when eglSwapBuffers() is called soon after
> eglSurfaceDestroy(), for example when a surface is attached after another
> was destroyed.
> 
> This race condition could be fixed by destroying the callback proxy when
> destroying the surface, but doing this triggers another bug in the event
> queue handling code which is fixed in this patch. What happened was that
> when a proxy object was destroyed as explained above, it could
> potentially happen after a closure associated with the proxy object was
> queued, but before it was invoked.

This happens in the case where we get a wl_callback.done, then
wl_display.delete_id (or the wl_callback) ID, and queue the done event
in the EGL queue and the delete_id in the main queue.  Then we
dispatch the main queue and as part of that calls wl_callback delete,
which, together with the wl_display.delete_id destroys the proxy and
inserts a NULL in the map.

The problem with just checking for proxy == NULL is that if we set the
proxy to NULL, we've seen the delete_id event, but that also means
that we could have reallocated that ID for something else.

The core issue is that we're deleting a proxy associated with one
event queue (EGL) in a callback from another (main).  If you delete
the object from within a callback from its own queue, the order of
events will be fine. A workaround would be to always dispatch all EGL
events before deleting the callback, if we're not running out of an
EGL event callback.

That's not very nice though, and I think that we need to make
wl_proxy::id_deleted into a uint32_t flags field and make id_deleted a
flag and add a WL_PROXY_QUEUED flag.  Then when we queue an event, we
mark the proxy with the QUEUED flag and clear the flag before
dispatching the event.  Then if we get a delete_id and hit
wl_proxy_destroy for a queue proxy, we can insert
WL_QUEUE_ZOMBIE_OBJECT (that'll be (void *) 3) and then check for that
in dispatch_event, don't call out if we get that and call
wl_map_remove() for the ID if we do.  Something like that...

Kristian


> Jonas
> 
>  src/wayland-client.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/wayland-client.c b/src/wayland-client.c
> index 7e50b40..155a5b7 100644
> --- a/src/wayland-client.c
> +++ b/src/wayland-client.c
> @@ -740,7 +740,7 @@ dispatch_event(struct wl_display *display, struct wl_event_queue *queue)
>  
>  	pthread_mutex_unlock(&display->mutex);
>  
> -	if (proxy != WL_ZOMBIE_OBJECT &&
> +	if (proxy && proxy != WL_ZOMBIE_OBJECT &&
>  	    proxy->object.implementation && ret == 0) {
>  		if (wl_debug)
>  			wl_closure_print(closure, &proxy->object, false);
> -- 
> 1.7.10.4
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel