On Mon, 30 Mar 2015 06:37:59 -0400 Marek Chalupa <[email protected]> wrote:
> Test misc races when adding/releasing devices > > v2.: use one roundtrip after releasing devices > add touch support > > Signed-off-by: Marek Chalupa <[email protected]> > --- > Makefile.am | 7 +- > tests/devices-test.c | 299 > +++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 305 insertions(+), 1 deletion(-) > create mode 100644 tests/devices-test.c > diff --git a/tests/devices-test.c b/tests/devices-test.c > new file mode 100644 > index 0000000..cf41471 > --- /dev/null > +++ b/tests/devices-test.c > +TEST(device_release_before_destroy) > +{ > + 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; > + > + wl_touch_release(cl->input->touch->wl_touch); > + free(cl->input->touch); > + cl->input->touch = NULL; > + > + weston_test_device_release(cl->test->weston_test, "pointer"); > + weston_test_device_release(cl->test->weston_test, "keyboard"); > + weston_test_device_release(cl->test->weston_test, "touch"); > + client_roundtrip(cl); > + > + assert(cl->input->caps == 0); > + assert(wl_display_get_error(cl->wl_display) == 0); Is this display error check needed? Doesn't client_roundtrip() guarantee success? > + > + /* restore previous state */ > + weston_test_device_add(cl->test->weston_test, "pointer"); > + weston_test_device_add(cl->test->weston_test, "keyboard"); > + weston_test_device_add(cl->test->weston_test, "touch"); > + client_roundtrip(cl); > + > + assert(cl->input->caps == WL_SEAT_CAPABILITY_ALL); > +} > + > +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(); Is it intentional to create a hundred clients without destroying the old ones? Or is that exactly the point? Maybe it's a good test just like this but needs a comment, otherwise someone might "fix" it. > + } > +} > + > +/* normal work-flow test */ > +TEST(device_release_after_destroy) > +{ > + struct client *cl = client_create(100, 100, 100, 100); > + > + weston_test_device_release(cl->test->weston_test, "pointer"); > + wl_pointer_release(cl->input->pointer->wl_pointer); > + /* we must free the memory manually, otherwise seat.capabilities > + * will try to free it and will use invalid proxy */ > + free(cl->input->pointer); > + cl->input->pointer = NULL; > + > + client_roundtrip(cl); > + > + weston_test_device_release(cl->test->weston_test, "keyboard"); > + wl_keyboard_release(cl->input->keyboard->wl_keyboard); > + free(cl->input->keyboard); > + cl->input->keyboard = NULL; > + > + client_roundtrip(cl); > + > + weston_test_device_release(cl->test->weston_test, "touch"); > + wl_touch_release(cl->input->touch->wl_touch); > + free(cl->input->touch); > + cl->input->touch = NULL; > + > + client_roundtrip(cl); > + > + assert(cl->input->caps == 0); > + assert(wl_display_get_error(cl->wl_display) == 0); Is the display error check needed? > + > + /* restore previous state */ > + weston_test_device_add(cl->test->weston_test, "pointer"); > + weston_test_device_add(cl->test->weston_test, "keyboard"); > + weston_test_device_add(cl->test->weston_test, "touch"); > + client_roundtrip(cl); > + > + assert(cl->input->caps == WL_SEAT_CAPABILITY_ALL); > +} > + > +TEST(device_release_after_destroy_multiple) > +{ > + int i; > + > + /* if weston crashed during this test, then there is > + * some inconsistency */ > + for (i = 0; i < 100; ++i) { > + device_release_after_destroy(); Again hundred simultaneous clients? > + } > +} > + > +/* see https://bugzilla.gnome.org/show_bug.cgi?id=745008 > + * It is a mutter bug, but highly relevant. Weston does not > + * suffer from this bug atm, but it is worth of testing. */ > +TEST(get_device_after_destroy) > +{ > + struct client *cl = client_create(100, 100, 100, 100); > + struct wl_pointer *wl_pointer; > + struct wl_keyboard *wl_keyboard; > + struct wl_touch *wl_touch; > + > + /* 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 > + */ > + > + /* 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); > + > + weston_test_device_release(cl->test->weston_test, "touch"); > + wl_touch = wl_seat_get_touch(cl->input->wl_seat); > + assert(wl_touch); > + wl_touch_release(wl_touch); > + client_roundtrip(cl); > + > + assert(wl_display_get_error(cl->wl_display) == 0); And this check. > + > + /* get weston to the previous state, so that other tests > + * have the same environment */ This is such an important note, that it could stand on its own. Like in the beginning of this whole file as a major doc block. > + weston_test_device_add(cl->test->weston_test, "pointer"); > + weston_test_device_add(cl->test->weston_test, "keyboard"); > + weston_test_device_add(cl->test->weston_test, "touch"); > + client_roundtrip(cl); > + > + assert(cl->input->caps == WL_SEAT_CAPABILITY_ALL); > +} > + > +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(); Again a hundred clients. > + } > +} Patches 1, 2 and 4 are R-b me. Patch 3 I commented on IRC, I didn't like the hardcoded call to seat_handle_capabilities(). Thanks, pq _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
