GNOME Bugzilla – Bug 743062
GstObject subclasses sinking instances in instance_init/constructor cause leaks in bindings
Last modified: 2018-05-09 08:28:36 UTC
Various GstObject subclasses leak in bindings (PyGObject, Gjs and Vala) when created with g_object_new() because they call ref_sink() in "_init" or "_constructor". The problem is that bindings assume for GInitiallyUnowned subclasses that they get created floating, and if they happen to be not floating something has already sunk them and taken ownership. There is no way for bindings to differentiate between g_object_new(GTK_TYPE_WINDOW, ...) and g_object_new(GST_TYPE_BUS, ...). They both return a non-floating object and both inherit from GInitiallyUnowned while in case of GTK_TYPE_WINDOW no reference is passed to the caller, and in case of GST_TYPE_BUS there is. The following (introspected) classes are affected: Gst.Bus Gst.Task Gst.TaskPool Gst.Clock GstCheck.TestClock Gst.SystemClock GstNet.NetClientClock GstAudio.AudioClock GstBase.CollectPads Gst.ControlSource GstController.LFOControlSource GstController.TimedValueControlSource GstController.InterpolationControlSource GstController.TriggerControlSource The non-abstract classes define an additional constructor which are correctly marked "transfer full". Possible solutions: 1) Remove the ref_sink() in all cases and mark custom constructors "transfer floating". 2) Remove the ref_sink in all cases and add a ref_sink() to the constructors, keeping the "transfer full" there. 3) Change nothing and advice binding users to use the class constructors and set properties after creation. (Gst.Bus.new() instead of Gst.Bus() in Python) I can provide a patch if needed.
You realise we have added that because of g-i / bindings, not because we thought it was a good thing to do? I think perhaps instead of randomly doing stuff depending on the g-i issue du jour, perhaps we should wait for the g-i / bindings people to write up some comprehensive design documentation that covers all the use cases we need (and doesn't require us to change ABI), and documents what bindings should do as well. So there's something authoritative to go on.
(In reply to comment #1) > You realise we have added that because of g-i / bindings, not because we > thought it was a good thing to do? Sure. I'm just trying to analyze the current situation as detailed as possible and find out what could be changed and with what effect. > I think perhaps instead of randomly doing stuff depending on the g-i issue du > jour, perhaps we should wait for the g-i / bindings people to write up some > comprehensive design documentation that covers all the use cases we need (and > doesn't require us to change ABI), and documents what bindings should do as > well. So there's something authoritative to go on. Imho you first need to know the use cases/problems, and this is one of them. This might all be common knowledge for GStreamer developers but I couldn't find anything about it.
Not sure what to do with this. Seems like a bindings issue to me. One and a half years later, are we any wiser? :)
At least for Python we could work around the leak in gst-python overrides, by manually unrefing after creation: class Bus(Gst.Bus): def __init__(self, *args, **kwargs): super(Bus, self).__init__(*args, **kwargs) self.unref() If you'd rather close this bug I can move it to pygobject.
It doesn't seem right to me to work around bugs in gst-python overrides. Also, I think many people are using the python bindings without gst-python. I don't mind keeping it open, it's just that I don't see anything actionable from our side.
Created attachment 330071 [details] [review] Change GInitiallyUnowned subclasses to create a floating reference OK, here is the in my opinion least invasive fix. This is proposal (2) from the initial post. ------ Creating them through g_object_new with bindings leaked as they returned a sunk reference, which bindings interpret as them being owned/sunk by some other object and thus having to ref it again, leading to two initial references. This changes the subclasses which did a ref_sink to always return floating refs and sink them in the cunstructor functions (gst_foo_new(), ..) instead. This results in floating refs in case these classes are created with g_object_new, but since they are afaics not used with functions dealing with floating refs, there should be no difference.
Right, but this just ping-pongs something we did because of some bindings in the first place. So the issue remains, as far as I can tell: someone from the g-i side needs to formulate and document a clear policy on how bindings have to handle these cases, so we can clearly know what to do here in GStreamer because we can rely on bindings having to behave in a certain way.
> Right, but this just ping-pongs something we did because of some bindings in the first place. Do you remember by any chance which bindings and/or which issues? > So the issue remains, as far as I can tell: someone from the g-i side needs to formulate and document a clear policy on how bindings have to handle these cases, so we can clearly know what to do here in GStreamer because we can rely on bindings having to behave in a certain way. OK, I'll investigate and ask gir-devel-list
What I suggest here is that we sink these in the explicit constructors (the new() functions), and mark those constructors as "transfer full" (as they give a non-floating, full reference to the caller). The new() functions of other classes (that return floating references), should be "transfer floating" as they are now in a few places already. While "transfer floating" is not perfectly defined, it's the best we have right now and it maps to "transfer none" for bindings (which causes them to do the right thing). (Note: The GObject documentation says bindings *should* not use floating references and should not expose them, i.e. get rid of the floating references wherever encountered. As such "transfer none/floating" is causing the correct behaviour for functions returning floating references, and ref-sinking floating references as arguments)
The main problem here is that this will "break" every subclass of GstClock and GstControlSource and GstTaskPool (and GstTask, GstBus, but who does that?). The constructors of those subclasses would now have to unset the floating flag instead, requiring some code to be added to every of these subclasses. However the impact of this should be rather minimal, even if they don't get the new ref_sink() added: 1) For C code this distinction rarely matters. All objects apart from control sources never have a parent (they are not "added" to anything), and while control sources are "added" to a control bindings, the bindings take a new reference anyway so the control sources always stay floating. Only if someone manually ref_sinks the control sources, there would be a problem. 2) For bindings it is broken anyway, see this bug
Created attachment 351883 [details] [review] gst: Don't ref_sink() GstObject subclasses in instance_init/constructor This is something bindings can't handle and it causes leaks. Instead move the ref_sink() to the explicit, new() constructors. This means that abstract classes, and anything that can have subclasses, will have to do ref_sink() in their new() function now. Specifically this affects GstClock and GstControlSource.
Created attachment 351884 [details] [review] allocator: ref_sink() the global sysmem allocator after creation It's not owned by the first one to ask for it, but by this very code.
Created attachment 351886 [details] [review] audioclock: Sink the reference in the constructor This is now needed as GstClock does not do that internally anymore, because that broke bindings. And mark the function correctly as (transfer full), which it already was before.
Created attachment 351887 [details] [review] 1394: Sink the clock reference in the constructor This is now needed as GstClock does not do that internally anymore, because that broke bindings.
Created attachment 351888 [details] [review] decklink: Sink the clock reference in the constructor This is now needed as GstClock does not do that internally anymore, because that broke bindings.
These ref_sink() were all added because bindings people asked for it btw, relevant commit is: commit c0c79188ca5d9f0a2d13ab3940a7e4977410cdca Author: Tim-Philipp Müller <tim@centricular.net> Date: Tue Jul 3 00:07:11 2012 +0100 bus, clock: make sure these never have a floating ref Clear the initial floating ref in the init function for busses and clocks. These objects can be set on multiple elements, so there's no clear parent-child relationship here. Ideally we'd just not make them derive from GInitiallyUnowned at all, but since we want to keep using GstObject features for debugging, we'll just do it like this. This should also fix some problems with bindings, which seem to get confused when they get floating refs from non-constructor functions (or functions annotated to have a 'transfer full' return type). This works now: from gi.repository import GObject, Gst GObject.threads_init() Gst.init(None) pipeline=Gst.Pipeline() bus = pipeline.get_bus() pipeline.set_state(Gst.State.NULL) del pipeline; https://bugzilla.gnome.org/show_bug.cgi?id=679286 https://bugzilla.gnome.org/show_bug.cgi?id=657202 Which is also still correct: Whenever a floating reference appears in bindings, they have no way of knowing whether it's theirs or not. And so they currently assume that every floating reference that appears is theirs when seen in a (transfer none)/(transfer floating) context and ref_sink() it. With (transfer full) the problem is first out of the way, but delayed further down the line until it appears in a situation where a new reference has to be retrieved (ref_sink() again). The only solutions other than this here now are for the long-term: 1) Get rid of floating references completely (yes please!) 2) Add transfer annotations for all the different variants of how you can get a floating reference to GI. And break all users (bindings, code generators, etc) of GI while at it.
There are more types in GStreamer with this problem btw: GstAllocator and GstBufferPool at least. These are also never really "owned" by something, but have floating references. Same goes for various other types.
(In reply to Sebastian Dröge (slomo) from comment #17) > There are more types in GStreamer with this problem btw: GstAllocator and > GstBufferPool at least. These are also never really "owned" by something, > but have floating references. Same goes for various other types. And I guess we should just fix all of those in our code while we're at it. Otherwise someone will just be surprised at a later time and someone else has to understand the problem another time.
Created attachment 351899 [details] [review] gst: Clear floating flag in constructor of all GstObject subclasses that are not owned by any parent I.e. most of them unfortunately.
Created attachment 351906 [details] [review] gst: Clear floating flag in constructor of all GstObject subclasses that are not owned by any parent
Created attachment 351907 [details] [review] gst: Clear floating flag in constructor of all GstObject subclasses that are not owned by any parent
Created attachment 351912 [details] [review] gst: Clear floating flag in constructor of all GstObject subclasses that are not owned by any parent
Attachment 351883 [details] pushed as daa98fc - gst: Don't ref_sink() GstObject subclasses in instance_init/constructor Attachment 351884 [details] pushed as 888fc33 - allocator: ref_sink() the global sysmem allocator after creation Attachment 351912 [details] pushed as f119e93 - gst: Clear floating flag in constructor of all GstObject subclasses that are not owned by any parent
Comment on attachment 351886 [details] [review] audioclock: Sink the reference in the constructor Attachment 351886 [details] pushed as 2a8784e - audioclock: Sink the reference in the constructor
Comment on attachment 351887 [details] [review] 1394: Sink the clock reference in the constructor Attachment 351887 [details] pushed as defbe06 - 1394: Sink the clock reference in the constructor
Comment on attachment 351888 [details] [review] decklink: Sink the clock reference in the constructor Attachment 351888 [details] pushed as c40b8a8 - decklink: Sink the clock reference in the constructor
Comment on attachment 330071 [details] [review] Change GInitiallyUnowned subclasses to create a floating reference Merged the remaining parts of this
(In reply to Sebastian Dröge (slomo) from comment #16) > 1) Get rid of floating references completely (yes please!) Yes Please!
Thanks slomo!
This is unfortunately breaking ABI, and breaking all bindings that are generated from code. pre-1.14 bindings with 1.14 will leak, 1.14 bindings running with pre-1.14 GStreamer will crash sooner or later due to objects being destroyed too early. Too late to change it back now though, that will only break more things at this point.