[PATCH weston] text_backend: make destructor call explicit

[Pekka Paalanen]

On Thu, 25 Jun 2015 09:06:52 -0500
Derek Foreman <derekf at osg.samsung.com> wrote:

> 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.

Hi,

cool, and saw your vkb fix cover letter, too. Pushed:
   ffcc452..aa9536a  master -> master


Thanks,
pq