On Thu, 19 Mar 2015 03:38:52 -0400 Marek Chalupa <[email protected]> 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 <[email protected]> > --- > 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@1: error 1: invalid method 1 (since 1 < 3), object wl_pointer@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 _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
