[WESTON patch 2/2] tests: add tests for devices handling

[Pekka Paalanen]

On Thu, 19 Mar 2015 03:38:52 -0400
Marek Chalupa <mchqwerty at gmail.com> wrote:

> Test misc races when adding/releasing devices. One of the
> tests reveals a race that is not currently handled by Weston.
> 
> Signed-off-by: Marek Chalupa <mchqwerty at gmail.com>
> ---
>  Makefile.am          |   7 ++-
>  tests/devices-test.c | 160 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 166 insertions(+), 1 deletion(-)
>  create mode 100644 tests/devices-test.c
> 
> diff --git a/Makefile.am b/Makefile.am
> index c509f6e..b73cf59 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -936,7 +936,8 @@ weston_tests =					\
>  	text.weston				\
>  	presentation.weston			\
>  	roles.weston				\
> -	subsurface.weston
> +	subsurface.weston			\
> +	devices.weston
>  
>  
>  AM_TESTS_ENVIRONMENT = \
> @@ -1027,6 +1028,10 @@ button_weston_SOURCES = tests/button-test.c
>  button_weston_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS)
>  button_weston_LDADD = libtest-client.la
>  
> +devices_weston_SOURCES = tests/devices-test.c
> +devices_weston_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS)
> +devices_weston_LDADD = libtest-client.la
> +
>  text_weston_SOURCES = tests/text-test.c
>  nodist_text_weston_SOURCES =			\
>  	protocol/text-protocol.c		\
> diff --git a/tests/devices-test.c b/tests/devices-test.c
> new file mode 100644
> index 0000000..bd32674
> --- /dev/null
> +++ b/tests/devices-test.c
> @@ -0,0 +1,160 @@
> +/*
> + * Copyright © 2015 Red Hat, Inc.
> + *
> + * Permission to use, copy, modify, distribute, and sell this software and
> + * its documentation for any purpose is hereby granted without fee, provided
> + * that the above copyright notice appear in all copies and that both that
> + * copyright notice and this permission notice appear in supporting
> + * documentation, and that the name of the copyright holders not be used in
> + * advertising or publicity pertaining to distribution of the software
> + * without specific, written prior permission.  The copyright holders make
> + * no representations about the suitability of this software for any
> + * purpose.  It is provided "as is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> + * SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
> + * FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY
> + * SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER
> + * RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF
> + * CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
> + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +#include "config.h"
> +
> +#include <string.h>
> +#include "weston-test-client-helper.h"
> +
> +/* simply test if weston sends the right capabilities when
> + * some devices are removed */
> +TEST(seat_capabilities_test)
> +{
> +	struct client *cl = client_create(100, 100, 100, 100);
> +	assert(cl->input->pointer);
> +	weston_test_device_release(cl->test->weston_test, "pointer");
> +	client_roundtrip(cl);
> +	/* do two roundtrips - the first makes sure the device was
> +	 * released, the other that new capabilities came */
> +	client_roundtrip(cl);

Hi,

Why do you need two roundtrips?

Are the new capabilities not sent while processing the device_release
request?

weston_seat_release_pointer() calls seat_send_updated_caps() which
calls wl_seat_send_capabilities(). This means the capability event is
emitted before the compositor has a chance to process the
wl_display.sync, which means the capabilities come necessarily before
the wl_callback.done of the roundtrip.

weston-test-client-helper.c also processes the capability event
immediately.

Am I missing something?

When you *add a keyboard*, then you may want the double-roundtrip to
make sure the keymap event has arrived after creating the wl_keyboard
object.

> +
> +	assert(!cl->input->pointer);
> +	assert(cl->input->keyboard);
> +	weston_test_device_release(cl->test->weston_test, "keyboard");
> +	client_roundtrip(cl);
> +
> +	client_roundtrip(cl);
> +	assert(!cl->input->keyboard);
> +
> +	weston_test_device_add(cl->test->weston_test, "keyboard");
> +	weston_test_device_add(cl->test->weston_test, "pointer");
> +	client_roundtrip(cl);
> +	client_roundtrip(cl);
> +
> +	assert(cl->input->pointer);
> +	assert(cl->input->keyboard);
> +}
> +
> +TEST(device_release_before_destroy)

For completeness, shouldn't we also test device_release_after_destroy?
Which is the usual sequence.

weston-test-client-helper.c uses only the destroy methods, not the
release.

> +{
> +	struct client *cl = client_create(100, 100, 100, 100);
> +
> +	/* we can release pointer when we won't be using it anymore.
> +	 * Do it and see what happens if the device is destroyed
> +	 * right after that */
> +	wl_pointer_release(cl->input->pointer->wl_pointer);
> +	/* we must free and set to NULL the structures, otherwise
> +	 * seat capabilities will double-free them */
> +	free(cl->input->pointer);
> +	cl->input->pointer = NULL;
> +	wl_keyboard_release(cl->input->keyboard->wl_keyboard);
> +	free(cl->input->keyboard);
> +	cl->input->keyboard = NULL;
> +
> +	weston_test_device_release(cl->test->weston_test, "pointer");
> +	weston_test_device_release(cl->test->weston_test, "keyboard");
> +	client_roundtrip(cl);
> +
> +	client_roundtrip(cl);
> +	assert(wl_display_get_error(cl->wl_display) == 0);
> +
> +	/* restore previous state */
> +	weston_test_device_add(cl->test->weston_test, "pointer");
> +	weston_test_device_add(cl->test->weston_test, "keyboard");
> +	client_roundtrip(cl);
> +}
> +
> +TEST(device_release_before_destroy_multiple)
> +{
> +	int i;
> +
> +	/* if weston crashed during this test, then there is
> +	 * some inconsistency */
> +	for (i = 0; i < 100; ++i) {
> +		device_release_before_destroy();
> +	}
> +}
> +
> +/* see https://bugzilla.gnome.org/show_bug.cgi?id=745008
> + * It is a mutter bug, but highly relevant */
> +TEST(get_device_after_destroy)
> +{
> +	struct client *cl = client_create(100, 100, 100, 100);
> +	struct wl_pointer *wl_pointer;
> +	struct wl_keyboard *wl_keyboard;
> +
> +	/* There's a race:
> +	 *  1) compositor destroyes device
> +	 *  2) client asks for the device, because it has not got
> +	 *     new capabilities yet
> +	 *  3) compositor gets request with new_id for destroyed device
> +	 *  4) client uses the new_id
> +	 *  5) client gets new capabilities, destroying the objects
> +	 *
> +	 * If compositor just bail out in step 3) and does not create
> +	 * resource, then client gets error in step 4) - even though
> +	 * it followed the protocol (it just didn't know about new
> +	 * capabilities).
> +	 *
> +	 * This test simulates this situation
> +	 */

A very good case to test indeed.

> +
> +	/* connection is buffered, so after calling client_roundtrip(),
> +	 * this whole batch will be delivered to compositor and will
> +	 * exactly simulate our situation */
> +	weston_test_device_release(cl->test->weston_test, "pointer");
> +	wl_pointer = wl_seat_get_pointer(cl->input->wl_seat);
> +	assert(wl_pointer);
> +
> +	/* this should be ignored */
> +	wl_pointer_set_cursor(wl_pointer, 0, NULL, 0, 0);
> +
> +	/* this should not be ignored */
> +	wl_pointer_release(wl_pointer);
> +	client_roundtrip(cl);
> +
> +	weston_test_device_release(cl->test->weston_test, "keyboard");
> +	wl_keyboard = wl_seat_get_keyboard(cl->input->wl_seat);
> +	assert(wl_keyboard);
> +	wl_keyboard_release(wl_keyboard);
> +	client_roundtrip(cl);
> +
> +	client_roundtrip(cl);
> +	assert(wl_display_get_error(cl->wl_display) == 0);
> +
> +	/* get weston to the previous state, so that other tests
> +	 * have the same environment */
> +	weston_test_device_add(cl->test->weston_test, "pointer");
> +	weston_test_device_add(cl->test->weston_test, "keyboard");
> +	client_roundtrip(cl);

Just for the future... maybe we should somehow make Weston restart for
all or some tests? So clean-up like this would not be necessary. Or is
that a bad idea?

> +}
> +
> +TEST(get_device_afer_destroy_multiple)
> +{
> +	int i;
> +
> +	/* if weston crashed during this test, then there is
> +	 * some inconsistency */
> +	for (i = 0; i < 100; ++i) {
> +		get_device_after_destroy();
> +	}
> +}

I ran this series of 4 patches (one for wayland) and got:

test-client: got global pointer 100 100
test-client: got keyboard keymap
test-client: got surface enter output 0x15d3720
test-client: got keyboard modifiers 0 0 0 0
test-client: got pointer enter 0 0, surface 0x15d3ad0
libwayland: wl_display at 1: error 1: invalid method 1 (since 1 < 3), object wl_pointer at 13
devices.weston: tests/devices-test.c:133: get_device_after_destroy: Assertion `wl_display_roundtrip((cl)->wl_display) 
>= 0' failed.
test "get_device_afer_destroy_multiple":        signal 6, fail.

I suppose the client helpers do not bind the correct version. I think
all tests that attempt to use release requests fail with this.


Is this the test that you expect Weston to fail?
devices.weston: tests/devices-test.c:40: seat_capabilities_test: Assertion `!cl->input->pointer' failed.

There is also a fundamental weakness in all these tests. They assume
that the seat has a keyboard and a pointer to begin with. That might
not be the case when running with a real backend.

A obvious further addition would be the same for wl_touch.

Maybe we should have a separate "test" seat, created by the test
plugin? But I'm not sure how test clients would know to pick the right
seat then.


Thanks,
pq