[PATCH weston] text_backend: make destructor call explicit
Derek Foreman
derekf at osg.samsung.com
Thu Jun 25 07:06:52 PDT 2015
Oops, I spoke too soon...
On 25/06/15 09:01 AM, Derek Foreman wrote:
> On 24/06/15 08:28 AM, Pekka Paalanen wrote:
>> From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>>
>> We used to rely on the order in which the
>> weston_compositor::destroy_signal callbacks happened, to not access
>> freed memory. Don't know when, but this broke at least with ivi-shell,
>> which caused crashes in random places on compositor shutdown.
>
> FWIW, I first noticed this breakage at 9a51cd7d but I didn't really try
> to figure out if it was newly introduced or a problem that was just even
> harder to trigger before that point. :)
>
>> Valgrind found the following:
>>
>> Invalid write of size 8
>> at 0xC2EDC69: unbind_input_panel (input-panel-ivi.c:340)
>> by 0x4E3B6BB: destroy_resource (wayland-server.c:537)
>> by 0x4E3E085: for_each_helper.isra.0 (wayland-util.c:359)
>> by 0x4E3E60D: wl_map_for_each (wayland-util.c:365)
>> by 0x4E3BEC7: wl_client_destroy (wayland-server.c:675)
>> by 0x4182F2: text_backend_notifier_destroy (text-backend.c:1047)
>> by 0x4084FB: wl_signal_emit (wayland-server-core.h:264)
>> by 0x4084FB: main (compositor.c:5465)
>> Address 0x67ea360 is 208 bytes inside a block of size 232 free'd
>> at 0x4C2A6BC: free (vg_replace_malloc.c:473)
>> by 0x4084FB: wl_signal_emit (wayland-server-core.h:264)
>> by 0x4084FB: main (compositor.c:5465)
>>
>> Invalid write of size 8
>> at 0x4E3E0D7: wl_list_remove (wayland-util.c:57)
>> by 0xC2EDEE9: destroy_input_panel_surface (input-panel-ivi.c:191)
>> by 0x4E3B6BB: destroy_resource (wayland-server.c:537)
>> by 0x4E3BC7B: wl_resource_destroy (wayland-server.c:550)
>> by 0x40DB8B: wl_signal_emit (wayland-server-core.h:264)
>> by 0x40DB8B: weston_surface_destroy (compositor.c:1883)
>> by 0x40DB8B: weston_surface_destroy (compositor.c:1873)
>> by 0x4E3B6BB: destroy_resource (wayland-server.c:537)
>> by 0x4E3E085: for_each_helper.isra.0 (wayland-util.c:359)
>> by 0x4E3E60D: wl_map_for_each (wayland-util.c:365)
>> by 0x4E3BEC7: wl_client_destroy (wayland-server.c:675)
>> by 0x4182F2: text_backend_notifier_destroy (text-backend.c:1047)
>> by 0x4084FB: wl_signal_emit (wayland-server-core.h:264)
>> by 0x4084FB: main (compositor.c:5465)
>> Address 0x67ea370 is 224 bytes inside a block of size 232 free'd
>> at 0x4C2A6BC: free (vg_replace_malloc.c:473)
>> by 0x4084FB: wl_signal_emit (wayland-server-core.h:264)
>> by 0x4084FB: main (compositor.c:5465)
>>
>> Invalid write of size 8
>> at 0x4E3E0E7: wl_list_remove (wayland-util.c:58)
>> by 0xC2EDEE9: destroy_input_panel_surface (input-panel-ivi.c:191)
>> by 0x4E3B6BB: destroy_resource (wayland-server.c:537)
>> by 0x4E3BC7B: wl_resource_destroy (wayland-server.c:550)
>> by 0x40DB8B: wl_signal_emit (wayland-server-core.h:264)
>> by 0x40DB8B: weston_surface_destroy (compositor.c:1883)
>> by 0x40DB8B: weston_surface_destroy (compositor.c:1873)
>> by 0x4E3B6BB: destroy_resource (wayland-server.c:537)
>> by 0x4E3E085: for_each_helper.isra.0 (wayland-util.c:359)
>> by 0x4E3E60D: wl_map_for_each (wayland-util.c:365)
>> by 0x4E3BEC7: wl_client_destroy (wayland-server.c:675)
>> by 0x4182F2: text_backend_notifier_destroy (text-backend.c:1047)
>> by 0x4084FB: wl_signal_emit (wayland-server-core.h:264)
>> by 0x4084FB: main (compositor.c:5465)
>> Address 0x67ea368 is 216 bytes inside a block of size 232 free'd
>> at 0x4C2A6BC: free (vg_replace_malloc.c:473)
>> by 0x4084FB: wl_signal_emit (wayland-server-core.h:264)
>> by 0x4084FB: main (compositor.c:5465)
>>
>> Looking at the first of these, unbind_input_panel() gets called when the
>> text-backend destroys its helper client which has bound to input_panel
>> interface. This happens after the shell's destroy_signal callback has
>> been called, so the shell has already been freed.
>>
>> The other two errors come from
>> wl_list_remove(&input_panel_surface->link);
>> which has gone stale when the shell was destroyed
>> (shell->input_panel.surfaces list).
>>
>> Rather than creating even more destroy listeners and hooking them up in
>> spaghetti, modify text-backend to not hook up to the compositor destroy
>> signal. Instead, make it the text_backend_init() callers' responsibility
>> to also call text_backend_destroy() appropriately, before the shell goes
>> away.
>
> haha, the destroy listener is how I tried to fix it in
> patchwork.freedesktop.org/patch/52013
>
> I'll close that ticket.
>
> This looks correct to me, and I'm no longer able to reproduce the crash.
>
> Reviewed-By: Derek Foreman <derekf at osg.samsung.com>
I still see the same crash I was seeing previously.
I'll look into that, I still think this is a good idea, and the code
still looks good to me. so the RB stands.
> Tested-By: Derek Foreman <derekf at osg.samsung.com>
^ Let's remove that, though.
>> This fixed all the above Valgrind errors, and avoid a crash with
>> ivi-shell when exiting Weston.
>>
>> Also using desktop-shell exhibited similar Valgrind errors which are
>> fixed by this patch, but those didn't happen to cause any crashes AFAIK.
>>
>> Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>> ---
>> desktop-shell/shell.c | 4 +++-
>> desktop-shell/shell.h | 2 ++
>> ivi-shell/ivi-shell.c | 4 +++-
>> ivi-shell/ivi-shell.h | 2 ++
>> src/compositor.h | 7 ++++++-
>> src/text-backend.c | 18 +++++-------------
>> 6 files changed, 21 insertions(+), 16 deletions(-)
>>
>> diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
>> index 0fe4658..ed7e896 100644
>> --- a/desktop-shell/shell.c
>> +++ b/desktop-shell/shell.c
>> @@ -6347,6 +6347,7 @@ shell_destroy(struct wl_listener *listener, void *data)
>> wl_list_remove(&shell->idle_listener.link);
>> wl_list_remove(&shell->wake_listener.link);
>>
>> + text_backend_destroy(shell->text_backend);
>> input_panel_destroy(shell);
>>
>> wl_list_for_each_safe(shell_output, tmp, &shell->output_list, link) {
>> @@ -6513,7 +6514,8 @@ module_init(struct weston_compositor *ec,
>> if (input_panel_setup(shell) < 0)
>> return -1;
>>
>> - if (text_backend_init(ec) < 0)
>> + shell->text_backend = text_backend_init(ec);
>> + if (!shell->text_backend)
>> return -1;
>>
>> shell_configuration(shell);
>> diff --git a/desktop-shell/shell.h b/desktop-shell/shell.h
>> index f76f181..c0585a2 100644
>> --- a/desktop-shell/shell.h
>> +++ b/desktop-shell/shell.h
>> @@ -148,6 +148,8 @@ struct desktop_shell {
>> bool showing_input_panels;
>> bool prepare_event_sent;
>>
>> + struct text_backend *text_backend;
>> +
>> struct {
>> struct weston_surface *surface;
>> pixman_box32_t cursor_rectangle;
>> diff --git a/ivi-shell/ivi-shell.c b/ivi-shell/ivi-shell.c
>> index 896ba1b..f958fd4 100644
>> --- a/ivi-shell/ivi-shell.c
>> +++ b/ivi-shell/ivi-shell.c
>> @@ -343,6 +343,7 @@ shell_destroy(struct wl_listener *listener, void *data)
>> container_of(listener, struct ivi_shell, destroy_listener);
>> struct ivi_shell_surface *ivisurf, *next;
>>
>> + text_backend_destroy(shell->text_backend);
>> input_panel_destroy(shell);
>>
>> wl_list_for_each_safe(ivisurf, next, &shell->ivi_surface_list, link) {
>> @@ -439,7 +440,8 @@ module_init(struct weston_compositor *compositor,
>> if (input_panel_setup(shell) < 0)
>> goto out_settings;
>>
>> - if (text_backend_init(compositor) < 0)
>> + shell->text_backend = text_backend_init(compositor);
>> + if (!shell->text_backend)
>> goto out_settings;
>>
>> if (wl_global_create(compositor->wl_display,
>> diff --git a/ivi-shell/ivi-shell.h b/ivi-shell/ivi-shell.h
>> index bd2571e..9a05eb2 100644
>> --- a/ivi-shell/ivi-shell.h
>> +++ b/ivi-shell/ivi-shell.h
>> @@ -35,6 +35,8 @@ struct ivi_shell
>>
>> struct wl_list ivi_surface_list; /* struct ivi_shell_surface::link */
>>
>> + struct text_backend *text_backend;
>> +
>> struct wl_listener show_input_panel_listener;
>> struct wl_listener hide_input_panel_listener;
>> struct wl_listener update_input_panel_listener;
>> diff --git a/src/compositor.h b/src/compositor.h
>> index 3586f5b..9069fe1 100644
>> --- a/src/compositor.h
>> +++ b/src/compositor.h
>> @@ -1456,9 +1456,14 @@ weston_screenshooter_shoot(struct weston_output *output, struct weston_buffer *b
>> struct clipboard *
>> clipboard_create(struct weston_seat *seat);
>>
>> -int
>> +struct text_backend;
>> +
>> +struct text_backend *
>> text_backend_init(struct weston_compositor *ec);
>>
>> +void
>> +text_backend_destroy(struct text_backend *text_backend);
>> +
>> struct weston_process;
>> typedef void (*weston_process_cleanup_func_t)(struct weston_process *process,
>> int status);
>> diff --git a/src/text-backend.c b/src/text-backend.c
>> index 55013a2..9485f7e 100644
>> --- a/src/text-backend.c
>> +++ b/src/text-backend.c
>> @@ -109,7 +109,6 @@ struct text_backend {
>> } input_method;
>>
>> struct wl_listener seat_created_listener;
>> - struct wl_listener destroy_listener;
>> };
>>
>> static void
>> @@ -1037,21 +1036,17 @@ text_backend_configuration(struct text_backend *text_backend)
>> free(client);
>> }
>>
>> -static void
>> -text_backend_notifier_destroy(struct wl_listener *listener, void *data)
>> +WL_EXPORT void
>> +text_backend_destroy(struct text_backend *text_backend)
>> {
>> - struct text_backend *text_backend =
>> - container_of(listener, struct text_backend, destroy_listener);
>> -
>> if (text_backend->input_method.client)
>> wl_client_destroy(text_backend->input_method.client);
>>
>> free(text_backend->input_method.path);
>> -
>> free(text_backend);
>> }
>>
>> -WL_EXPORT int
>> +WL_EXPORT struct text_backend *
>> text_backend_init(struct weston_compositor *ec)
>> {
>> struct text_backend *text_backend;
>> @@ -1059,7 +1054,7 @@ text_backend_init(struct weston_compositor *ec)
>>
>> text_backend = zalloc(sizeof(*text_backend));
>> if (text_backend == NULL)
>> - return -1;
>> + return NULL;
>>
>> text_backend->compositor = ec;
>>
>> @@ -1071,10 +1066,7 @@ text_backend_init(struct weston_compositor *ec)
>> wl_signal_add(&ec->seat_created_signal,
>> &text_backend->seat_created_listener);
>>
>> - text_backend->destroy_listener.notify = text_backend_notifier_destroy;
>> - wl_signal_add(&ec->destroy_signal, &text_backend->destroy_listener);
>> -
>> text_input_manager_create(ec);
>>
>> - return 0;
>> + return text_backend;
>> }
>>
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
More information about the wayland-devel
mailing list