GNOME Bugzilla – Bug 675726
props accessor leaks references for properties of type G_TYPE_OBJECT
Last modified: 2013-03-04 14:20:14 UTC
While attempting a patch for: https://bugzilla.gnome.org/show_bug.cgi?id=675582 I noticed the props accessor leaks references when accessing properties holding GObjects. This only occurs for objects with properties pulled in through introspection and will not happen with object properties dynamically generated in python. This is due to two code paths for retrieving the value: gi/_gobject/pygobject.c:PyGProps_getattro will call pygi_get_property_value which will eventually end up calling gi/pygi-argument.c:_pygi_argument_to_object of which lines 1678-1687 show an explicit g_object_ref for the value. This accounts for one of leaks but was also put in place to fix another bug, see: https://bugzilla.gnome.org/show_bug.cgi?id=661359 Steps to reproduce (using Binding directly here because it's the only thing I could find with the current code base to show this). Notice each access of "binding.props.source" increments __grefcount__ by two: from gi.repository import GObject, Gtk a = Gtk.Adjustment(upper=10) b = Gtk.Adjustment(upper=10) binding = GObject.Binding(source=a, source_property='value', target=b, target_property='value', flags=GObject.BindingFlags.DEFAULT) print 'count', a.__grefcount__ binding.props.source print 'count', a.__grefcount__ binding.props.source print 'count', a.__grefcount__
Updated example: from gi.repository import GObject, Gtk a = Gtk.Adjustment(upper=10) b = Gtk.Adjustment(upper=10) binding = a.bind_property('value', b, 'value') print('count %s' % a.__grefcount__) binding().props.source print('count %s' % a.__grefcount__) binding().props.source print('count %s' % a.__grefcount__)
Confirmed.
Created attachment 219871 [details] [review] unit test I turned Simon's test case in a unit test
This bug is caused by the fix for bug 661359. The root cause of which was the editing-started signal passes a GtkEntry instance which has a floating ref to the callback closure. Within the python callback closure the GtkEntry ref count is 2, 1 for the floating ref and 1 incremented by the signal emission. The GtkEntry argument is then wrapped by a PyGObject which sinks the floating ref bringing the ref count down to 1. Then after signal emission the refcount is again decremented, destroying the GtkEntry before the gtk_cell_renderer_text_start_editing actually uses it, causing the assertion. bug 661359 attempted to fix this by incremented the ref count for all GObject arguments being passed into closures. This then causes a leak for objects which are not floating. Perhaps the correct fix is for Gtk to not pass objects with floating refs into closures as the gtk cell renderer could very well own the ref for its editor.
Comment on attachment 219871 [details] [review] unit test Even when reverting the fix for bug 661359 this test case fails with self.assertEqual(n, obj1.__grefcount__) AssertionError: 1 != 2 i. e. the refcount still isn't 1 as expected. So this wasn't the only change which caused a leak here. The patch looks good, except that the comment could be a bit more verbose such as # verify that property access does not leak (#675726) Marking as reviewed; we do not want to commit this without a fix, but I'd like to get it off the patch review queue. Thanks!
Created attachment 234659 [details] [review] First pass at a possible patch. I think it would be safe to assume that we should not sink a floating ref if it comes from a signal, and we will not receive one when querying a property or field, although we do need to sink a floating ref on an object returned by a function. This patch might need some work (would changing the api for pygobject_new break anything outside of pygobject?), so I am only proposing it as a starting point. Also, it does not make the unit test pass, although I could write a unit test that this patch would fix. The other issue is that a ref is leaked when fetching the source property on a gbinding--the property is not annotated, so it is marked (transfer none) by default, but the getter in fact refs the object, so I'll file a separate bug for that.
Review of attachment 234659 [details] [review]: Thanks for looking at this. I've been playing with basically the same idea as well. One thing to be weary of is pygobject_new_full is called elsewhere with varying values of the sink argument, but the current implementation ignores the argument so it has no effect. The places where FALSE is passed to pygobject_new_full in gobjectmodule.c will need to change to pass TRUE in order to keep the current behavior (unless a change is intended). Instead of modifying pygobject_new to take sink, I just started using pygobject_new_full everywhere, but either way sounds fine to me. There is no problem with changing these APIs as pygobject does not expose them publicly. I've been trying to nail down a more complete analysis of all the places where we have to deal with GObject refs and the different situations which can occur. Please have a look at: https://live.gnome.org/PyGObject/Analysis/Object%20Reference%20Counting%20for%20VFuncs%20and%20Closures Comments corrections and ideas are very welcome. Additionally I have been working on unittests for a lot of these situations which can be found in bug 687522.
More reasonable link: https://live.gnome.org/PyGObject/Analysis/ObjectReferenceCountingForVFuncsAndClosures
Created attachment 234957 [details] [review] Fix leak when marshaling floating closure input object Modified patch so pygobject_new defaults to sinking refs and instead use pygobject_new_full where we don't want to sink. This is a bit less intrusive. I also removed the unref for TRANSFER_EVERYTHING on input args because there might be problems with that in cases where we already have a python wrapper for the input arg. In this case the additional ref added by pygobject_new_full would be skipped and we might get an invalid ref held by the wrapper if we unref. We need tests for this but the we are about to run into a combinatorial explosion in regards to test_object_marshaling.py. Added property accessor tests, this also found a new leak in setting properties.
Simon, is there still something else to be fixed for this bug, or should this be closed? Thank you!
(In reply to comment #10) > Simon, is there still something else to be fixed for this bug, or should this > be closed? Thank you! Getting close, but not until the new get/set property method leaks are resolved or at least known not to be the fault of pygobject.
I think the more recent leaks for getting/setting properties is caused by the fix for bug 684062. I recall get_property didn't leak at all and using "props.object" only leaked a single ref. So bug 684062 started using annotations for properties which are generally wrong which caused the new leak.
The test from comment 1 works properly now with the patch I just committed for fixing the test_gi.TestPropertiesObject.test_strv() leaks: http://git.gnome.org/browse/pygobject/commit/?id=8cfd596c7