[PATCH wayland 1/2] client: Keep track of proxy validity and number of reference holders

Kristian Høgsberg hoegsberg at gmail.com
Mon Nov 5 14:11:34 PST 2012


On Sat, Nov 03, 2012 at 10:26:10PM +0100, Jonas Ådahl wrote:
> When events are queued, the associated proxy objects (target proxy and
> potentially closure argument proxies) are verified being valid. However,
> as any event may destroy some proxy object, validity needs to be
> verified again before dispatching. Before this change this was done by
> again looking up the object via the display object map, but that did not
> work because a delete_id event could be dispatched out-of-order if it
> was queued in another queue, causing the object map to either have a new
> proxy object with the same id or none at all, had it been destroyed in
> an earlier event in the queue.
> 
> Instead, make wl_proxy reference counted and increase the reference
> counter of every object associated with an event when it is queued. In
> wl_proxy_destroy() set a flag saying the proxy has been destroyed by the
> application and only free the proxy if the reference counter reaches
> zero after decreasing it.
> 
> Before dispatching, verify that a proxy object still is valid by
> checking that the flag set in wl_proxy_destroy() has not been set. When
> dequeuing the event, all associated proxy objects are dereferenced and
> free:ed if the reference counter reaches zero. As proxy reference counter
> is initiated to 1, when dispatching an event it can never reach zero
> without having the destroyed flag set.
> 
> Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> ---
> 
> Hi Kristian,
> 
> Following up on the discussion the other day about this issue, we cannot
> only rely on reference counting for knowing if we can drop events or not
> since multiple events may be reference holders of the same destroyed
> proxy. Instead I added another wayland-client specific field to
> wl_closure to keep a pointer to the target proxy object.
> 
> To verify validity of the proxy I introduced a new flag (that also
> absorbed `id_deleted') that is set in wl_proxy_destroy(). Reference
> counting is still needed so that wl_proxy_destroy() doesn't free any
> memory before everyone is done with it.

Yes, I agree, and the patch looks good.  Thanks, committed.

Kristian

> Jonas
> 
> 
>  src/wayland-client.c  |  128 +++++++++++++++++++++++++++++++++++++++++--------
>  src/wayland-private.h |    2 +
>  2 files changed, 110 insertions(+), 20 deletions(-)
> 
> diff --git a/src/wayland-client.c b/src/wayland-client.c
> index 7e50b40..d3a7970 100644
> --- a/src/wayland-client.c
> +++ b/src/wayland-client.c
> @@ -45,11 +45,17 @@
>  
>  /** \cond */
>  
> +enum wl_proxy_flag {
> +	WL_PROXY_FLAG_ID_DELETED = (1 << 0),
> +	WL_PROXY_FLAG_DESTROYED = (1 << 1)
> +};
> +
>  struct wl_proxy {
>  	struct wl_object object;
>  	struct wl_display *display;
>  	struct wl_event_queue *queue;
> -	int id_deleted;
> +	uint32_t flags;
> +	int refcount;
>  	void *user_data;
>  };
>  
> @@ -216,7 +222,8 @@ wl_proxy_create(struct wl_proxy *factory, const struct wl_interface *interface)
>  	proxy->object.implementation = NULL;
>  	proxy->display = display;
>  	proxy->queue = factory->queue;
> -	proxy->id_deleted = 0;
> +	proxy->flags = 0;
> +	proxy->refcount = 1;
>  
>  	pthread_mutex_lock(&display->mutex);
>  	proxy->object.id = wl_map_insert_new(&display->objects,
> @@ -243,7 +250,8 @@ wl_proxy_create_for_id(struct wl_proxy *factory,
>  	proxy->object.id = id;
>  	proxy->display = display;
>  	proxy->queue = factory->queue;
> -	proxy->id_deleted = 0;
> +	proxy->flags = 0;
> +	proxy->refcount = 1;
>  
>  	wl_map_insert_at(&display->objects, id, proxy);
>  
> @@ -259,9 +267,11 @@ wl_proxy_create_for_id(struct wl_proxy *factory,
>  WL_EXPORT void
>  wl_proxy_destroy(struct wl_proxy *proxy)
>  {
> -	pthread_mutex_lock(&proxy->display->mutex);
> +	struct wl_display *display = proxy->display;
> +
> +	pthread_mutex_lock(&display->mutex);
>  
> -	if (proxy->id_deleted)
> +	if (proxy->flags & WL_PROXY_FLAG_ID_DELETED)
>  		wl_map_remove(&proxy->display->objects, proxy->object.id);
>  	else if (proxy->object.id < WL_SERVER_ID_START)
>  		wl_map_insert_at(&proxy->display->objects,
> @@ -270,9 +280,14 @@ wl_proxy_destroy(struct wl_proxy *proxy)
>  		wl_map_insert_at(&proxy->display->objects,
>  				 proxy->object.id, NULL);
>  
> -	pthread_mutex_unlock(&proxy->display->mutex);
>  
> -	free(proxy);
> +	proxy->flags |= WL_PROXY_FLAG_DESTROYED;
> +
> +	proxy->refcount--;
> +	if (!proxy->refcount)
> +		free(proxy);
> +
> +	pthread_mutex_unlock(&display->mutex);
>  }
>  
>  /** Set a proxy's listener
> @@ -401,7 +416,7 @@ display_handle_delete_id(void *data, struct wl_display *display, uint32_t id)
>  
>  	proxy = wl_map_lookup(&display->objects, id);
>  	if (proxy != WL_ZOMBIE_OBJECT)
> -		proxy->id_deleted = 1;
> +		proxy->flags |= WL_PROXY_FLAG_ID_DELETED;
>  	else
>  		wl_map_remove(&display->objects, id);
>  
> @@ -514,6 +529,8 @@ wl_display_connect_to_fd(int fd)
>  	display->proxy.object.implementation = (void(**)(void)) &display_listener;
>  	display->proxy.user_data = display;
>  	display->proxy.queue = &display->queue;
> +	display->proxy.flags = 0;
> +	display->proxy.refcount = 1;
>  
>  	display->connection = wl_connection_create(display->fd);
>  	if (display->connection == NULL) {
> @@ -673,6 +690,31 @@ create_proxies(struct wl_proxy *sender, struct wl_closure *closure)
>  	return 0;
>  }
>  
> +static void
> +increase_closure_args_refcount(struct wl_closure *closure)
> +{
> +	const char *signature;
> +	struct argument_details arg;
> +	int i, count;
> +	struct wl_proxy *proxy;
> +
> +	signature = closure->message->signature;
> +	count = arg_count_for_signature(signature) + 2;
> +	for (i = 2; i < count; i++) {
> +		signature = get_next_argument(signature, &arg);
> +		switch (arg.type) {
> +		case 'n':
> +		case 'o':
> +			proxy = *(struct wl_proxy **) closure->args[i];
> +			if (proxy)
> +				proxy->refcount++;
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +}
> +
>  static int
>  queue_event(struct wl_display *display, int len)
>  {
> @@ -709,6 +751,15 @@ queue_event(struct wl_display *display, int len)
>  		return -1;
>  	}
>  
> +	if (wl_closure_lookup_objects(closure, &display->objects) != 0) {
> +		wl_closure_destroy(closure);
> +		return -1;
> +	}
> +
> +	increase_closure_args_refcount(closure);
> +	proxy->refcount++;
> +	closure->proxy = proxy;
> +
>  	if (wl_list_empty(&proxy->queue->event_list))
>  		pthread_cond_signal(&proxy->queue->cond);
>  	wl_list_insert(proxy->queue->event_list.prev, &closure->link);
> @@ -717,31 +768,68 @@ queue_event(struct wl_display *display, int len)
>  }
>  
>  static void
> +decrease_closure_args_refcount(struct wl_closure *closure)
> +{
> +	const char *signature;
> +	struct argument_details arg;
> +	int i, count;
> +	struct wl_proxy *proxy;
> +
> +	signature = closure->message->signature;
> +	count = arg_count_for_signature(signature) + 2;
> +	for (i = 2; i < count; i++) {
> +		signature = get_next_argument(signature, &arg);
> +		switch (arg.type) {
> +		case 'n':
> +		case 'o':
> +			proxy = *(struct wl_proxy **) closure->args[i];
> +			if (proxy) {
> +				if (proxy->flags & WL_PROXY_FLAG_DESTROYED)
> +					*(void **) closure->args[i] = NULL;
> +
> +				proxy->refcount--;
> +				if (!proxy->refcount)
> +					free(proxy);
> +			}
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +}
> +
> +static void
>  dispatch_event(struct wl_display *display, struct wl_event_queue *queue)
>  {
>  	struct wl_closure *closure;
>  	struct wl_proxy *proxy;
> -	uint32_t id;
> -	int opcode, ret;
> +	int opcode;
> +	bool proxy_destroyed;
>  
>  	closure = container_of(queue->event_list.next,
>  			       struct wl_closure, link);
>  	wl_list_remove(&closure->link);
> -	id = closure->buffer[0];
>  	opcode = closure->buffer[1] & 0xffff;
>  
> -	/* Verify that the receiving object is still valid and look up
> -	 * proxies for any arguments.  We have to do this just before
> -	 * calling the handler, since preceeding events may have
> -	 * destroyed either the proxy or the proxy args since the
> -	 * event was queued. */
> -	proxy = wl_map_lookup(&display->objects, id);
> -	ret = wl_closure_lookup_objects(closure, &display->objects);
> +	/* Verify that the receiving object is still valid by checking if has
> +	 * been destroyed by the application. */
> +
> +	decrease_closure_args_refcount(closure);
> +	proxy = closure->proxy;
> +	proxy_destroyed = !!(proxy->flags & WL_PROXY_FLAG_DESTROYED);
> +
> +	proxy->refcount--;
> +	if (!proxy->refcount)
> +		free(proxy);
> +
> +	if (proxy_destroyed) {
> +		wl_closure_destroy(closure);
> +		return;
> +	}
>  
>  	pthread_mutex_unlock(&display->mutex);
>  
> -	if (proxy != WL_ZOMBIE_OBJECT &&
> -	    proxy->object.implementation && ret == 0) {
> +	if (proxy->object.implementation) {
>  		if (wl_debug)
>  			wl_closure_print(closure, &proxy->object, false);
>  
> diff --git a/src/wayland-private.h b/src/wayland-private.h
> index 1b98ce2..4ec9896 100644
> --- a/src/wayland-private.h
> +++ b/src/wayland-private.h
> @@ -59,6 +59,7 @@ void wl_map_for_each(struct wl_map *map, wl_iterator_func_t func, void *data);
>  
>  struct wl_connection;
>  struct wl_closure;
> +struct wl_proxy;
>  
>  struct wl_connection *wl_connection_create(int fd);
>  void wl_connection_destroy(struct wl_connection *connection);
> @@ -80,6 +81,7 @@ struct wl_closure {
>  	void *args[20];
>  	uint32_t *start;
>  	struct wl_list link;
> +	struct wl_proxy *proxy;
>  	uint32_t buffer[0];
>  };
>  
> -- 
> 1.7.10.4
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel


More information about the wayland-devel mailing list