Re: [PATCH] xwm: Fix memory leak

[Scott Moreau]

Hi Pekka,

On Tue, Mar 20, 2018 at 2:21 AM, Pekka Paalanen  wrote:

> On Mon, 19 Mar 2018 18:06:03 -0600
> Scott Moreau  wrote:
>
> > Fix memory leak introduced by 6b58ea8c. weston_wm_handle_icon() was
> > calling xcb_get_property_reply() without freeing the reply.
> > ---
> >  xwayland/window-manager.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c
> > index c307e19..24e7213 100644
> > --- a/xwayland/window-manager.c
> > +++ b/xwayland/window-manager.c
> > @@ -1387,6 +1387,8 @@ weston_wm_handle_icon(struct weston_wm *wm, struct
> weston_wm_window *window)
> >   CAIRO_FORMAT_ARGB32,
> >   width, height, width *
> 4);
> >
> > + free(reply);
> > +
> >   /* Bail out in case anything wrong happened during surface
> creation. */
> >   if (cairo_surface_status(new_surface) != CAIRO_STATUS_SUCCESS) {
> >   cairo_surface_destroy(new_surface);
>
> Hi,
>
> this is strictly an improvement, so I pushed it:
>dc402462..d2cb711d  master -> master
>

Thanks for picking it up, I thought this was a simple one at first too.


>
> It does still miss the early error returns the function, though, so
> it's only a partial solution.
>
> A few seconds later, I thought of a problem with this patch, so I had
> to push a revert immediately (no force-pushing allowed):
>d2cb711d..9fe5d5fa  master -> master
>
> I explained the issue in the revert, and it boils down to
> cairo_image_surface_create_for_data() not making a copy of the memory
> it is passed in, instead it will just keep using the pointer passed in,
> which means we cannot free the data until the cairo surface is
> destroyed. So while there is a leak to fix, the fix needs to be more
> involved.
>

Yes it does need a more involved fix. After hooking up a destroy listener
for the cairo surface, I noticed that window->icon_surface is never being
destroyed either, and there isn't a straight forward fix especially since
window->icon_surface being set to NULL in weston_wm_window_create_frame().

Also as a side note, I notice some strange effect sometimes where the icon
will appear where the mouse cursor is for a split second when opening a
window with an icon, and quickly do fade out animation on it.

Thanks,
Scott


>
> This happens when I try to rush things. Sorry.
>
>
> Thanks,
> pq
>
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH] xwm: Fix memory leak

[Pekka Paalanen]

On Mon, 19 Mar 2018 18:06:03 -0600
Scott Moreau  wrote:

> Fix memory leak introduced by 6b58ea8c. weston_wm_handle_icon() was
> calling xcb_get_property_reply() without freeing the reply.
> ---
>  xwayland/window-manager.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c
> index c307e19..24e7213 100644
> --- a/xwayland/window-manager.c
> +++ b/xwayland/window-manager.c
> @@ -1387,6 +1387,8 @@ weston_wm_handle_icon(struct weston_wm *wm, struct 
> weston_wm_window *window)
>   CAIRO_FORMAT_ARGB32,
>   width, height, width * 4);
>  
> + free(reply);
> +
>   /* Bail out in case anything wrong happened during surface creation. */
>   if (cairo_surface_status(new_surface) != CAIRO_STATUS_SUCCESS) {
>   cairo_surface_destroy(new_surface);

Hi,

this is strictly an improvement, so I pushed it:
   dc402462..d2cb711d  master -> master

It does still miss the early error returns the function, though, so
it's only a partial solution.

A few seconds later, I thought of a problem with this patch, so I had
to push a revert immediately (no force-pushing allowed):
   d2cb711d..9fe5d5fa  master -> master

I explained the issue in the revert, and it boils down to
cairo_image_surface_create_for_data() not making a copy of the memory
it is passed in, instead it will just keep using the pointer passed in,
which means we cannot free the data until the cairo surface is
destroyed. So while there is a leak to fix, the fix needs to be more
involved.

This happens when I try to rush things. Sorry.


Thanks,
pq


pgpm1aAQO0Jge.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH] xwm: Fix memory leak

[Scott Moreau]

Fix memory leak introduced by 6b58ea8c. weston_wm_handle_icon() was
calling xcb_get_property_reply() without freeing the reply.
---
 xwayland/window-manager.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c
index c307e19..24e7213 100644
--- a/xwayland/window-manager.c
+++ b/xwayland/window-manager.c
@@ -1387,6 +1387,8 @@ weston_wm_handle_icon(struct weston_wm *wm, struct 
weston_wm_window *window)
CAIRO_FORMAT_ARGB32,
width, height, width * 4);
 
+   free(reply);
+
/* Bail out in case anything wrong happened during surface creation. */
if (cairo_surface_status(new_surface) != CAIRO_STATUS_SUCCESS) {
cairo_surface_destroy(new_surface);
-- 
2.7.4

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel