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 766008 - (mini)object: add flag marking "leaked" objects
(mini)object: add flag marking "leaked" objects
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal enhancement
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 765052
 
 
Reported: 2016-05-04 21:29 UTC by Guillaume Desmottes
Modified: 2016-07-13 09:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
(mini)object: add LEAKED flag (1.94 KB, patch)
2016-05-09 08:06 UTC, Guillaume Desmottes
none Details | Review
use LEAKED (mini) object flag (4.40 KB, patch)
2016-05-09 08:10 UTC, Guillaume Desmottes
none Details | Review
(mini)object: add MAY_BE_LEAKED flag (1.99 KB, patch)
2016-05-19 06:56 UTC, Guillaume Desmottes
committed Details | Review
use MAY_BE_LEAKED (mini) object flag (6.93 KB, patch)
2016-05-20 07:22 UTC, Guillaume Desmottes
reviewed Details | Review
use MAY_BE_LEAKED_FLAG (4.51 KB, patch)
2016-05-30 10:46 UTC, Guillaume Desmottes
committed Details | Review

Description Guillaume Desmottes 2016-05-04 21:29:38 UTC
As suggested on bug #765540 I tried a new approach to filter out objects which are intentionally "leaked".
I did so by introducing a new flag on Gst(Mini)Object allowing us to explicitly mark such objects in the code.
I then played with GST_TRACE in order to determine if it could be used as a proper leaks detection tool.
I'm pretty happy with the results so far! I fixed quiet a bunch of leaks in gst-validate and core. All the core tests are now passing with no leak detected.

See this branch for the code: https://git.collabora.com/cgit/user/cassidy/gstreamer/gstreamer.git/log/?h=trace-leaks

This kind of tool is not going to be a full replacements of valgrind but I think it could be a nice addition to our toolbox.
I see the following advantages in this approach:
  - Much lighter to use than valgrind, which should make it usable on embedded device with low ressources (I didn't try this yet).
  - Integrated in core so no need to build external tools which can sometime be problematic on some platform/devices.
  - Much faster than valgrind so can easily be run with "make check" to detect/prevent leak regressions.
  - Better control on what is being filtered out; valgrind suppression files can be a pretty big hammer.
  - Track only leaks in gst code so should be more robust and consistent accross different platforms or system lib versions. This is important to reduce the "noise" if we integrate this as part of our automatic QA.

I'm still not sure if GST_TRACE is the proper tool for that, but we could easily implement similar features using a GstTracer.

For the record here is the list of bugs I found during this experimentation:
https://phabricator.freedesktop.org/D958
https://phabricator.freedesktop.org/D959
https://bugzilla.gnome.org/show_bug.cgi?id=765706
https://phabricator.freedesktop.org/D979
https://phabricator.freedesktop.org/D980
https://bugzilla.gnome.org/show_bug.cgi?id=765719
https://bugzilla.gnome.org/show_bug.cgi?id=765720
https://bugzilla.gnome.org/show_bug.cgi?id=765903
https://bugzilla.gnome.org/show_bug.cgi?id=765904
https://bugzilla.gnome.org/show_bug.cgi?id=765957
https://bugzilla.gnome.org/show_bug.cgi?id=765958
https://bugzilla.gnome.org/show_bug.cgi?id=765961
https://bugzilla.gnome.org/show_bug.cgi?id=765976
https://bugzilla.gnome.org/show_bug.cgi?id=765978

So, is that something we could consider adding to core at some point?
Comment 1 Sebastian Dröge (slomo) 2016-05-04 21:39:49 UTC
Having a GstTracer for that seems nicer, than parsing free-form GST_TRACE output :) Otherwise seems very useful to have, just not sure what to do about GObjects... as we don't have control over g_object_ref/unref and can't add a tracer in there. We could however add a GstObject init/finalize hook for GstTracer
Comment 2 Guillaume Desmottes 2016-05-04 21:46:33 UTC
Yeah we can just track creation/destruction of GObject and GstMiniObject as GST_TRACE is already doing atm.
Comment 3 Sebastian Dröge (slomo) 2016-05-05 06:44:35 UTC
Also having a GstMiniObjectFlag/GstObjectFlag for objects that are certainly going to be leaked seems useful.

Can you create a separate bug about this? And also it seems like that new bug, and bug #765052 would then become "sub-bugs" of this one?
Comment 4 Guillaume Desmottes 2016-05-09 08:05:11 UTC
Ok then we can just use this bug for the new flag thing and turn bug #765052 to a tracer checking for leaks instead of my GST_TRACE hack.
Comment 5 Guillaume Desmottes 2016-05-09 08:06:58 UTC
Created attachment 327493 [details] [review]
(mini)object: add LEAKED flag
Comment 6 Guillaume Desmottes 2016-05-09 08:10:34 UTC
Created attachment 327495 [details] [review]
use LEAKED (mini) object flag
Comment 7 Tim-Philipp Müller 2016-05-13 14:28:55 UTC
Comment on attachment 327493 [details] [review]
(mini)object: add LEAKED flag

This looks fine to me, but I'd like to brainstorm/bikeshed the name a little bit more. It's not guaranteed to be leaked, so perhaps FLAG_PERSISTENT or FLAG_GLOBAL or FLAG_MAY_BE_LEAKED or something? :)

Also, please add (Since: 1.10) at the end of the new flag description line.
Comment 8 Guillaume Desmottes 2016-05-16 08:10:17 UTC
(In reply to Tim-Philipp Müller from comment #7)
> Comment on attachment 327493 [details] [review] [review]
> (mini)object: add LEAKED flag
> 
> This looks fine to me, but I'd like to brainstorm/bikeshed the name a little
> bit more. It's not guaranteed to be leaked, so perhaps FLAG_PERSISTENT or
> FLAG_GLOBAL or FLAG_MAY_BE_LEAKED or something? :)

I agree LEAKED is a bit miss-leading, any one you suggested is fine with me, just pick one and I'll use it. :)

> Also, please add (Since: 1.10) at the end of the new flag description line.

Will do.
Comment 9 Guillaume Desmottes 2016-05-16 09:46:02 UTC
(In reply to Guillaume Desmottes from comment #8)
> (In reply to Tim-Philipp Müller from comment #7)
> > Comment on attachment 327493 [details] [review] [review] [review]
> > (mini)object: add LEAKED flag
> > 
> > This looks fine to me, but I'd like to brainstorm/bikeshed the name a little
> > bit more. It's not guaranteed to be leaked, so perhaps FLAG_PERSISTENT or
> > FLAG_GLOBAL or FLAG_MAY_BE_LEAKED or something? :)

I think I'd vote for FLAG_MAY_BE_LEAKED as some may not be "global" (in the C sense of it) and it's clearer about the actual use of this flag.
Comment 10 Guillaume Desmottes 2016-05-19 06:56:47 UTC
Created attachment 328169 [details] [review]
(mini)object: add MAY_BE_LEAKED flag
Comment 11 Sebastian Dröge (slomo) 2016-05-20 06:13:20 UTC
Comment on attachment 327495 [details] [review]
use LEAKED (mini) object flag

Needs to be updated for the new name
Comment 12 Sebastian Dröge (slomo) 2016-05-20 06:13:44 UTC
Comment on attachment 328169 [details] [review]
(mini)object: add MAY_BE_LEAKED flag

Let's get this in together with the tracer
Comment 13 Sebastian Dröge (slomo) 2016-05-20 06:14:41 UTC
Comment on attachment 327495 [details] [review]
use LEAKED (mini) object flag

You might want to add GstSystemClock here, but only the singleton instance not all instances
Comment 14 Guillaume Desmottes 2016-05-20 07:17:23 UTC
(In reply to Sebastian Dröge (slomo) from comment #13)
> Comment on attachment 327495 [details] [review] [review]
> use LEAKED (mini) object flag
> 
> You might want to add GstSystemClock here, but only the singleton instance
> not all instances

Actually I don't, gst_deinit() already takes care of destroying it: https://cgit.freedesktop.org/gstreamer/gstreamer/tree/gst/gst.c#n1021
Comment 15 Guillaume Desmottes 2016-05-20 07:22:51 UTC
Created attachment 328243 [details] [review]
use MAY_BE_LEAKED (mini) object flag
Comment 16 Tim-Philipp Müller 2016-05-20 08:19:16 UTC
Comment on attachment 328243 [details] [review]
use MAY_BE_LEAKED (mini) object flag

Re. all this set_leaked_foreach() / set_leaked_on_value() / priv_gst_buffer_set_may_be_leaked() - did you have a use case where this was required in practice?

It's technically correct of course, but I would've thought it should only be needed if we have buffer fields in static caps, and I don't think one would ever do that. Do you remember where this was needed?
Comment 17 Guillaume Desmottes 2016-05-20 08:46:20 UTC
(In reply to Tim-Philipp Müller from comment #16)
> Comment on attachment 328243 [details] [review] [review]
> use MAY_BE_LEAKED (mini) object flag
> 
> Re. all this set_leaked_foreach() / set_leaked_on_value() /
> priv_gst_buffer_set_may_be_leaked() - did you have a use case where this was
> required in practice?

Yes, all this code has been added after loads of testing in order to have "make check" passing with all gst components.

> It's technically correct of course, but I would've thought it should only be
> needed if we have buffer fields in static caps, and I don't think one would
> ever do that. Do you remember where this was needed?

We have some here with the 'codec_data' key https://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/tests/check/elements/qtmux.c#n49
Comment 18 Tim-Philipp Müller 2016-05-20 09:02:17 UTC
Hrm ok. It's a bit unfortunate to add all this for something that's just needed in one or two unit tests imho.
Comment 19 Guillaume Desmottes 2016-05-20 09:08:22 UTC
I can move the code to the test itself I suppose, but that means it will just be duplicated each time we need it. I don't think it's that bad to have it in core tbh.
Comment 20 Tim-Philipp Müller 2016-05-27 17:30:20 UTC
I think the qtmux test should be changed, it's not a normal scenario really.

The code does have a small performance impact, and it also doesn't handle all theoretical or practical scenarios, so I'd prefer to just skip it for now if it's just one or two tests that need changing.
Comment 21 Guillaume Desmottes 2016-05-30 10:44:29 UTC
(In reply to Tim-Philipp Müller from comment #20)
> I think the qtmux test should be changed, it's not a normal scenario really.

Ok I'll let you take a look at this test then.
Btw, I have the same problem here: https://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/tests/check/elements/matroskamux.c#n36

(In reply to Tim-Philipp Müller from comment #20) 
> The code does have a small performance impact, and it also doesn't handle
> all theoretical or practical scenarios, so I'd prefer to just skip it for
> now if it's just one or two tests that need changing.

Ok, I'll remove it for now then.
Comment 22 Guillaume Desmottes 2016-05-30 10:46:10 UTC
Created attachment 328719 [details] [review]
use MAY_BE_LEAKED_FLAG

This helps having "make check" passing with the leaks tracer enabled.
Comment 23 Tim-Philipp Müller 2016-06-02 22:16:41 UTC
commit 4a41468ce7aa8abbf354c19c7d1bf5e8d1327048
Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Date:   Mon May 30 12:11:13 2016 +0200

    Use MAY_BE_LEAKED_FLAG
    
    This helps having "make check" passing with the leaks tracer enabled.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=766008
Comment 24 Guillaume Desmottes 2016-07-13 09:30:56 UTC
(In reply to Tim-Philipp Müller from comment #20)
> I think the qtmux test should be changed, it's not a normal scenario really.
> 
> The code does have a small performance impact, and it also doesn't handle
> all theoretical or practical scenarios, so I'd prefer to just skip it for
> now if it's just one or two tests that need changing.

For the record, I opened bug #768762 about this.