GNOME Bugzilla – Bug 682560
leak fixes
Last modified: 2013-02-03 05:46:43 UTC
Was looking at trying to land bug 666114 and ended up doing some leakfixing. You can argue whether the unix signal watch patch is (a) an API change that may break existing code, or (b) a fix to an obvious bug that no one ever reported... I went for (b). If other people think (a) then we can just change the documentation (and the test program) instead. The patch to glib/tests/regex.c is huge just because I got anal about re-aligning all the "\"s. The only real change is the switch to use g_test_add_data_func_full().
Created attachment 222245 [details] [review] gmain: free source_lists when freeing GMainContext If a context was freed with sources still attached, those sources correctly got destroyed, but the corresponding GSourceList structs were being leaked.
Created attachment 222246 [details] [review] gmain: don't leak child sources that are destroyed before their parents A parent source holds refs on its children, so if the child source is destroyed, we need to drop that ref. Fix, and reorganize to make this all more obvious.
Created attachment 222247 [details] [review] gmain: remove unix signal watch if its GSourceFunc returns FALSE g_unix_signal_watch_dispatch() was ignore the callback's return value. Fix that.
Created attachment 222248 [details] [review] g_option_context_help: don't modify the input data If there are options that need their names to be aliased, keep track of that internally rather than modifying the passed-in GOptionGroup (and leaking strings in the process).
Created attachment 222249 [details] [review] gtestutils: add g_test_add_data_func_full() Like g_test_add_data_func(), but includes a GDestroyNotify for the test data.
Created attachment 222250 [details] [review] glib/tests: fix leaks
Review of attachment 222245 [details] [review]: This is a regression introduced by 55bac5da0ada8f46824a4d565cdd8ea7e3774a47 right? Hm. How is freeing a context with sources still attached possible? I'd have thought the source would hold a ref on the context. But looking at the code, that's not the case. Ah, I see. Ok.
Review of attachment 222246 [details] [review]: I think this makes sense...
Review of attachment 222247 [details] [review]: Per IRC discussion, this looks safe.
Review of attachment 222248 [details] [review]: This is a pretty old leak...it dates to: commit 2effb8d0eaf8041615dda14e4cc82ed764255859 Author: Matthias Clasen <mclasen@redhat.com> Date: Fri Sep 3 21:20:07 2010 -0400 Remove excessive header includes Which seems to have nothing to do with headers...I'm leaving this one for Matthias to look at.
Review of attachment 222249 [details] [review]: A new test API, but no tests using it?
Review of attachment 222249 [details] [review]: Nevermind, I see that's in the next patch.
Review of attachment 222250 [details] [review]: All looks right to me.
Review of attachment 222248 [details] [review]: Looks right, though it took a while to spot the actual leak among all the changes. Might be nicer to do this as two changes: - first plug the leak (by freeing the old entry->long_name before overwriting it) - then introduce the aliases hash table to avoid modifying the entry
(In reply to comment #14) > Might be nicer to do this as two changes: > - first plug the leak (by freeing the old entry->long_name before overwriting > it) > - then introduce the aliases hash table to avoid modifying the entry No, the leaked memory is the *new* entry->long_name value; the old value is (almost certainly) a compile-time constant. So to avoid leaking, we have to free the new value before we return, meaning we need some data structure to keep track of all the new strings we created. And then, if we want the entry to still be valid when we return, then it has to still/again have the old long_name value when we return, since the new value will have been freed. So there's no way to split this into two pieces.
Created attachment 222529 [details] [review] gobject/tests: use g_test_expect_messages()
Created attachment 222530 [details] [review] gobject/tests: plug leaks
Created attachment 222531 [details] [review] xdgmime: plug a small leak
Created attachment 222532 [details] [review] GDesktopAppInfo: fix leaks g_desktop_app_info_ensure_saved() was leaking the file contents. _g_desktop_app_info_launch_uris_internal() was leaking the session bus on error.
Created attachment 222533 [details] [review] g_file_copy: plug a leak The fallback copy code was leaking the GFileInfo if it didn't have G_FILE_ATTRIBUTE_STANDARD_TYPE.
Created attachment 222534 [details] [review] gio/tests: port some stuff to g_test_expect_message()
Created attachment 222535 [details] [review] gio/tests/cancellable: fix to still work when running slowly The test was assuming that all cancelled ops would finish within a certain amount of time, but this often failed under valgrind. Instead, just run the loop until all of the ops have actually finished.
Created attachment 222536 [details] [review] gio/tests: fix leaks
Review of attachment 222529 [details] [review]: ok
Review of attachment 222530 [details] [review]: ok
Review of attachment 222531 [details] [review]: ok
Review of attachment 222532 [details] [review]: ok
Review of attachment 222533 [details] [review]: ok
Review of attachment 222534 [details] [review]: sure
Review of attachment 222535 [details] [review]: sure
Review of attachment 222536 [details] [review]: ok
Attachment 222529 [details] pushed as e0cba35 - gobject/tests: use g_test_expect_messages() Attachment 222530 [details] pushed as 03be681 - gobject/tests: plug leaks Attachment 222531 [details] pushed as 4e7031f - xdgmime: plug a small leak Attachment 222532 [details] pushed as fa58cef - GDesktopAppInfo: fix leaks Attachment 222533 [details] pushed as 039ecf2 - g_file_copy: plug a leak Attachment 222534 [details] pushed as 568f737 - gio/tests: port some stuff to g_test_expect_message() Attachment 222535 [details] pushed as 17bb9d5 - gio/tests/cancellable: fix to still work when running slowly Attachment 222536 [details] pushed as beb0f9c - gio/tests: fix leaks
g_option_context_help() patch still uncommitted (comment 4, comment 14, comment 15).
(In reply to comment #35) > Attachment 222531 [details] pushed as 4e7031f - xdgmime: plug a small leak This one causes a double-free during gnome boxes startup, reverting it gets things back in order. xdg_dir_time_list_add takes ownership of file_name, I guess it's sometimes freeing it on its own.
I reported the regression as bug #683270
Review of attachment 222248 [details] [review]: .
Attachment 222248 [details] pushed as 39a528b - g_option_context_help: don't modify the input data