<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Mon, Mar 30, 2015 at 8:22 AM, Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Mon, 30 Mar 2015 06:37:59 -0400 Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>> wrote: > Test misc races when adding/releasing devices > > v2.: use one roundtrip after releasing devices > add touch support > > Signed-off-by: Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>> > --- > 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 <div><div class="h5">> +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); </div></div>Is this display error check needed? Doesn't client_roundtrip() guarantee success? </blockquote><div>Nope, it is not needed, really. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + > + /* 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? </blockquote><div> </div><div>It is intentional to run it so many times, but it is not intentional to run a hundred clients at a time. </div><div>The problem is that currently we have no destroy function for client. However, the clients do not </div><div>run simultaneously but serially, so the effect should be the same as if we'd destroy them </div><div>(after the client finishes its body, it just 'is' and does nothing until the process exits) </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Maybe it's a good test just like this but needs a comment, otherwise someone might "fix" it. <div><div class="h5"> > + } > +} > + > +/* 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); </div></div>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? <div><div class="h5"> > + } > +} > + > +/* see <a href="https://bugzilla.gnome.org/show_bug.cgi?id=745008" target="_blank">https://bugzilla.gnome.org/show_bug.cgi?id=745008</a> > + * 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); </div></div>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 </blockquote></div> </div><div class="gmail_extra">Fixed the code according the notes, thanks for the review! </div><div class="gmail_extra">Marek </div></div>