After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 711779 - Fix memory leaks in libgobject tests
Fix memory leaks in libgobject tests
Status: RESOLVED OBSOLETE
Product: glib
Classification: Platform
Component: gobject
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks: 711778
 
 
Reported: 2013-11-10 14:49 UTC by Stef Walter
Modified: 2018-05-24 15:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
enums: Fix leaks in tests (868 bytes, patch)
2013-11-10 15:13 UTC, Stef Walter
committed Details | Review
threadtests: Fix leaks in tests (1.79 KB, patch)
2013-11-10 15:13 UTC, Stef Walter
committed Details | Review
param: Avoid g_test_add_data_func_full() which leaks (1.16 KB, patch)
2013-11-10 15:14 UTC, Stef Walter
reviewed Details | Review
accumulator: Fix leak in test (653 bytes, patch)
2013-11-10 15:14 UTC, Stef Walter
committed Details | Review
gvalue-test: Fix leaks in test (905 bytes, patch)
2013-11-10 15:14 UTC, Stef Walter
committed Details | Review
override: Fix leaks in test (781 bytes, patch)
2013-11-10 15:14 UTC, Stef Walter
committed Details | Review
paramspec-test: Fix leaks in tests (989 bytes, patch)
2013-11-10 15:14 UTC, Stef Walter
committed Details | Review
signals: Fix leak in test (620 bytes, patch)
2013-11-10 15:14 UTC, Stef Walter
committed Details | Review
timeloop-closure: Fix leaks in test (1.65 KB, patch)
2013-11-10 15:14 UTC, Stef Walter
committed Details | Review
defaultiface: Fix leak in test (1006 bytes, patch)
2013-11-10 15:14 UTC, Stef Walter
none Details | Review
dynamictype: Fix leak in test (966 bytes, patch)
2013-11-10 15:14 UTC, Stef Walter
accepted-commit_now Details | Review

Description Stef Walter 2013-11-10 14:49:44 UTC
In order to make libgobject tests be valgrindable, I'll post patches
which fix memory leaks.

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.
Comment 1 Stef Walter 2013-11-10 15:13:04 UTC
Created attachment 259434 [details] [review]
enums: Fix leaks in tests
Comment 2 Stef Walter 2013-11-10 15:13:09 UTC
Created attachment 259435 [details] [review]
threadtests: Fix leaks in tests
Comment 3 Stef Walter 2013-11-10 15:14:16 UTC
Created attachment 259437 [details] [review]
param: Avoid g_test_add_data_func_full() which leaks

It leaks on tests that have not been run. This is the case when
the g_test_subprocess mechanics are in use.
Comment 4 Stef Walter 2013-11-10 15:14:20 UTC
Created attachment 259438 [details] [review]
accumulator: Fix leak in test
Comment 5 Stef Walter 2013-11-10 15:14:24 UTC
Created attachment 259439 [details] [review]
gvalue-test: Fix leaks in test
Comment 6 Stef Walter 2013-11-10 15:14:29 UTC
Created attachment 259440 [details] [review]
override: Fix leaks in test
Comment 7 Stef Walter 2013-11-10 15:14:33 UTC
Created attachment 259441 [details] [review]
paramspec-test: Fix leaks in tests
Comment 8 Stef Walter 2013-11-10 15:14:37 UTC
Created attachment 259442 [details] [review]
signals: Fix leak in test
Comment 9 Stef Walter 2013-11-10 15:14:42 UTC
Created attachment 259443 [details] [review]
timeloop-closure: Fix leaks in test
Comment 10 Stef Walter 2013-11-10 15:14:46 UTC
Created attachment 259444 [details] [review]
defaultiface: Fix leak in test
Comment 11 Stef Walter 2013-11-10 15:14:51 UTC
Created attachment 259445 [details] [review]
dynamictype: Fix leak in test
Comment 12 Matthias Clasen 2013-11-11 01:46:04 UTC
Review of attachment 259434 [details] [review]:

sure
Comment 13 Matthias Clasen 2013-11-11 01:48:03 UTC
Review of attachment 259435 [details] [review]:

ok
Comment 14 Matthias Clasen 2013-11-11 01:50:37 UTC
Review of attachment 259437 [details] [review]:

ok
Comment 15 Matthias Clasen 2013-11-11 01:51:47 UTC
Review of attachment 259438 [details] [review]:

ok
Comment 16 Matthias Clasen 2013-11-11 01:55:23 UTC
Review of attachment 259439 [details] [review]:

ok
Comment 17 Matthias Clasen 2013-11-11 01:56:59 UTC
Review of attachment 259440 [details] [review]:

ok
Comment 18 Matthias Clasen 2013-11-11 02:04:03 UTC
Review of attachment 259441 [details] [review]:

sure
Comment 19 Matthias Clasen 2013-11-11 02:04:43 UTC
Review of attachment 259442 [details] [review]:

ok
Comment 20 Matthias Clasen 2013-11-11 02:06:36 UTC
Review of attachment 259437 [details] [review]:

This is somewhat unusual - it is the only patch where you had to use cleanup facilities to fix a leak.
Are we missing some api here ?
Comment 21 Matthias Clasen 2013-11-11 02:06:55 UTC
Review of attachment 259437 [details] [review]:

.
Comment 22 Matthias Clasen 2013-11-11 02:09:53 UTC
Review of attachment 259443 [details] [review]:

ok
Comment 23 Matthias Clasen 2013-11-11 02:10:21 UTC
Review of attachment 259444 [details] [review]:

yes
Comment 24 Matthias Clasen 2013-11-11 02:10:37 UTC
Review of attachment 259445 [details] [review]:

sure
Comment 25 Stef Walter 2013-11-11 06:38:02 UTC
Thanks. First pass pushing patches that don't need changes.
Attachment 259434 [details] pushed as ac6d35b - enums: Fix leaks in tests
Attachment 259435 [details] pushed as 5339950 - threadtests: Fix leaks in tests
Attachment 259438 [details] pushed as 83301d8 - accumulator: Fix leak in test
Attachment 259439 [details] pushed as e6de9c6 - gvalue-test: Fix leaks in test
Attachment 259440 [details] pushed as 320f0b3 - override: Fix leaks in test
Attachment 259441 [details] pushed as 1b96620 - paramspec-test: Fix leaks in tests
Attachment 259442 [details] pushed as d872244 - signals: Fix leak in test
Attachment 259443 [details] pushed as bac4179 - timeloop-closure: Fix leaks in test
Attachment 259444 [details] pushed as fd7b2fa - defaultiface: Fix leak in test
Comment 26 Stef Walter 2013-11-11 07:30:04 UTC
(In reply to comment #24)
> Review of attachment 259445 [details] [review]:
> 
> sure

This is actually dependent on a patch in bug #711778. Moved there.

(In reply to comment #20)
> Review of attachment 259437 [details] [review]:
> 
> This is somewhat unusual - it is the only patch where you had to use cleanup
> facilities to fix a leak.
> Are we missing some api here ?

Indeed. First of all this patch is on the wrong bug. Moved to bug #711778.

But yes, g_test_add_data_func_full() uses the teardown functionality to free the fixture data. teardown() isn't run if the test isn't run. Therefore this will always leak. Personally, I don't think using teardown() for freeing fixture data is completely correct. Maybe Dan can shed more light here, was introduced here:

   commit 0c0cdfd9c4a9d57aae0fb50b5e18cab6ba9e1a76
   Author: Dan Winship <danw@gnome.org>
   Date:   Thu Aug 23 12:29:36 2012 -0400

      gtestutils: add g_test_add_data_func_full()

Note that in the above I refer to Fixture as the static input data to a text. In gtestutils.c and documentation we confuse the modifiable test case structure with the actual const data which is input to the test. This latter is what other unit test suites call a 'fixture'.
Comment 27 Stef Walter 2013-11-11 07:31:45 UTC
Leaving this bug open to receive more test case leak patches. But right now both glib/tests/ and tests/gobject/ run without leaks (given the gcleanup stuff, of course).
Comment 28 Stef Walter 2013-11-11 16:08:58 UTC
Comment on attachment 259444 [details] [review]
defaultiface: Fix leak in test

This depends on GTypeModule cleanup, so reverted, and moving to that bug.
Comment 29 GNOME Infrastructure Team 2018-05-24 15:55:32 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/783.