[PATCH weston 3/3] ivi-shell: clean up remaining ivisurface during de-init

Discussion:

Harsha M M

2018-08-07 13:35:04 UTC

Signed-off-by: Harsha M M <***@in.bosch.com>
---
ivi-shell/ivi-shell.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/ivi-shell/ivi-shell.c b/ivi-shell/ivi-shell.c
index 58f53bc..92e8f17 100644
--- a/ivi-shell/ivi-shell.c
+++ b/ivi-shell/ivi-shell.c
@@ -366,6 +366,8 @@ shell_destroy(struct wl_listener *listener, void *data)
wl_list_remove(&shell->wake_listener.link);

wl_list_for_each_safe(ivisurf, next, &shell->ivi_surface_list, link) {
+ if (ivisurf->layout_surface != NULL)
+ layout_surface_cleanup(ivisurf);
wl_list_remove(&ivisurf->link);
free(ivisurf);
}

--
2.7.4

Harsha M M

2018-08-07 13:35:02 UTC

Permalink

During de-init ensure removal of added signals from list. Otherwise
a dongling pointer is left behind which will affect other plugins.

Signed-off-by: Harsha M M <***@in.bosch.com>
---
compositor/text-backend.c | 3 +++
compositor/weston-screenshooter.c | 2 ++
desktop-shell/shell.c | 1 +
3 files changed, 6 insertions(+)

diff --git a/compositor/text-backend.c b/compositor/text-backend.c
index 5424242..4b2e854 100644
--- a/compositor/text-backend.c
+++ b/compositor/text-backend.c
@@ -452,6 +452,7 @@ text_input_manager_notifier_destroy(struct wl_listener *listener, void *data)
struct text_input_manager,
destroy_listener);

+ wl_list_remove(&text_input_manager->destroy_listener.link);
wl_global_destroy(text_input_manager->text_input_manager_global);

free(text_input_manager);
@@ -1060,6 +1061,8 @@ text_backend_configuration(struct text_backend *text_backend)
WL_EXPORT void
text_backend_destroy(struct text_backend *text_backend)
{
+ wl_list_remove(&text_backend->seat_created_listener.link);
+
if (text_backend->input_method.client) {
/* disable respawn */
wl_list_remove(&text_backend->client_listener.link);
diff --git a/compositor/weston-screenshooter.c b/compositor/weston-screenshooter.c
index 0994cb4..981aff8 100644
--- a/compositor/weston-screenshooter.c
+++ b/compositor/weston-screenshooter.c
@@ -162,6 +162,8 @@ screenshooter_destroy(struct wl_listener *listener, void *data)
struct screenshooter *shooter =
container_of(listener, struct screenshooter, destroy_listener);

+ wl_list_remove(&shooter->destroy_listener.link);
+
wl_global_destroy(shooter->global);
free(shooter);
}
diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
index ea3c453..9a44715 100644
--- a/desktop-shell/shell.c
+++ b/desktop-shell/shell.c
@@ -4911,6 +4911,7 @@ shell_destroy(struct wl_listener *listener, void *data)
wl_client_destroy(shell->child.client);
}

+ wl_list_remove(&shell->destroy_listener.link);
wl_list_remove(&shell->idle_listener.link);
wl_list_remove(&shell->wake_listener.link);
wl_list_remove(&shell->transform_listener.link);

--
2.7.4

Pekka Paalanen

2018-08-08 09:59:30 UTC

Permalink

On Tue, 7 Aug 2018 19:05:02 +0530

Post by Harsha M M
During de-init ensure removal of added signals from list. Otherwise
a dongling pointer is left behind which will affect other plugins.
---
compositor/text-backend.c | 3 +++
compositor/weston-screenshooter.c | 2 ++
desktop-shell/shell.c | 1 +
3 files changed, 6 insertions(+)
diff --git a/compositor/text-backend.c b/compositor/text-backend.c
index 5424242..4b2e854 100644
--- a/compositor/text-backend.c
+++ b/compositor/text-backend.c
@@ -452,6 +452,7 @@ text_input_manager_notifier_destroy(struct wl_listener *listener, void *data)
struct text_input_manager,
destroy_listener);
+ wl_list_remove(&text_input_manager->destroy_listener.link);
wl_global_destroy(text_input_manager->text_input_manager_global);
free(text_input_manager);
@@ -1060,6 +1061,8 @@ text_backend_configuration(struct text_backend *text_backend)
WL_EXPORT void
text_backend_destroy(struct text_backend *text_backend)
{
+ wl_list_remove(&text_backend->seat_created_listener.link);
+
if (text_backend->input_method.client) {
/* disable respawn */
wl_list_remove(&text_backend->client_listener.link);
diff --git a/compositor/weston-screenshooter.c b/compositor/weston-screenshooter.c
index 0994cb4..981aff8 100644
--- a/compositor/weston-screenshooter.c
+++ b/compositor/weston-screenshooter.c
@@ -162,6 +162,8 @@ screenshooter_destroy(struct wl_listener *listener, void *data)
struct screenshooter *shooter =
container_of(listener, struct screenshooter, destroy_listener);
+ wl_list_remove(&shooter->destroy_listener.link);
+
wl_global_destroy(shooter->global);
free(shooter);
}
diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
index ea3c453..9a44715 100644
--- a/desktop-shell/shell.c
+++ b/desktop-shell/shell.c
@@ -4911,6 +4911,7 @@ shell_destroy(struct wl_listener *listener, void *data)
wl_client_destroy(shell->child.client);
}
+ wl_list_remove(&shell->destroy_listener.link);
wl_list_remove(&shell->idle_listener.link);
wl_list_remove(&shell->wake_listener.link);
wl_list_remove(&shell->transform_listener.link);

Hi,

I would have preferred this patch to be split by the signal from which
the listeners are being removed, but I think it's ok anyway.

The seat_created fix seems obviously good.

The weston_compositor::destroy_signal I have checked that we do already
have at least one listener that gets removed on the callback, so we do
need to have all listeners removed.

Reviewed-by: Pekka Paalanen <***@collabora.co.uk>

Thanks,
pq

Harsha M M

2018-08-07 13:35:03 UTC

Permalink

During de-init ensure removal of compositor destroy notification
from list. Otherwise a dongling pointer is left behind which will
affect other plugins.

Signed-off-by: Harsha M M <***@in.bosch.com>
---
ivi-shell/ivi-shell.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/ivi-shell/ivi-shell.c b/ivi-shell/ivi-shell.c
index 0235d26..58f53bc 100644
--- a/ivi-shell/ivi-shell.c
+++ b/ivi-shell/ivi-shell.c
@@ -358,6 +358,8 @@ shell_destroy(struct wl_listener *listener, void *data)
container_of(listener, struct ivi_shell, destroy_listener);
struct ivi_shell_surface *ivisurf, *next;

+ wl_list_remove(&shell->destroy_listener.link);
+
text_backend_destroy(shell->text_backend);
input_panel_destroy(shell);

--
2.7.4

Pekka Paalanen

2018-08-08 09:45:26 UTC

Permalink

On Tue, 7 Aug 2018 19:05:04 +0530

Post by Harsha M M
---
ivi-shell/ivi-shell.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/ivi-shell/ivi-shell.c b/ivi-shell/ivi-shell.c
index 58f53bc..92e8f17 100644
--- a/ivi-shell/ivi-shell.c
+++ b/ivi-shell/ivi-shell.c
@@ -366,6 +366,8 @@ shell_destroy(struct wl_listener *listener, void *data)
wl_list_remove(&shell->wake_listener.link);
wl_list_for_each_safe(ivisurf, next, &shell->ivi_surface_list, link) {
+ if (ivisurf->layout_surface != NULL)
+ layout_surface_cleanup(ivisurf);
wl_list_remove(&ivisurf->link);
free(ivisurf);
}

Hi,

using the bundled hmi-controller.so, I opened two apps (flower and
simple-shm) on weston/x11, then closed the weston window. That resulted
in the following crash:

Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x00007f3dd8db46c2 in wl_list_empty (list=***@entry=0x90) at src/wayland-util.c:80
80 return list->next == list;
(gdb) bt
#0 0x00007f3dd8db46c2 in wl_list_empty (list=***@entry=0x90) at src/wayland-util.c:80
#1 0x00007f3dd8db46e4 in wl_list_insert_list (list=***@entry=0x558b8b3d8fc0, other=0x90) at src/wayland-util.c:86
#2 0x00007f3dd8fc8f6a in surface_stash_subsurface_views (surface=0x558b8b3d93f0) at /home/pq/git/weston/libweston/compositor.c:2210
#3 0x00007f3dd8fce1d0 in weston_compositor_build_view_list (compositor=0x558b8adcdfa0)
at /home/pq/git/weston/libweston/compositor.c:2318
#4 0x00007f3dd8fce3ac in weston_view_destroy (view=0x558b8b3f49c0) at /home/pq/git/weston/libweston/compositor.c:1922
#5 0x00007f3dcf01890c in ivi_view_destroy (ivi_view=0x558b8b3f4900) at /home/pq/git/weston/ivi-shell/ivi-layout.c:156
#6 0x00007f3dcf01a89d in ivi_layout_surface_destroy (ivisurf=0x558b8b3efb30) at /home/pq/git/weston/ivi-shell/ivi-layout.c:243
#7 0x00007f3dcf01c042 in layout_surface_cleanup (ivisurf=0x558b8af8fda0) at /home/pq/git/weston/ivi-shell/ivi-shell.c:160
#8 0x00007f3dcf01c0fb in shell_destroy (listener=0x558b8af713c0, data=<optimized out>)
at /home/pq/git/weston/ivi-shell/ivi-shell.c:370
#9 0x00007f3dd8fd2804 in wl_signal_emit (data=0x558b8adcdfa0, signal=0x558b8adcdfa0)
at /home/pq/local/include/wayland-server-core.h:468
#10 weston_compositor_destroy (compositor=0x558b8adcdfa0) at /home/pq/git/weston/libweston/compositor.c:6690
#11 0x0000558b88c2271b in main (argc=<optimized out>, argv=<optimized out>) at /home/pq/git/weston/compositor/main.c:2598

If I drop patch 3 from this series, the crash does not happen.

I suppose there is more to fix before this patch can land, otherwise it
did look good.

Thanks,
pq

Harsha Manjula Mallikarjun (RBEI/ECF3)

2018-08-08 09:52:57 UTC

Permalink

-----Original Message-----
Sent: Wednesday, August 08, 2018 3:15 PM
To: Harsha Manjula Mallikarjun (RBEI/ECF3)
Subject: Re: [PATCH weston 3/3] ivi-shell: clean up remaining ivisurface
during de-init
On Tue, 7 Aug 2018 19:05:04 +0530

*data)

Post by Harsha M M
wl_list_remove(&shell->wake_listener.link);
wl_list_for_each_safe(ivisurf, next, &shell->ivi_surface_list, link) {
+ if (ivisurf->layout_surface != NULL)
+ layout_surface_cleanup(ivisurf);
wl_list_remove(&ivisurf->link);
free(ivisurf);
}

Hi,
using the bundled hmi-controller.so, I opened two apps (flower and
simple-shm) on weston/x11, then closed the weston window. That resulted
Program terminated with signal SIGSEGV, Segmentation fault.
80 return list->next == list;
(gdb) bt
#1 0x00007f3dd8db46e4 in wl_list_insert_list
#2 0x00007f3dd8fc8f6a in surface_stash_subsurface_views
(surface=0x558b8b3d93f0) at
/home/pq/git/weston/libweston/compositor.c:2210
#3 0x00007f3dd8fce1d0 in weston_compositor_build_view_list
(compositor=0x558b8adcdfa0)
at /home/pq/git/weston/libweston/compositor.c:2318
#4 0x00007f3dd8fce3ac in weston_view_destroy (view=0x558b8b3f49c0) at
/home/pq/git/weston/libweston/compositor.c:1922
#5 0x00007f3dcf01890c in ivi_view_destroy (ivi_view=0x558b8b3f4900) at
/home/pq/git/weston/ivi-shell/ivi-layout.c:156
#6 0x00007f3dcf01a89d in ivi_layout_surface_destroy
(ivisurf=0x558b8b3efb30) at /home/pq/git/weston/ivi-shell/ivi-layout.c:243
#7 0x00007f3dcf01c042 in layout_surface_cleanup (ivisurf=0x558b8af8fda0)
at /home/pq/git/weston/ivi-shell/ivi-shell.c:160
#8 0x00007f3dcf01c0fb in shell_destroy (listener=0x558b8af713c0, data=<optimized out>)
at /home/pq/git/weston/ivi-shell/ivi-shell.c:370
#9 0x00007f3dd8fd2804 in wl_signal_emit (data=0x558b8adcdfa0,
signal=0x558b8adcdfa0)
at /home/pq/local/include/wayland-server-core.h:468
#10 weston_compositor_destroy (compositor=0x558b8adcdfa0) at
/home/pq/git/weston/libweston/compositor.c:6690
#11 0x0000558b88c2271b in main (argc=<optimized out>, argv=<optimized
out>) at /home/pq/git/weston/compositor/main.c:2598
If I drop patch 3 from this series, the crash does not happen.
I suppose there is more to fix before this patch can land, otherwise it
did look good.

Hi Pekka,

Thanks for the feedback. I will take a look using the bundled hmi-controller.so
and resolve this problem. In my setup I was using ivi-controller from genivi
wayland extensions.

Best Regards,
Harsha

Thanks,
pq

Pekka Paalanen

2018-08-08 09:59:45 UTC

Permalink

On Tue, 7 Aug 2018 19:05:01 +0530

valgrind reports several invalid memory access during shutdown of
weston. In general problem is with components which do not remove
the signals added using wl_signal_emit, but free their memory.
This patch series fixes invalid memory access during shutdown of weston
libweston: Remove signals from the list during de-init
ivi-shell: Remove the compositor destory listener from list during
de-init
ivi-shell: clean up remaining ivisurface during de-init
compositor/text-backend.c | 3 +++
compositor/weston-screenshooter.c | 2 ++
desktop-shell/shell.c | 1 +
ivi-shell/ivi-shell.c | 4 ++++
4 files changed, 10 insertions(+)

Hi,

I made a MR from patches 1 and 2 as I think those are fine:
https://gitlab.freedesktop.org/wayland/weston/merge_requests/3

Patch 3 needs more work.

Thanks,
pq

Pekka Paalanen

2018-08-10 10:32:29 UTC

Permalink

On Wed, 8 Aug 2018 12:59:45 +0300

Post by Pekka Paalanen
On Tue, 7 Aug 2018 19:05:01 +0530

Hi,
https://gitlab.freedesktop.org/wayland/weston/merge_requests/3

After I fixed my own fork to enable CI, force-pushed to trigger it, and
it passed, I merged the MR upstream.

Post by Pekka Paalanen
Patch 3 needs more work.

Thanks,
pq