On Wed, 14 Dec 2016 14:41:21 +0200 Pekka Paalanen <[email protected]> wrote:
> 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; > > }; ... > > /** \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; > > } ... > > 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. Hi, after sleeping the night, realized something I missed, so I need to at least temporarily withdraw my R-b. If someone was still using the deprecated public form of struct wl_resource, then the code in libwayland-server cannot touch *at all* the new fields added in the private definition. If it did, it would read and write arbitrary application data causing corruption. I have not yet reviewed what this affects, or if we already broke it, but I suspect we need to write more tests to ensure we do not currupt data like that. I'll see if I could have some time for that, but I am also leaving for winter holidays after Wednesday next week, and will be away until Jan 12. Thanks, pq
pgp4fLCI6kIIA.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
