<div dir="ltr">Hi,<br><div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Mar 19, 2015 at 10:51 AM, Pekka Paalanen <span dir="ltr"><<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Thu, 19 Mar 2015 03:38:52 -0400<br>
Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>> wrote:<br>
<br>
> Test misc races when adding/releasing devices. One of the<br>
> tests reveals a race that is not currently handled by Weston.<br>
><br>
> Signed-off-by: Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>><br>
> ---<br>
> Makefile.am | 7 ++-<br>
> tests/devices-test.c | 160 +++++++++++++++++++++++++++++++++++++++++++++++++++<br>
> 2 files changed, 166 insertions(+), 1 deletion(-)<br>
> create mode 100644 tests/devices-test.c<br>
><br>
> diff --git a/Makefile.am b/Makefile.am<br>
> index c509f6e..b73cf59 100644<br>
> --- a/Makefile.am<br>
> +++ b/Makefile.am<br>
> @@ -936,7 +936,8 @@ weston_tests = \<br>
> text.weston \<br>
> presentation.weston \<br>
> roles.weston \<br>
> - subsurface.weston<br>
> + subsurface.weston \<br>
> + devices.weston<br>
><br>
><br>
> AM_TESTS_ENVIRONMENT = \<br>
> @@ -1027,6 +1028,10 @@ button_weston_SOURCES = tests/button-test.c<br>
> button_weston_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS)<br>
> button_weston_LDADD = <a href="http://libtest-client.la" target="_blank">libtest-client.la</a><br>
><br>
> +devices_weston_SOURCES = tests/devices-test.c<br>
> +devices_weston_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS)<br>
> +devices_weston_LDADD = <a href="http://libtest-client.la" target="_blank">libtest-client.la</a><br>
> +<br>
> text_weston_SOURCES = tests/text-test.c<br>
> nodist_text_weston_SOURCES = \<br>
> protocol/text-protocol.c \<br>
> diff --git a/tests/devices-test.c b/tests/devices-test.c<br>
> new file mode 100644<br>
> index 0000000..bd32674<br>
> --- /dev/null<br>
> +++ b/tests/devices-test.c<br>
> @@ -0,0 +1,160 @@<br>
> +/*<br>
> + * Copyright © 2015 Red Hat, Inc.<br>
> + *<br>
> + * Permission to use, copy, modify, distribute, and sell this software and<br>
> + * its documentation for any purpose is hereby granted without fee, provided<br>
> + * that the above copyright notice appear in all copies and that both that<br>
> + * copyright notice and this permission notice appear in supporting<br>
> + * documentation, and that the name of the copyright holders not be used in<br>
> + * advertising or publicity pertaining to distribution of the software<br>
> + * without specific, written prior permission. The copyright holders make<br>
> + * no representations about the suitability of this software for any<br>
> + * purpose. It is provided "as is" without express or implied warranty.<br>
> + *<br>
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS<br>
> + * SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND<br>
> + * FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY<br>
> + * SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER<br>
> + * RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF<br>
> + * CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN<br>
> + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.<br>
> + */<br>
> +<br>
> +#include "config.h"<br>
> +<br>
> +#include <string.h><br>
> +#include "weston-test-client-helper.h"<br>
> +<br>
> +/* simply test if weston sends the right capabilities when<br>
> + * some devices are removed */<br>
> +TEST(seat_capabilities_test)<br>
> +{<br>
> + struct client *cl = client_create(100, 100, 100, 100);<br>
> + assert(cl->input->pointer);<br>
> + weston_test_device_release(cl->test->weston_test, "pointer");<br>
> + client_roundtrip(cl);<br>
> + /* do two roundtrips - the first makes sure the device was<br>
> + * released, the other that new capabilities came */<br>
> + client_roundtrip(cl);<br>
<br>
</div></div>Hi,<br>
<br>
Why do you need two roundtrips?<br>
<br>
Are the new capabilities not sent while processing the device_release<br>
request?<br></blockquote><div><br></div><div>Yes, they are. The capabilities are received before the wl_callback.done,<br></div><div>so one roundtrip is enough. My mistake. Don't know why I had thought<br>I need two roundtrips.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
weston_seat_release_pointer() calls seat_send_updated_caps() which<br>
calls wl_seat_send_capabilities(). This means the capability event is<br>
emitted before the compositor has a chance to process the<br>
wl_display.sync, which means the capabilities come necessarily before<br>
the wl_callback.done of the roundtrip.<br>
<br>
weston-test-client-helper.c also processes the capability event<br>
immediately.<br>
<br>
Am I missing something?<br>
<br>
When you *add a keyboard*, then you may want the double-roundtrip to<br>
make sure the keymap event has arrived after creating the wl_keyboard<br>
object.<br>
<span class=""><br>
> +<br>
> + assert(!cl->input->pointer);<br>
> + assert(cl->input->keyboard);<br>
> + weston_test_device_release(cl->test->weston_test, "keyboard");<br>
> + client_roundtrip(cl);<br>
> +<br>
> + client_roundtrip(cl);<br>
> + assert(!cl->input->keyboard);<br>
> +<br>
> + weston_test_device_add(cl->test->weston_test, "keyboard");<br>
> + weston_test_device_add(cl->test->weston_test, "pointer");<br>
> + client_roundtrip(cl);<br>
> + client_roundtrip(cl);<br>
> +<br>
> + assert(cl->input->pointer);<br>
> + assert(cl->input->keyboard);<br>
> +}<br>
> +<br>
> +TEST(device_release_before_destroy)<br>
<br>
</span>For completeness, shouldn't we also test device_release_after_destroy?<br>
Which is the usual sequence.<br>
<br>
weston-test-client-helper.c uses only the destroy methods, not the<br>
release.<br></blockquote><div><br></div><div>Good idea<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
> +{<br>
> + struct client *cl = client_create(100, 100, 100, 100);<br>
> +<br>
> + /* we can release pointer when we won't be using it anymore.<br>
> + * Do it and see what happens if the device is destroyed<br>
> + * right after that */<br>
> + wl_pointer_release(cl->input->pointer->wl_pointer);<br>
> + /* we must free and set to NULL the structures, otherwise<br>
> + * seat capabilities will double-free them */<br>
> + free(cl->input->pointer);<br>
> + cl->input->pointer = NULL;<br>
> + wl_keyboard_release(cl->input->keyboard->wl_keyboard);<br>
> + free(cl->input->keyboard);<br>
> + cl->input->keyboard = NULL;<br>
> +<br>
> + weston_test_device_release(cl->test->weston_test, "pointer");<br>
> + weston_test_device_release(cl->test->weston_test, "keyboard");<br>
> + client_roundtrip(cl);<br>
> +<br>
> + client_roundtrip(cl);<br>
> + assert(wl_display_get_error(cl->wl_display) == 0);<br>
> +<br>
> + /* restore previous state */<br>
> + weston_test_device_add(cl->test->weston_test, "pointer");<br>
> + weston_test_device_add(cl->test->weston_test, "keyboard");<br>
> + client_roundtrip(cl);<br>
> +}<br>
> +<br>
> +TEST(device_release_before_destroy_multiple)<br>
> +{<br>
> + int i;<br>
> +<br>
> + /* if weston crashed during this test, then there is<br>
> + * some inconsistency */<br>
> + for (i = 0; i < 100; ++i) {<br>
> + device_release_before_destroy();<br>
> + }<br>
> +}<br>
> +<br>
> +/* see <a href="https://bugzilla.gnome.org/show_bug.cgi?id=745008" target="_blank">https://bugzilla.gnome.org/show_bug.cgi?id=745008</a><br>
> + * It is a mutter bug, but highly relevant */<br>
> +TEST(get_device_after_destroy)<br>
> +{<br>
> + struct client *cl = client_create(100, 100, 100, 100);<br>
> + struct wl_pointer *wl_pointer;<br>
> + struct wl_keyboard *wl_keyboard;<br>
> +<br>
> + /* There's a race:<br>
> + * 1) compositor destroyes device<br>
> + * 2) client asks for the device, because it has not got<br>
> + * new capabilities yet<br>
> + * 3) compositor gets request with new_id for destroyed device<br>
> + * 4) client uses the new_id<br>
> + * 5) client gets new capabilities, destroying the objects<br>
> + *<br>
> + * If compositor just bail out in step 3) and does not create<br>
> + * resource, then client gets error in step 4) - even though<br>
> + * it followed the protocol (it just didn't know about new<br>
> + * capabilities).<br>
> + *<br>
> + * This test simulates this situation<br>
> + */<br>
<br>
</div></div>A very good case to test indeed.<br>
<div><div class="h5"><br>
> +<br>
> + /* connection is buffered, so after calling client_roundtrip(),<br>
> + * this whole batch will be delivered to compositor and will<br>
> + * exactly simulate our situation */<br>
> + weston_test_device_release(cl->test->weston_test, "pointer");<br>
> + wl_pointer = wl_seat_get_pointer(cl->input->wl_seat);<br>
> + assert(wl_pointer);<br>
> +<br>
> + /* this should be ignored */<br>
> + wl_pointer_set_cursor(wl_pointer, 0, NULL, 0, 0);<br>
> +<br>
> + /* this should not be ignored */<br>
> + wl_pointer_release(wl_pointer);<br>
> + client_roundtrip(cl);<br>
> +<br>
> + weston_test_device_release(cl->test->weston_test, "keyboard");<br>
> + wl_keyboard = wl_seat_get_keyboard(cl->input->wl_seat);<br>
> + assert(wl_keyboard);<br>
> + wl_keyboard_release(wl_keyboard);<br>
> + client_roundtrip(cl);<br>
> +<br>
> + client_roundtrip(cl);<br>
> + assert(wl_display_get_error(cl->wl_display) == 0);<br>
> +<br>
> + /* get weston to the previous state, so that other tests<br>
> + * have the same environment */<br>
> + weston_test_device_add(cl->test->weston_test, "pointer");<br>
> + weston_test_device_add(cl->test->weston_test, "keyboard");<br>
> + client_roundtrip(cl);<br>
<br>
</div></div>Just for the future... maybe we should somehow make Weston restart for<br>
all or some tests? So clean-up like this would not be necessary. Or is<br>
that a bad idea?<br></blockquote><div><br></div><div>It is not bad idea, but it needs to be configurable (so I'm in for: 'for some tests').<br>Spawning Weston for all tests could bring big overhead and increase the<br>running time of tests significantly.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> +}<br>
> +<br>
> +TEST(get_device_afer_destroy_multiple)<br>
> +{<br>
> + int i;<br>
> +<br>
> + /* if weston crashed during this test, then there is<br>
> + * some inconsistency */<br>
> + for (i = 0; i < 100; ++i) {<br>
> + get_device_after_destroy();<br>
> + }<br>
> +}<br>
<br>
</span>I ran this series of 4 patches (one for wayland) and got:<br>
<br>
test-client: got global pointer 100 100<br>
test-client: got keyboard keymap<br>
test-client: got surface enter output 0x15d3720<br>
test-client: got keyboard modifiers 0 0 0 0<br>
test-client: got pointer enter 0 0, surface 0x15d3ad0<br>
libwayland: wl_display@1: error 1: invalid method 1 (since 1 < 3), object wl_pointer@13<br>
devices.weston: tests/devices-test.c:133: get_device_after_destroy: Assertion `wl_display_roundtrip((cl)->wl_display)<br>
>= 0' failed.<br>
test "get_device_afer_destroy_multiple": signal 6, fail.<br>
<br>
I suppose the client helpers do not bind the correct version. I think<br>
all tests that attempt to use release requests fail with this.<br>
<br></blockquote><div> </div><div>Oops, yes. I have patched it locally but totally forgot about it. I'll send a patch.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Is this the test that you expect Weston to fail?<br>
devices.weston: tests/devices-test.c:40: seat_capabilities_test: Assertion `!cl->input->pointer' failed.<br></blockquote><div> </div><div>Nope, not at all. Do you have more output? I can't reproduce it.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
There is also a fundamental weakness in all these tests. They assume<br>
that the seat has a keyboard and a pointer to begin with. That might<br>
not be the case when running with a real backend.<br>
<br></blockquote><div> </div><div>Yep. Actually, more (all?) tests assume that. For example keyboard-test.c does not even check if the client has keyboard.<br></div><div>Maybe we should just skip the tests when the backend has insufficient capabilities. (If we won't be using the test seat as you<br></div><div>proposed below)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
A obvious further addition would be the same for wl_touch.<br></blockquote><div><br></div><div>Yes. wl_touch is not implemented at all in tests, so I did do so in this test too. Once we implement it, we can add it.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Maybe we should have a separate "test" seat, created by the test<br>
plugin? But I'm not sure how test clients would know to pick the right<br>
seat then.<br>
<br></blockquote><div><br></div><div>Maybe we could just create test seat with missing objects. I mean when backend provides seat with keyboard but nothing more,<br></div><div>we would create another seat that would provide pointer and touch. How is it in such set-up? Can Weston handle multi-seat?<br></div><div><br>Or (almost) equivalently, the test plugin could just create the missing devices on the default seat when they are not present.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Thanks,<br>
pq<br>
</blockquote></div><br></div><div class="gmail_extra">Thanks for review,<br></div><div class="gmail_extra">Marek<br></div></div></div>