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 <[email protected]>
> ---
>
> 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
> [email protected]
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
_______________________________________________
wayland-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
