[PATCH v2] xwm: Fix memory leak

[Scott Moreau]

Hi Derek,

On Fri, Mar 23, 2018 at 12:22 PM, Derek Foreman <derekf at osg.samsung.com>
wrote:

> On 2018-03-20 10:26 AM, Scott Moreau wrote:
> > A memory leak introduced by 6b58ea8c led to me finding a bigger
> > leak, which is xwm was calling frame_create() without calling
> > frame_destroy(). This meant that the associated icon_surface
> > was not being destroyed, leaving the destroy handler for it
> > broken. Here we fix this by calling frame_destroy() when the
> > window is destroyed and free the reply in the icon_surface
> > destroy handler.
>
> Ran valgrind memcheck to try to confirm this resolved a leak, found a
> lot more leaks. ugh. :)
>

Yes, I seem to find more and more too.


>
> > ---
> >  xwayland/window-manager.c | 23 +++++++++++++++++++++--
> >  1 file changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c
> > index c307e19..9e61a67 100644
> > --- a/xwayland/window-manager.c
> > +++ b/xwayland/window-manager.c
> > @@ -1353,6 +1353,14 @@ weston_wm_window_schedule_repaint(struct
> weston_wm_window *window)
> >  }
> >
> >  static void
> > +handle_icon_surface_destroy(void *data)
> > +{
> > +     xcb_get_property_reply_t *reply = (xcb_get_property_reply_t *)data;
> > +
> > +     free(reply);
>
> free takes a void * anyway, so just free(data); should be fine.
>

Ok, great.


>
> > +}
> > +
> > +static void
> >  weston_wm_handle_icon(struct weston_wm *wm, struct weston_wm_window
> *window)
> >  {
> >       xcb_get_property_reply_t *reply;
> > @@ -1371,16 +1379,20 @@ weston_wm_handle_icon(struct weston_wm *wm,
> struct weston_wm_window *window)
> >       length = xcb_get_property_value_length(reply);
> >
> >       /* This is in 32-bit words, not in bytes. */
> > -     if (length < 2)
> > +     if (length < 2) {
> > +             free(reply);
> >               return;
> > +     }
> >
> >       data = xcb_get_property_value(reply);
> >       width = *data++;
> >       height = *data++;
> >
> >       /* Some checks against malformed input. */
> > -     if (width == 0 || height == 0 || length < 2 + width * height)
> > +     if (width == 0 || height == 0 || length < 2 + width * height) {
> > +             free(reply);
> >               return;
> > +     }
> >
> >       new_surface =
> >               cairo_image_surface_create_for_data((unsigned char *)data,
> > @@ -1390,9 +1402,13 @@ weston_wm_handle_icon(struct weston_wm *wm,
> struct weston_wm_window *window)
> >       /* Bail out in case anything wrong happened during surface
> creation. */
> >       if (cairo_surface_status(new_surface) != CAIRO_STATUS_SUCCESS) {
> >               cairo_surface_destroy(new_surface);
> > +             free(reply);
> >               return;
> >       }
> >
> > +     cairo_surface_set_user_data(new_surface, NULL, reply,
>
> (wrong tab size again)
>

My editor was set to 4 wide tabs so this should be fixed now.


>
> Those nits resolved and this is
> Reviewed-by: Derek Foreman <derekf at osg.samsung.com>
>

Great, thanks for the review.


>
> Thanks,
> Derek
>

Thanks,
Scott


>
> > +
>  &handle_icon_surface_destroy);
> > +
> >       if (window->frame)
> >               frame_set_icon(window->frame, new_surface);
> >       else /* We don’t have a frame yet */
> > @@ -1502,6 +1518,9 @@ weston_wm_window_destroy(struct weston_wm_window
> *window)
> >               window->frame_id = XCB_WINDOW_NONE;
> >       }
> >
> > +     if (window->frame)
> > +             frame_destroy(window->frame);
> > +
> >       if (window->surface_id)
> >               wl_list_remove(&window->link);
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180323/282e2054/attachment-0001.html>