[PATCH wayland 5/5] tests: test whether the destroyed object is set to NULL on client destruction

[Pekka Paalanen]

On Fri, 13 May 2016 15:01:22 +0200
Marek Chalupa <mchqwerty at gmail.com> wrote:

> when the client is being destroyed, we can use wl_client_get_object()
> to get an object. Test whether it returns NULL when the object
> has been destroyed
> 
> Signed-off-by: Marek Chalupa <mchqwerty at gmail.com>
> ---
>  tests/client-test.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
> diff --git a/tests/client-test.c b/tests/client-test.c
> index cb08e39..f171f19 100644
> --- a/tests/client-test.c
> +++ b/tests/client-test.c
> @@ -126,3 +126,66 @@ TEST(client_post_nomem_on_destruction)
>  	wl_display_destroy(display);
>  }
>  
> +static void
> +seat_destroy_get_object(struct wl_listener *l, void *data)
> +{
> +	struct wl_resource *resource = data;
> +	struct wl_client *client = wl_resource_get_client(resource);
> +	/* This seat was created last,
> +	 * therefore the seat's id is the upper bound for all ids */
> +	uint32_t max_id = wl_resource_get_id(resource);
> +	uint32_t i;
> +
> +	for (i = WL_SERVER_ID_START; i < max_id; ++i) {
> +		/* all these objects are destroyed now, since
> +		 * we destroy the objects in id order */
> +		assert(wl_client_get_object(client, i) == NULL);

Hi,

because the objects are destroyed, calling wl_client_get_object() on
them is illegal. The ids you pass in are not what you expect them to
be. See my comments to patch 2.

Also, I don't think you can rely on the destruction order, so you don't
actually know *if* they have been destroyed already. Unless, you are
specifically also testing for the destruction order in quite a weak
manner.

The legal way to do this would be to record the object ids in a list,
and install a destroy listener for each wl_resource, because that is
the only way to know if an id is still valid for the thing you think it
is.

Making invalid ids guaranteed to return NULL does not get you very far
without explicitly tracking the wl_resource lifetime anyway. If the id
gets re-used, you will get a valid pointer but to a different object.
Checking the object interface and implementation is not enough either,
because it might be another object of the same type, but still not the
object you were looking for.

Therefore NAK from me for this patch.

Makes me wonder if we should actually deprecate
wl_client_get_object(), and maybe offer a new
wl_client_get_display_resource() for the few cases where one just needs
to send an error without having an appropriate wl_resource and/or error
code. Or even a wl_client_send_display_error().


Thanks,
pq

> +	}
> +
> +	/* only our object is not still destroyed */
> +	assert(wl_client_get_object(client, max_id) != NULL);
> +	assert(wl_client_get_object(client, max_id + 1) == NULL);
> +}
> +
> +TEST(client_get_object_on_destruction)
> +{
> +	struct wl_display *display;
> +	struct wl_client *client;
> +	struct wl_resource *resource;
> +	int s[2];
> +
> +	assert(socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, s) == 0);
> +	display = wl_display_create();
> +	assert(display);
> +	client = wl_client_create(display, s[0]);
> +	assert(client);
> +
> +	/* create few other objects
> +	 * -- it doesn't matter what is the interface */
> +	resource = wl_resource_create(client, &wl_seat_interface,
> +				      wl_seat_interface.version, 0);
> +	assert(resource);
> +	resource = wl_resource_create(client, &wl_seat_interface,
> +				      wl_seat_interface.version, 0);
> +	assert(resource);
> +	resource = wl_resource_create(client, &wl_seat_interface,
> +				      wl_seat_interface.version, 0);
> +	assert(resource);
> +
> +
> +	/* for the last one we set destroy listener */
> +	resource = wl_resource_create(client, &wl_seat_interface,
> +				      wl_seat_interface.version, 0);
> +	assert(resource);
> +
> +	seat_destroy_listener.notify = seat_destroy_get_object;
> +	wl_resource_add_destroy_listener(resource, &seat_destroy_listener);
> +
> +	wl_client_destroy(client);
> +
> +	close(s[0]);
> +	close(s[1]);
> +
> +	wl_display_destroy(display);
> +}
> +

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160516/49dbba09/attachment.sig>