Memory leak fixes pt. 2 #1113

tbzatek · 2020-07-22T16:15:03Z

Another bunch of memleak fixes found by compiling applets in-process. The fixes from #1112 are not included in this branch.

I'm still seeing increasing memory consumption about 1MB an hour but nothing dramatic otherwise.

mate-panel/panel-profile.c

applets/clock/clock.c

mate-panel/panel-run-dialog.c

applets/clock/clock.c

lukefromdc

I just tested this, builds and runs seemingly same as before if not monitoring RAM consumption. I found that on the clock, I was still able to run up panel RAM usage by rapidly opening and closing the calender, by a little if no location is set and "locations" is closed, and my a bit more (a couple MB maybe) if a location is set and "locations" is kept showing above the calendar itself.

Also, we still have that big leak on hovering on an open window's button in the taskbar if and only if previews are enabled: RAM use goes up as much as 50MB on showing the preview and does NOT go back down until another preview is shown, in which case it simply changes to the usage expected for that particular preview. That's also a LOT of RAM for that small preview image. Note that the WM in question is compiz-reloaded in this test.

Still, as I said before, every bit helps and all these will just have to be found one at a time.

lukefromdc · 2020-07-23T05:08:11Z

Note that my test was with all possible applets built in-process

tbzatek · 2020-07-23T10:16:42Z

Thanks for the extensive review, @rbuj. Generally I try to keep changes at minimum and preserve existing data structures. Not sure what the specific MATE Desktop devel guidelines look like, I can go deeper and include slight local refactoring and/or code style cleanup if that's desired.

For instance my commit dd5148c is a good candidate for a better list data structure rewrite but I wanted to preserve the existing array for the time being.

rbuj · 2020-07-23T13:35:25Z

@tbzatek Don't worry about that. When I do a PR review, often I see things that need to be improved ;)

tbzatek · 2020-07-23T15:41:50Z

Also, we still have that big leak on hovering on an open window's button in the taskbar if and only if previews are enabled: RAM use goes up as much as 50MB on showing the preview and does NOT go back down until another preview is shown, in which case it simply changes to the usage expected for that particular preview. That's also a LOT of RAM for that small preview image. Note that the WM in question is compiz-reloaded in this test.

How do I turn thumbnails on while I'm on it? No compiz here though.

I also tend to believe there's a leak somewhere in Pango. While this looked like a static initialization (font cache?) it shows up way too often.

==7308== 6 bytes in 1 blocks are indirectly lost in loss record 287 of 21,417
==7308==    at 0x483AA0F: malloc (vg_replace_malloc.c:306)
==7308==    by 0x5C7A5EA: strdup (in /lib64/libc-2.31.so)
==7308==    by 0x60D64ED: FcPatternAddString (in /usr/lib64/libfontconfig.so.1.12.0)
==7308==    by 0x60AE3E5: pango_fc_font_map_load_fontset (in /usr/lib64/libpangoft2-1.0.so.0.4200.4)
==7308==    by 0x530A658: pango_context_get_metrics (in /usr/lib64/libpango-1.0.so.0.4200.4)
==7308==    by 0x167696: calculate_minimum_height (panel-toplevel.c:2285)

lukefromdc · 2020-07-23T18:29:54Z

To enable the thumbnails in the window list, right-click on the window list's handle (to the left of the first window button), and select "preferences" in the popup menu. Check the "Show thumbnails on hover" box in "Window Thumbnails" and close the dialog. This is NOT applied until the panel is restarted(no update callback or one that does not work), so run mate-panel --replace to restart the panel and apply it. Now open mate-system-monitor and bring up mate-panel in "processes." Watch the RAM use while hovering over thumbnails. To consume around 50MB for those 200px wide thumbnail is a hell of a lot, but leaking it until another thumbnail is generated is even worse.

I just verified I get the same memory leak on the window thumbnails when running marco. Thumbnails are made by wncklet, using data provided by the WM, so this is no suprise.

And use stack-allocated GVariantBuilder for temporary storage.

The PANEL_GLIB_STR_EMPTY() macro returns TRUE even when the memory was allocated yet the string was empty, so do explicit free() here.

Overwriting an already allocated memory.

Iteration over a linked list should be done over a side control variable and list head needs to be preserved for proper list free.

The gtk_tree_view_set_model() adds its own reference.

The gtk_tree_model_get() duplicates memory or adds a reference and any data need to be freed explicitly.

Let the GError ownership on the async finish method call.

rbuj

It's awesome, I have no words to express our gratitude for everything you have done.

applets/clock/set-timezone.c

rcaridade145 · 2020-07-24T14:24:52Z

Amazing work @tbzatek

tbzatek · 2020-07-24T14:25:12Z

I just verified I get the same memory leak on the window thumbnails when running marco. Thumbnails are made by wncklet, using data provided by the WM, so this is no suprise.

Ah, had an old libwnck. For some reason Gentoo only ships libwnck-3.30.0 in its official repos.

Anyway, got it working, I do see some memory consumption increase (~550 MB) but there's nothing obvious in the code nor reported by Valgrind other than

==21333== 4,718,592 bytes in 3 blocks are still reachable in loss record 29,434 of 29,434
==21333==    at 0x4838575: calloc (vg_replace_malloc.c:759)
==21333==    by 0x53BAB12: _cairo_mempool_init (in /usr/lib64/libcairo.so.2.11600.0)
==21333==    by 0x543B6E5: _cairo_xlib_shm_surface_create.lto_priv.0 (in /usr/lib64/libcairo.so.2.11600.0)
==21333==    by 0x543BBC9: _cairo_xlib_surface_get_shm (in /usr/lib64/libcairo.so.2.11600.0)
==21333==    by 0x5433EA9: _cairo_xlib_surface_acquire_source_image.lto_priv.0 (in /usr/lib64/libcairo.so.2.11600.0)
==21333==    by 0x53FE923: _cairo_surface_acquire_source_image (in /usr/lib64/libcairo.so.2.11600.0)
==21333==    by 0x53BA26E: _pixman_image_for_pattern (in /usr/lib64/libcairo.so.2.11600.0)
==21333==    by 0x53BA763: _cairo_image_source_create_for_pattern (in /usr/lib64/libcairo.so.2.11600.0)
==21333==    by 0x53EF045: clip_and_composite_boxes.part.0 (in /usr/lib64/libcairo.so.2.11600.0)
==21333==    by 0x53F03E2: _cairo_spans_compositor_mask.lto_priv.0 (in /usr/lib64/libcairo.so.2.11600.0)
==21333==    by 0x539C4D0: _cairo_compositor_paint (in /usr/lib64/libcairo.so.2.11600.0)
==21333==    by 0x5401C05: _cairo_surface_paint (in /usr/lib64/libcairo.so.2.11600.0)
==21333==    by 0x53AB8D9: _cairo_gstate_paint (in /usr/lib64/libcairo.so.2.11600.0)
==21333==    by 0x5415A74: cairo_paint (in /usr/lib64/libcairo.so.2.11600.0)
==21333==    by 0x5205CE3: gdk_pixbuf_get_from_window (in /usr/lib64/libgdk-3.so.0.2404.17)
==21333==    by 0xC3EB19B: preview_window_thumbnail (window-list.c:182)
==21333==    by 0xC3EB58A: applet_enter_notify_event (window-list.c:296)
==21333==    by 0x577DAFD: g_cclosure_marshal_VOID__POINTERv (in /usr/lib64/libgobject-2.0.so.0.6400.3)
==21333==    by 0x577A2B5: _g_closure_invoke_va (in /usr/lib64/libgobject-2.0.so.0.6400.3)
==21333==    by 0x579B6A5: g_signal_emit_valist (in /usr/lib64/libgobject-2.0.so.0.6400.3)

The GdkPixbuf as returned from gdk_pixbuf_get_from_window() is properly unreff'ed as the object reference count is proper value of 1. This will need to be dealt with separately, out of scope of this pull request. Got a ticket for that?

tbzatek · 2020-07-24T14:29:42Z

I think I'm done with the fixes here, I'll keep running this branch for a couple of days to ensure it's stable.

Please test my fixes, notably the changes in the clock applet are slightly intrusive.

lukefromdc

Builds and runs fine as of today, adding cities works same as in master, and yes we still have the leak on window thumbnails that libwnck was found responsible for. Still, every leak we can cork up here helps, and they will add up over time

lukefromdc · 2020-07-29T06:32:06Z

Oh yeah I just found this fixes #1115 and the error dialog on dismissing the priviliges dialog for changing time zone in calendar now works fine

muktupavels · 2020-07-29T09:10:28Z

Builds and runs fine as of today, adding cities works same as in master, and yes we still have the leak on window thumbnails that libwnck was found responsible for. Still, every leak we can cork up here helps, and they will add up over time

Please don't keep in secret your findings. If you have identified memory leak in libwnck then post needed info so that it can be fixed. Thanks!

raveit65 · 2020-07-29T10:27:20Z

Builds and runs fine as of today, adding cities works same as in master, and yes we still have the leak on window thumbnails that libwnck was found responsible for. Still, every leak we can cork up here helps, and they will add up over time

Please don't keep in secret your findings. If you have identified memory leak in libwnck then post needed info so that it can be fixed. Thanks!

I think i will change the default of this previews to disable for fedora.
Why?

Displaying the preview without disabling the tooltips on top of the previews isn't nice.
In result we got only negative feedback from users.

For myself i have patched Libwnck to disable the tooltips, but in this way they will never displayed.

From: raveit65 <mate@raveit.de>
Date: Sun, 14 Jun 2020 18:47:41 +0200
Subject: [PATCH] task-list: disable tooltips

---
 libwnck/tasklist.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libwnck/tasklist.c b/libwnck/tasklist.c
index 2b20424..85ff463 100644
--- a/libwnck/tasklist.c
+++ b/libwnck/tasklist.c
@@ -3898,6 +3898,8 @@ wnck_task_create_widgets (WnckTask *task, GtkReliefStyle relief)
   gtk_widget_set_tooltip_text (task->button, text);
   g_free (text);
 
+  gtk_widget_set_has_tooltip (task->button, FALSE);
+
   /* Set up signals */
   if (GTK_IS_TOGGLE_BUTTON (task->button))
     g_signal_connect_object (G_OBJECT (task->button), "toggled",
-- 
2.26.2

I think we need a function in libwnck to disable the tooltips which we can call from wnck-task-list settings via a gsettings key.
Sorry, i can't do that better for myself because of my low coding skills.

tbzatek · 2020-07-29T15:44:37Z

Please don't keep in secret your findings. If you have identified memory leak in libwnck then post needed info so that it can be fixed. Thanks!

That would be a gtk+ problem, libwnck is used only to get X window ID here. However looking at gdk_pixbuf_get_from_window() sources I don't see anything obvious there either.

lukefromdc · 2020-07-29T17:24:50Z

The libwnck issue was not my finding, I learned of it from another comment on this PR or a related discussion. (edited to remove extra text from poorly functioning email reply/quote system)

raveit65 · 2020-07-29T17:48:02Z

The libwnck issue was not my finding, I learned of it from another comment on this PR or a related discussion. On 7/29/2020 at 6:27 AM, "raveit65" wrote: Builds and runs fine as of today, adding cities works same as in master, and yes we still have the leak on window thumbnails that libwnck was found responsible for.

@lukefromdc
Please do not quote you postings under my name!!!

This was your post from your aproval 12 hours ago.

Builds and runs fine as of today, adding cities works same as in master, and yes we still have the leak on window thumbnails that libwnck was found responsible for. Still, every leak we can cork up here helps, and they will add up over time

Edit: PS: Your posts becoming more and more confusing since you are using this crazy build-in quoting function.

lukefromdc · 2020-07-29T20:46:27Z

Note that I have had limited time due to the Black Lives Matter uprising in my country, in which I am a direct participant

lukefromdc · 2020-07-29T20:48:43Z

Merging as this reduces memory leaks, fixes a calendar bug, and has two approving reviews

sc0w · 2020-08-03T11:11:34Z

@mate-desktop/core-team This PR must be cherry-picked to 1.24 branch?

raveit65 · 2020-08-03T11:24:57Z

Of course, fixing memleaks are always bug fixes.

sc0w · 2020-08-03T13:22:23Z

ok, done