GNOME Bugzilla – Bug 711751
Fix memory leaks in libglib tests
Last modified: 2013-12-16 00:30:20 UTC
In order to make libglib tests be valgrindable, I'll post a bunch of patches which fix memory leaks there. In general it's good practice to free stuff in tests, so that we can run through those code paths, and make sure we received appropriate references/memory.
Created attachment 259366 [details] [review] private: Fix memory leak in tests Don't use g_private_new(), it's deprecated, and leaks by definition.
Created attachment 259369 [details] [review] gerror: Don't leak memory when overwrite warning Even though we can't always make no-leak guarantees when g_warning() in this case we're testing this behavior in tests, and it would be good to be able to valgrind this.
Created attachment 259371 [details] [review] unix: Fix memory leak in unix test
Created attachment 259373 [details] [review] asyncqueue-test: Fix leaks in tests
Created attachment 259374 [details] [review] child-test: Fix leak in test
Created attachment 259375 [details] [review] completion-test: Fix leaks in tests
Created attachment 259376 [details] [review] gio-test: Fix leaks in tests
Created attachment 259377 [details] [review] mainloop-test: Fix uninitialized memory access in tests
Created attachment 259378 [details] [review] mapping-test: Fix leaks in tests
Created attachment 259380 [details] [review] sources: Fix leaks in tests
Created attachment 259381 [details] [review] datetime: Fix leak in test
Created attachment 259382 [details] [review] file-test: Fix leaks in test
Created attachment 259383 [details] [review] mainloop-test: Fix leaks in tests
Created attachment 259384 [details] [review] thread-test: Fix leaks in tests This is a test of deprecated functionality and its age is showing. Doesn't actually do what it says. But fix leaks anyway.
Created attachment 259385 [details] [review] timeloop: Fix leaks in tests
Created attachment 259386 [details] [review] unicode-encoding: Fix leaks in test
Created attachment 259387 [details] [review] threadpool-test: Fix leaks in tests
Created attachment 259410 [details] [review] iochannel-test: Fix leaks in test
Created attachment 259411 [details] [review] threadpool-test: Fix leaks in tests
Review of attachment 259411 [details] [review]: ::: tests/threadpool-test.c @@ +247,3 @@ +{ + g_thread_pool_free (pool, FALSE, TRUE); +} I'm confused, this function appears to be unused in your patch?
Review of attachment 259366 [details] [review]: Looks right.
Review of attachment 259369 [details] [review]: Ok.
Review of attachment 259371 [details] [review]: Yep.
Review of attachment 259373 [details] [review]: At some point we should do a mass-porting away from GMainLoop to the new while () { GMainContext.iteration(); } pattern. Doesn't make sense to conflate that with these fixes, just noticing leaking GMainLoop is quite common.
Review of attachment 259374 [details] [review]: Yep.
Review of attachment 259375 [details] [review]: Looks correct.
Review of attachment 259376 [details] [review]: Holy cow this is some ugly code...not your fault =) Anyways looks right.
Review of attachment 259377 [details] [review]: ::: tests/mainloop-test.c @@ +221,2 @@ sprintf (buf1, "%d", a); sprintf (buf2, "%d", b); Are you sure this hunk is necessary?
Review of attachment 259378 [details] [review]: g_free() all the things!
Review of attachment 259380 [details] [review]: ::: tests/sources.c @@ +107,3 @@ + { + g_source_destroy (sources[i]); + g_source_unref (sources[i]); Are you sure you're not double-unreffing here? We're transferring ownership of the source to the worker thread, which per your above hunk will attach then call _unref(), so now the context owns one ref.
Review of attachment 259381 [details] [review]: Yep.
Review of attachment 259382 [details] [review]: ::: tests/file-test.c @@ +185,3 @@ g_assert (data == NULL && "could read link3"); g_assert_error (error, G_FILE_ERROR, G_FILE_ERROR_NOENT); + g_error_free (error); You could use g_clear_error () and also remove the subsequent line. Personally in my code, I much prefer g_clear_error() since it's much easier to confidently later add code.
Review of attachment 259383 [details] [review]: Ok.
Review of attachment 259384 [details] [review]: I'm not going to dive into this one...I'll trust you on it.
Review of attachment 259385 [details] [review]: ::: tests/timeloop.c @@ +154,3 @@ input_callback, in_channels[1]); + g_io_channel_unref (in_channels[0]); + g_io_channel_unref (out_channels[1]); This is a really minor comment, but wouldn't it make more sense to have the _unref() calls next to the corresponding close() calls? Really there should be g_io_channel_close_and_unref () or something. But anyways, feel free to ignore this too.
Review of attachment 259386 [details] [review]: Looks right.
Review of attachment 259410 [details] [review]: Ok.
Thanks for the reviews. Merging these. Will answer and rework others later... Attachment 259366 [details] pushed as e74b435 - private: Fix memory leak in tests Attachment 259369 [details] pushed as ab3c554 - gerror: Don't leak memory when overwrite warning Attachment 259371 [details] pushed as e525586 - unix: Fix memory leak in unix test Attachment 259373 [details] pushed as 9a67fb9 - asyncqueue-test: Fix leaks in tests Attachment 259374 [details] pushed as ba56c7b - child-test: Fix leak in test Attachment 259375 [details] pushed as ee74367 - completion-test: Fix leaks in tests Attachment 259376 [details] pushed as fc4630b - gio-test: Fix leaks in tests Attachment 259378 [details] pushed as 5ae5d43 - mapping-test: Fix leaks in tests Attachment 259381 [details] pushed as 83a14d1 - datetime: Fix leak in test Attachment 259383 [details] pushed as ae1764b - mainloop-test: Fix leaks in tests Attachment 259384 [details] pushed as e8cc096 - thread-test: Fix leaks in tests Attachment 259386 [details] pushed as 44bd2ab - unicode-encoding: Fix leaks in test Attachment 259410 [details] pushed as 0a02fd9 - iochannel-test: Fix leaks in test
Review of attachment 259411 [details] [review]: ::: tests/threadpool-test.c @@ +247,3 @@ +{ + g_thread_pool_free (pool, FALSE, TRUE); +} Indeed. Removed
Created attachment 259525 [details] [review] threadpool-test: Fix leaks in tests
Review of attachment 259377 [details] [review]: ::: tests/mainloop-test.c @@ +221,2 @@ sprintf (buf1, "%d", a); sprintf (buf2, "%d", b); Yes, because the entire buffer is written.
Review of attachment 259380 [details] [review]: ::: tests/sources.c @@ +107,3 @@ + { + g_source_destroy (sources[i]); + g_source_unref (sources[i]); Hmmm the test is confusing, but both of these g_source_unref() hunks happen before threads get involved. The threading stuff happens below.
Review of attachment 259382 [details] [review]: ::: tests/file-test.c @@ +185,3 @@ g_assert (data == NULL && "could read link3"); g_assert_error (error, G_FILE_ERROR, G_FILE_ERROR_NOENT); + g_error_free (error); Indeed. But that's the style of the rest of the test, so I just went with the flow.
Comment on attachment 259382 [details] [review] file-test: Fix leaks in test Attachment 259382 [details] pushed as 9e0ade0 - file-test: Fix leaks in test
(In reply to comment #35) > This is a really minor comment, but wouldn't it make more sense to have the > _unref() calls next to the corresponding close() calls? Really there should be > g_io_channel_close_and_unref () or something. But anyways, feel free to ignore > this too. Done and merged.
Comment on attachment 259385 [details] [review] timeloop: Fix leaks in tests Attachment 259385 [details] pushed as 177fe9f - timeloop: Fix leaks in tests
Review of attachment 259377 [details] [review]: Oh I see. Looks right then, thanks!
Review of attachment 259525 [details] [review]: Right.
Attachment 259377 [details] pushed as 3e041ce - mainloop-test: Fix uninitialized memory access in tests Attachment 259525 [details] pushed as d10f353 - threadpool-test: Fix leaks in tests
Attachment 259380 [details] pushed as 3f8888d - sources: Fix leaks in tests