On Mon, 5 Dec 2016 16:20:22 +0100 Giulio Camuffo <[email protected]> wrote:
> wl_list_for_each_safe, which is used by wl_signal_emit is not really > safe. If a signal has two listeners, and the first one removes and > re-inits the second one, it would enter an infinite loop, which was hit > in weston on resource destruction, which emits a signal. > This commit adds a new version of wl_signal, called wl_priv_signal, > which is private in wayland-server.c and which does not have this problem. > The old wl_signal cannot be improved without breaking backwards compatibility. > --- > Makefile.am | 4 + > src/wayland-private.h | 18 +++ > src/wayland-server.c | 110 ++++++++++++---- > tests/newsignal-test.c | 337 > +++++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 445 insertions(+), 24 deletions(-) > create mode 100644 tests/newsignal-test.c Hi, this patch fixes the following issue I have been having: Start weston/x11 or weston/wayland, run kcachegrind (Qt4) via Xwayland, hover pointer over any kcachegrind button that shows a tooltip, move pointer away, repeat a couple of times. Sooner rather than later Weston gets into a busyloop and freezes. That no longer happens. Tested-by: Pekka Paalanen <[email protected]> Reviewed-by: Pekka Paalanen <[email protected]> The new tests not only include the old signal tests, you also added tests for the exact case we were tripping over: removing the next callback from current callback while iterating the list of callbacks. It is missing the case of a callback removing the callback added and called just before the current one. You also add tests for removing and adding a listener from a callback to the same signal. Interesting behavioral guarantees. You test for get() returning non-NULL in various cases too, though it would be much better if it could test for the correct pointer rather than just non-NULL. Anyway, I find the tests adequate. You could also add yourself to the copyright holders, this is significant new code IMO. You missed one place that is still using the old wl_signal: event-loop.c, struct wl_event_loop, destroy_signal. Luckily that seems to be a completely private definition, and the implementation can be switched in a follow-up patch. > diff --git a/Makefile.am b/Makefile.am > index d78a0ca..d0c8bd3 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -159,6 +159,7 @@ built_test_programs = \ > socket-test \ > queue-test \ > signal-test \ > + newsignal-test \ > resources-test \ > message-test \ > headers-test \ > @@ -226,6 +227,9 @@ queue_test_SOURCES = tests/queue-test.c > queue_test_LDADD = libtest-runner.la > signal_test_SOURCES = tests/signal-test.c > signal_test_LDADD = libtest-runner.la > +# wayland-server.c is needed here to access wl_priv_* functions > +newsignal_test_SOURCES = tests/newsignal-test.c src/wayland-server.c > +newsignal_test_LDADD = libtest-runner.la The other alternative would be to put wl_priv_*() into wayland-util.c. That one gets built into a static helper lib, so the test programs would get the definitions. > resources_test_SOURCES = tests/resources-test.c > resources_test_LDADD = libtest-runner.la > message_test_SOURCES = tests/message-test.c > diff --git a/src/wayland-private.h b/src/wayland-private.h > index 676b181..434cb04 100644 > --- a/src/wayland-private.h > +++ b/src/wayland-private.h > @@ -35,6 +35,7 @@ > #define WL_HIDE_DEPRECATED 1 > > #include "wayland-util.h" > +#include "wayland-server-core.h" > > /* Invalid memory address */ > #define WL_ARRAY_POISON_PTR (void *) 4 > @@ -233,4 +234,21 @@ zalloc(size_t s) > return calloc(1, s); > } > > +struct wl_priv_signal { > + struct wl_list listener_list; > + struct wl_list emit_list; > +}; > + > +void > +wl_priv_signal_init(struct wl_priv_signal *signal); > + > +void > +wl_priv_signal_add(struct wl_priv_signal *signal, struct wl_listener > *listener); > + > +struct wl_listener * > +wl_priv_signal_get(struct wl_priv_signal *signal, wl_notify_func_t notify); > + > +void > +wl_priv_signal_emit(struct wl_priv_signal *signal, void *data); > + > #endif > diff --git a/src/wayland-server.c b/src/wayland-server.c > index 9d7d9c1..dae3c1d 100644 > --- a/src/wayland-server.c > +++ b/src/wayland-server.c > @@ -78,10 +78,10 @@ struct wl_client { > uint32_t mask; > struct wl_list link; > struct wl_map objects; > - struct wl_signal destroy_signal; > + struct wl_priv_signal destroy_signal; > struct ucred ucred; > int error; > - struct wl_signal resource_created_signal; > + struct wl_priv_signal resource_created_signal; > }; > > struct wl_display { > @@ -97,8 +97,8 @@ struct wl_display { > struct wl_list client_list; > struct wl_list protocol_loggers; > > - struct wl_signal destroy_signal; > - struct wl_signal create_client_signal; > + struct wl_priv_signal destroy_signal; > + struct wl_priv_signal create_client_signal; > > struct wl_array additional_shm_formats; > }; > @@ -117,11 +117,16 @@ struct wl_resource { > struct wl_object object; > wl_resource_destroy_func_t destroy; > struct wl_list link; > - struct wl_signal destroy_signal; > + /* Unfortunately some users of libwayland (e.g. mesa) still use the > + * deprecated wl_resource struct, even if creating it with the new > + * wl_resource_create(). So we cannot change the layout of the struct > + * unless after the data field. */ > + struct wl_signal deprecated_destroy_signal; Right. I see that you are keeping the deprecated signal still fully working. That's excellent. > struct wl_client *client; > void *data; > int version; > wl_dispatcher_func_t dispatcher; > + struct wl_priv_signal destroy_signal; > }; ... > @@ -1743,6 +1750,60 @@ wl_client_for_each_resource(struct wl_client *client, > wl_map_for_each(&client->objects, resource_iterator_helper, &context); > } > > +void > +wl_priv_signal_init(struct wl_priv_signal *signal) > +{ > + wl_list_init(&signal->listener_list); > + wl_list_init(&signal->emit_list); > +} > + > +void > +wl_priv_signal_add(struct wl_priv_signal *signal, struct wl_listener > *listener) > +{ > + wl_list_insert(signal->listener_list.prev, &listener->link); > +} > + > +struct wl_listener * > +wl_priv_signal_get(struct wl_priv_signal *signal, wl_notify_func_t notify) > +{ > + struct wl_listener *l; > + > + wl_list_for_each(l, &signal->listener_list, link) > + if (l->notify == notify) > + return l; > + wl_list_for_each(l, &signal->emit_list, link) > + if (l->notify == notify) > + return l; > + > + return NULL; > +} > + > +void > +wl_priv_signal_emit(struct wl_priv_signal *signal, void *data) > +{ > + struct wl_listener *l; > + struct wl_list *pos; > + > + wl_list_insert_list(&signal->emit_list, &signal->listener_list); > + wl_list_init(&signal->listener_list); > + > + /* Take every element out of the list and put them in a temporary list. > + * This way, the 'it' func can remove any element it wants from the list > + * without troubles, because we always get the first element, not the > + * one after the current, which may be invalid. > + * wl_list_for_each_safe tries to be safe but it fails: it works fine > + * if the current item is removed, but not if the next one is. */ > + while (!wl_list_empty(&signal->emit_list)) { > + pos = signal->emit_list.next; > + l = wl_container_of(pos, l, link); > + > + wl_list_remove(pos); > + wl_list_insert(&signal->listener_list, pos); > + > + l->notify(l, data); > + } > +} > + Yup, the implementation looks correct. The only thing lacking is documentation. I suspect this also makes emitting the same signal from its own callback also work ok, but I'd rather not document that as something that works or has defined behaviour. I would not want to encourage such code. > /** \cond */ /* Deprecated functions below. */ > > uint32_t > @@ -1767,7 +1828,8 @@ wl_client_add_resource(struct wl_client *client, > } > > resource->client = client; > - wl_signal_init(&resource->destroy_signal); > + wl_signal_init(&resource->deprecated_destroy_signal); > + wl_priv_signal_init(&resource->destroy_signal); > > return resource->object.id; > } > diff --git a/tests/newsignal-test.c b/tests/newsignal-test.c > new file mode 100644 > index 0000000..5275537 > --- /dev/null > +++ b/tests/newsignal-test.c > @@ -0,0 +1,337 @@ > +/* > + * Copyright © 2013 Marek Chalupa > + * ... > +struct signal > +{ > + struct wl_priv_signal signal; > + struct wl_listener l1, l2, l3; > + int count; > + struct wl_listener *current; > +}; > + > +static void notify_remove(struct wl_listener *l, void *data) > +{ > + struct signal *sig = data; > + wl_list_remove(&sig->current->link); > + wl_list_init(&sig->current->link); > + sig->count++; > +} > + > +#define INIT \ > + wl_priv_signal_init(&signal.signal); \ > + wl_list_init(&signal.l1.link); \ > + wl_list_init(&signal.l2.link); \ > + wl_list_init(&signal.l3.link); > +#define CHECK_EMIT(expected) \ > + signal.count = 0; \ > + wl_priv_signal_emit(&signal.signal, &signal); \ > + assert(signal.count == expected); Is there any reason to have these as macros that even implicitly use local variables? > + > +TEST(signal_remove_listener) > +{ > + test_set_timeout(4); > + > + struct signal signal; > + > + signal.l1.notify = notify_remove; > + signal.l2.notify = notify_remove; > + signal.l3.notify = notify_remove; > + > + INIT > + wl_priv_signal_add(&signal.signal, &signal.l1); > + > + signal.current = &signal.l1; > + CHECK_EMIT(1) > + CHECK_EMIT(0) > + > + INIT > + wl_priv_signal_add(&signal.signal, &signal.l1); > + wl_priv_signal_add(&signal.signal, &signal.l2); > + > + CHECK_EMIT(2) > + CHECK_EMIT(1) ... > +static void notify_addandget(struct wl_listener *l, void *data) > +{ > + struct signal *signal = data; > + wl_list_remove(&signal->current->link); > + wl_list_init(&signal->current->link); > + wl_priv_signal_add(&signal->signal, signal->current); > + > + assert(wl_priv_signal_get(&signal->signal, signal->current->notify) != > NULL); Too bad it seems to be a bit awkward to ensure we get the correct pointer from get(). > + > + signal->count++; > +} > + > +static void notify_get(struct wl_listener *l, void *data) > +{ > + struct signal *signal = data; > + assert(wl_priv_signal_get(&signal->signal, signal->current->notify) != > NULL); The same as above. > + signal->count++; > +} All my comments are minor and I would be happy to merge this patch as is. If no-one comments in a day or two, I will land this. When I do, I will add your S-o-b which you agreed on IRC, and my test case into the commit message. Thanks, pq
pgpemFuzafWB2.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
