GNOME Bugzilla – Bug 672640
[patch] memory leaks
Last modified: 2012-05-02 18:05:40 UTC
Hi, this is my first attempt to contribute to gnome, please don't bite :-) from valgrind log: ==31043== 7 bytes in 1 blocks are definitely lost in loss record 213 of 6,861 ==31043== at 0x402B018: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so) ==31043== by 0x417789A: ??? (in /usr/lib/libglib-2.0.so.0.3122.0) ==31043== by 0x4177C42: g_malloc (in /usr/lib/libglib-2.0.so.0.3122.0) ==31043== by 0x418DC3A: g_strdup (in /usr/lib/libglib-2.0.so.0.3122.0) ==31043== by 0x408C470: meta_display_open (display.c:475) ==31043== by 0x40A4D42: meta_run (main.c:552) ==31043== by 0x8048A74: main (mutter.c:96) 1. hostname seems never freed 2. if gethostname() fails hostname will be uninitialized patch attached
Created attachment 210370 [details] [review] proposed patch
Created attachment 210417 [details] [review] patch2 And a few more fixes for memory leaks...
Created attachment 210419 [details] [review] patch3
Created attachment 210420 [details] [review] patch4
Created attachment 210422 [details] [review] patch5
Created attachment 210423 [details] [review] patch6
Review of attachment 210417 [details] [review]: Ups, yes thanks for finding this. This needs a freeze break and I think it's worth of it since this function is called often. Pavel, will you request the freeze break or do you want me to do it for you? Also, do you have commit access to git.gnome.org?
Rui Matos, this is my first contribute to gnome, so I have no access to git and I don't know requires it freeze break or not or something... Please take care about this patches :-)
Review of attachment 210417 [details] [review]: Thanks for the patch - I do think we need a freeze break for this. However, the patch needs some cosmetic improvement - in particular, it needs an informative subject ike "meta_stack_list_window(): Fix leaked GList" I don't think including the valgrind log adds any information for someone looking at this commit in a week or a two years - it would be much more useful in the bug report than in the commit message
Review of attachment 210420 [details] [review]: Not correct - I don't really care if we clean up properly at shutdown time - it's a lot of work for zero gain for the user, but if we do try to clean up, we can't just g_free() some block of memory allocated by another piece of code , which could have pointers in it, etc.
Review of attachment 210422 [details] [review]: Needs Subject/Body imrovements, but otherwise looks good
Review of attachment 210423 [details] [review]: Good catch! needs Subject/Body improvements, but we should include this one in the freeze break request as well. Would also be good to get this into Metacity
Created attachment 210442 [details] [review] patch2
Created attachment 210443 [details] [review] patch5
Created attachment 210444 [details] [review] patch6 Would you like to I open bug against metacity for this patch?
(In reply to comment #15) > Created an attachment (id=210444) [details] [review] > patch6 [details] > > Would you like to I open bug against metacity for this patch? Don't think that's necessary - we can just land the patch in Metacity as well - my guess is that it will apply close to unmodified.
Comment on attachment 210420 [details] [review] patch4 Oh... it really requires non-trivial changes
Review of attachment 210419 [details] [review]: ::: src/core/screen.c @@ +1024,3 @@ + while (screen->workspaces) + meta_workspace_remove(screen->workspaces->data); + This doesn't work because of the assertion in meta_workspace_remove(). Also, freeing MetaScreens isn't something we do unless the process is exiting AFAIK, so it's not terribly important.
Review of attachment 210442 [details] [review]: Looks good now, thanks. I've requested a freeze break for this one.
Review of attachment 210444 [details] [review]: Nit in the summary: it's not a GList but a GSList. We can just amend it when before pushing. Looks good otherwise.
(In reply to comment #18) > This doesn't work because of the assertion in meta_workspace_remove(). What if move freeing memory from meta_workspace_remove to finalizer and on closing MetaScreen just unref MetaWorkspaces, how do you think?
The two patches with "accepted-commit_now" status have received approvals from the release-team - please commit these two.
The approved patches have been commited now. Thanks Pavel!
(In reply to comment #16) > (In reply to comment #15) > > Created an attachment (id=210444) [details] [review] [details] [review] > > patch6 [details] [details] > > > > Would you like to I open bug against metacity for this patch? > > Don't think that's necessary - we can just land the patch in Metacity as well - > my guess is that it will apply close to unmodified. I pushed this one into metacity.
Review of attachment 210443 [details] [review]: I'm not sure this is a memory leak (the stack tracker never gets freed unless the screen gets freed, which means you're shutting down mutter). It is a correction to meta_stack_tracker_free, though.
Review of attachment 210370 [details] [review]: Sure as well.