GNOME Bugzilla – Bug 782499
Floating references confusion
Last modified: 2017-05-17 12:38:48 UTC
This is a tracker bug, with some background and general solution. See below Related bugs (at least): https://bugzilla.gnome.org/show_bug.cgi?id=702960 https://bugzilla.gnome.org/show_bug.cgi?id=730138 https://bugzilla.gnome.org/show_bug.cgi?id=743062 https://bugzilla.gnome.org/show_bug.cgi?id=747990 And background info for GI here: https://bugzilla.gnome.org/show_bug.cgi?id=657202 https://bugzilla.gnome.org/show_bug.cgi?id=742618 And about floating references in general here: https://developer.gnome.org/gobject/stable/gobject-The-Base-Object-Type.html#floating-ref ========================= There are currently 3 distinct problems here 1) Bug #730138 and bug #702960 (and https://bugzilla.gnome.org/show_bug.cgi?id=702960#c25 especially). Functions taking a parameter and calling g_object_ref_sink() on them. According to GI and bindings developers, these should be "transfer none" (aka "transfer floating"), and this is what GTK+ does at least. We currently do "transfer full", which causes leaks in bindings. I would propose to mark these all "transfer floating" (aka "transfer none"). This keeps intention in the API docs ("floating") and makes bindings work. Currently bindings have overrides for our API (or in the case of Python just leak), and this shouldn't be necessary. We should do as a) GObject-Introspection developers and bindings developers say (see links above), and b) as bindings that work well (GTK especially) do. If we diverge from that, problems will appear. 2) Bug #747990. Our functions that ref_sink() (directly or indirectly) are inconsistent with that, and often behave different in error paths. This should obviously be fixed. That bug has an initial patch but it is not complete, see also https://bugzilla.gnome.org/show_bug.cgi?id=702960#c25 3) Bug #743062. We have a couple of GstObjects that are ref_sink()'d in their instance_init / GObject::constructor. This causes leaks in bindings as they have no way of knowing about that when using g_object_new(), as most bindings do. For the explicit constructors (gst_bin_new()) there are annotations ("transfer floating" aka "transfer none" in the case of not ref_sinking, or "transfer full" otherwise (control sources, clocks, etc.). Note that the second case seems to be generally wrong currently! I would propose to ref_sink() these objects in the explicit constructor (the new() functions) instead of instance_init / GObject::constructor, and fix up the annotations of them. Any other thoughts on this? I would suggest to fix this for 1.14, ASAP to give it a lot of testing time.
You approach to those 3 points sound very sensible to me. For 1. we are just doing it wrong right now, and we can see Gtk+ and GI devs agreed on a rightish solution, so let's do that. For 2., well obviously we need to have concitent way of handling refs in a function For the 3rd point, I like the idea of just ref_sink in the _new() constructor so that we can let the bindings know that we are returning an actual ref, as we can't let it know when it uses g_object_new().
I'll work on these Sunday / next week unless there are any objections.
https://lists.freedesktop.org/archives/gstreamer-devel/2017-May/063856.html