GNOME Bugzilla – Bug 709243
Changing backgrounds leaks as much as 50MB of memory
Last modified: 2015-10-27 12:37:46 UTC
Using gnome-control-center to change backgrounds causes anywhere from 20MB to 50MB of RAM to be consumed by gnome-control-center. Simply open up Settings, open up a system monitor in the background, keep an eye on gnome-control-center, and change the background several times (I used the backgrounds supplied by gnome). You should notice the RAM usage skyrocket. Build date was Tue 24 Sep 2013 07:24:24 AM PDT, distributed by Arch Linux, v3.10.0-1
Created attachment 256273 [details] [review] background: Fix memory leak in the picture sources The list store was taking a reference to the background item, so we need to unref it on our side to avoid leaking it.
Created attachment 256274 [details] [review] background: Fix leak of the preview picture
Created attachment 256275 [details] [review] background: Fix leak of the Flickr source The Flickr source was never disposed of when destroying the selection dialog.
Created attachment 256276 [details] [review] background: Add test program for the selection dialog
Created attachment 256277 [details] [review] background: Fix memory leak in Pictures source The enumerator was never unref'ed.
Created attachment 256278 [details] [review] background: Fix memory leak in background item object Some fields were never freed.
Created attachment 256279 [details] [review] background: Fix memory leaks in slideshow handling A string was leaked when loading the name of the slideshow, and a file object was left lingering as well.
Created attachment 256280 [details] [review] background: Fix a memory leak when monitoring directories Destroy directory monitors on when the XML object is destroyed.
Review of attachment 256273 [details] [review]: ::: panels/background/bg-wallpapers-source.c @@ +119,3 @@ + g_clear_object (&pixbuf); + g_object_unref (item); I think we shouldn't be releasing the reference to the item in the BgWallpapersSource. Instead CcBackgroundXml should be managing the lifetime of item when it emits the "added" signal. eg., idle_emit in CcBackgroundXml already unrefs the item after emitting the signal. Looking at cc_background_xml_load_xml_internal (the other place where the signal is emitted) I think the item should be unrefed once the signal has been emitted.
Review of attachment 256274 [details] [review]: Looks good to me.
Review of attachment 256273 [details] [review]: Looks good to me.
Review of attachment 256275 [details] [review]: Looks good to me.
Comment on attachment 256273 [details] [review] background: Fix memory leak in the picture sources Put it back to needs-work, because I am really not sure about it.
Review of attachment 256276 [details] [review]: ::: panels/background/Makefile.am @@ +22,3 @@ gdesktop-enums-types.h +noinst_LTLIBRARIES = libbackground.la libbackground-chooser.la Why did you move the noinst_LTLIBRARIES? If you kept it where it was, the patch would be easier to read. ::: panels/background/test-chooser-dialog.c @@ +12,3 @@ +} + +int main (int argc, char **argv) Return type should be on a separate line.
Review of attachment 256277 [details] [review]: Looks good to me.
Review of attachment 256278 [details] [review]: Looks good to me.
Created attachment 256292 [details] [review] background: Fix memory leak in the picture sources The list store was taking a reference to the background item, so we need to unref it on our side to avoid leaking it.
Created attachment 256293 [details] [review] background: Fix memory leak in XML signal handling When emitted in an idle, the item was correctly unref'ed, but not when emitting the signal straight away.
Review of attachment 256279 [details] [review]: Looks good to me.
Review of attachment 256280 [details] [review]: ::: panels/background/cc-background-xml.c @@ +370,3 @@ + g_signal_connect_object (monitor, "changed", + G_CALLBACK (gnome_wp_file_changed), + data, G_CONNECT_SWAPPED); 1) Using G_CONNECT_SWAPPED is wrong because it does not match with the signature of gnome_wp_file_changed. 2) The monitor is still leaked.
Review of attachment 256292 [details] [review]: Looks good to me.
Created attachment 256297 [details] [review] background: Fix memory leak in XML signal handling When emitted in an idle, the item was correctly unref'ed, but not when emitting the signal straight away.
Review of attachment 256293 [details] [review]: ::: panels/background/cc-background-xml.c @@ +318,2 @@ g_signal_emit (G_OBJECT (xml), signals[ADDED], 0, item); + g_object_unref (item); It should be unreffed in both cases.
Review of attachment 256297 [details] [review]: Looks good to me.
FWIW, the major leak was a GTK+ bug, see the blocker bugs. It was leaking the original image for each of the backgrounds.
Pushed all those to gnome-3-8 and master. Attachment 256274 [details] pushed as fc4be32 - background: Fix leak of the preview picture Attachment 256275 [details] pushed as 42f2f43 - background: Fix leak of the Flickr source Attachment 256276 [details] pushed as fba69ce - background: Add test program for the selection dialog Attachment 256277 [details] pushed as c9c28e0 - background: Fix memory leak in Pictures source Attachment 256278 [details] pushed as 4102ad2 - background: Fix memory leak in background item object Attachment 256279 [details] pushed as 756b9e7 - background: Fix memory leaks in slideshow handling Attachment 256292 [details] pushed as 6094ea6 - background: Fix memory leak in the picture sources Attachment 256297 [details] pushed as 86e61a4 - background: Fix memory leak in XML signal handling
Created attachment 256303 [details] [review] background: Fix a memory leak when monitoring directories Destroy directory monitors on when the XML object is destroyed.
(In reply to comment #14) > Review of attachment 256276 [details] [review]: > > ::: panels/background/Makefile.am > @@ +22,3 @@ > gdesktop-enums-types.h > > +noinst_LTLIBRARIES = libbackground.la libbackground-chooser.la > > Why did you move the noinst_LTLIBRARIES? If you kept it where it was, the patch > would be easier to read. > > ::: panels/background/test-chooser-dialog.c > @@ +12,3 @@ > +} > + > +int main (int argc, char **argv) > > Return type should be on a separate line. I was expecting you to clean this up before pushing. :-(
Review of attachment 256303 [details] [review]: Looks good to me.
Created attachment 256304 [details] [review] background: Fix a memory leak when monitoring directories Destroy directory monitors on when the XML object is destroyed.
Attachment 256304 [details] pushed as 732f864 - background: Fix a memory leak when monitoring directories
*** Bug 685438 has been marked as a duplicate of this bug. ***