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 675726 - props accessor leaks references for properties of type G_TYPE_OBJECT
props accessor leaks references for properties of type G_TYPE_OBJECT
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: gobject
unspecified
Other Linux
: Normal normal
: GNOME 3.8
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
Depends on: 692747
Blocks: 690397 693111
 
 
Reported: 2012-05-09 08:38 UTC by Simon Feltman
Modified: 2013-03-04 14:20 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
unit test (996 bytes, patch)
2012-07-30 09:17 UTC, Paolo Borelli
reviewed Details | Review
First pass at a possible patch. (9.31 KB, patch)
2013-01-28 21:12 UTC, Mike Gorse
none Details | Review
Fix leak when marshaling floating closure input object (10.72 KB, patch)
2013-02-01 10:46 UTC, Simon Feltman
committed Details | Review

Description Simon Feltman 2012-05-09 08:38:04 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__
Comment 1 Simon Feltman 2012-07-11 13:38:49 UTC
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__)
Comment 2 Martin Pitt 2012-07-24 13:03:44 UTC
Confirmed.
Comment 3 Paolo Borelli 2012-07-30 09:17:48 UTC
Created attachment 219871 [details] [review]
unit test

I turned Simon's test case in a unit test
Comment 4 Simon Feltman 2012-12-18 07:45:30 UTC
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 5 Martin Pitt 2013-01-14 08:57:00 UTC
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!
Comment 6 Mike Gorse 2013-01-28 21:12:30 UTC
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.
Comment 7 Simon Feltman 2013-01-29 05:20:21 UTC
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.
Comment 8 Simon Feltman 2013-01-29 05:40:04 UTC
More reasonable link:
https://live.gnome.org/PyGObject/Analysis/ObjectReferenceCountingForVFuncsAndClosures
Comment 9 Simon Feltman 2013-02-01 10:46:18 UTC
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.
Comment 10 Martin Pitt 2013-02-01 14:23:39 UTC
Simon, is there still something else to be fixed for this bug, or should this be closed? Thank you!
Comment 11 Simon Feltman 2013-02-02 05:16:10 UTC
(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.
Comment 12 Simon Feltman 2013-02-08 23:13:58 UTC
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.
Comment 13 Martin Pitt 2013-03-04 14:20:14 UTC
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