GNOME Bugzilla – Bug 687522
Objects returned from vfuncs should maintain floating reference
Last modified: 2013-02-08 11:00:18 UTC
Trying to run pygobject's appwindow.py demo gives the following error. [liveuser@localhost demos]$ python appwindow.py /usr/lib64/python2.7/site-packages/gi/types.py:47: Warning: g_object_set: assertion `G_IS_OBJECT (object)' failed return info.invoke(*args, **kwargs) (appwindow.py:4056): Gtk-CRITICAL **: gtk_activatable_set_related_action: assertion `GTK_IS_ACTIVATABLE (activatable)' failed /usr/lib64/python2.7/site-packages/gi/types.py:47: Warning: g_object_ref_sink: assertion `G_IS_OBJECT (object)' failed return info.invoke(*args, **kwargs) (appwindow.py:4056): Gtk-CRITICAL **: gtk_widget_set_name: assertion `GTK_IS_WIDGET (widget)' failed (appwindow.py:4056): Gtk-CRITICAL **: gtk_toolbar_insert: assertion `GTK_IS_TOOL_ITEM (item)' failed /usr/lib64/python2.7/site-packages/gi/types.py:47: Warning: instance of invalid non-instantiatable type `<invalid>' return info.invoke(*args, **kwargs) /usr/lib64/python2.7/site-packages/gi/types.py:47: Warning: g_signal_connect_data: assertion `G_TYPE_CHECK_INSTANCE (instance)' failed return info.invoke(*args, **kwargs) (appwindow.py:4056): Gtk-CRITICAL **: gtk_menu_tool_button_set_menu: assertion `GTK_IS_MENU_TOOL_BUTTON (button)' failed appwindow.py:407: Warning: g_object_unref: assertion `G_IS_OBJECT (object)' failed main() The code that I assume fails is below. class ToolMenuAction(Gtk.Action): __gtype_name__ = "GtkToolMenuAction" def do_create_tool_item(self): return Gtk.MenuToolButton() I have used the same 'do_create_tool_item' approach in gaupol, where I have gotten a bug report [1] according to which this happens at least on fedora 18 and arch linux, both of which have the latest pygobject and gtk+. With earlier 3.x versions of pygobject and/or gtk+ this 'do_create_tool_item' approach used to work. [1] https://bugzilla.gnome.org/show_bug.cgi?id=686608
Confirmed. Testing with python 3.3: - trunk and 3.4.2 fail with above bug - 3.2.0, 3.2.2, and 3.0.4 just crash without any output Testing with python 2.7: - trunk and 3.4.2 fail with above bug - 3.0.4 and 3.2.2 work Bisecting leads to this commit which introduced this bug: http://git.gnome.org/browse/pygobject/commit/?id=6ba6b7ba90122954bb421c23606e9516697ba147 Simon, any idea why the extra DECREF is bad here? Might Gtk.MenuToolButton() be a floating reference of some kind? (although this affects python refs, not GObject refs)
FTR, when I comment out the decref on trunk, appwindow.py works again with python2.7. It still doesn't work with python 3.3.
Taking a look...
What's happening is the within the vfunc the new MenuToolButton is being created and then sunk by the python object wrapper. The marshaling code attempts to steal the gobject reference in this case. The python DECREF will then also destroy the gobject in this case because it essentially owns it. I think when returning gobjects, we might always want to incref them regardless of if they are transfer full or none. Depending on the urgency we could remove the Py_DECREF and let it leak again until this can be fixed properly. For confidence hacking at these tricky parts of the code, having ref count tests for different combinations (ones we don't already test) of the following concepts as applied to gobjects will be needed: transfer none (bug 687522) transfer full floating ref (bug 661359) owned ref vfunc return (bug 687522) vfunc out arg vfunc in arg (bug 661359) property get (bug 675726) property set
Created attachment 233582 [details] [review] gimarshallingtests: Add tests helpers for marshaling of object arguments Add a number of vfuncs and methods which can be used for testing marshaling of objects with different combinations of ownership transference. An important part of these test vfuncs and methods is they do not pass object returns and arguments through them. Instead the methods return reference counts and floating attributes. This allows isolation and ensures any problem with round trip object marshaling does not obscure what should be tested from the perspective of C as the caller of a vfunc. Tests and vfuncs can then be written in any gi based language.
Created attachment 233583 [details] [review] Add tests for vfunc object arguments and returns Add tests which use different combinations of floating, transfer full, transfer none, and held wrapper as in, out, or return arguments to vfuncs. Most of these are marked as skip or expectedFailure due to various bugs noted on the tests.
Thanks Simon! It would be nice if the new vfuncs in gimarshallingtests.c could get some comments what they do; in particular, it's not quite obvious why they are called "return_object_*" without returning anything, or why there are _none_ and _full_ alternatives which have exactly the same function and annotation signature (there is no (transfer) annotation anywhere). Please feel free to push the tests, unless they cause aborts on the test suite; but it seems you already took care of that by skipping them.
(In reply to comment #7) > Thanks Simon! > > It would be nice if the new vfuncs in gimarshallingtests.c could get some > comments what they do; in particular, it's not quite obvious why they are > called "return_object_*" without returning anything, or why there are _none_ > and _full_ alternatives which have exactly the same function and annotation > signature (there is no (transfer) annotation anywhere). Will do. The methods are simply helpers for calling vfuncs which have those annotations. So perhaps I should name them as "call_vfunc_..." For now, I'll mark these patches as needing work and wait a little more bit before committing.
Created attachment 234823 [details] [review] gimarshallingtests: Add tests helpers for marshaling of object arguments Added comments and simplified the number of test helper methods by allowing a GType are to be passed for the type of object creation.
Created attachment 234824 [details] [review] Add tests for vfunc object arguments and returns Breaks tests out into a new file (test_object_marshaling.py) because test_gi is getting too big. Eventually other object marshaling tests (besides vfunc tests) can move into this new file as well.
Comment on attachment 234823 [details] [review] gimarshallingtests: Add tests helpers for marshaling of object arguments Thanks for the additional documentation! That helps.
Comment on attachment 234824 [details] [review] Add tests for vfunc object arguments and returns Thanks, please go ahead with this.
Comment on attachment 234823 [details] [review] gimarshallingtests: Add tests helpers for marshaling of object arguments Pushed with some additional cleanup
Comment on attachment 234824 [details] [review] Add tests for vfunc object arguments and returns Pushed with some additional test method renaming and Python 2.7 fixes. Attachment 234824 [details] pushed as 97f48f5 - Add tests for vfunc object arguments and returns
Created attachment 235051 [details] [review] Add better reference management for transient floating objects Track PyObject wrappers which sink GObjects that are initially floating. This allows for re-floating them when the GObject is only temporary passed through the Python boundary and avoids both leaks and invalid objects passed to C. Print warning when a callback/vfunc is returning the last reference to an object which will no longer be managed by Python. Remove "private_flags" union on PyGObject as this class does not have a public API anymore.
Created attachment 235285 [details] [review] Unify and refactor caller and callee GObject argument marshalers Combine code from the large switch statement used to marshal arguments to and from vfuncs/closures with the marshalers used for direct calls to gi functions. This fixes a reference leak when marshaling GObjects to Python with transfer=full due to the diverging code paths.
Created attachment 235491 [details] [review] Fix reference leaks with transient floating objects Unify and refactor caller and callee GObject argument marshalers. Combine code from the large switch statement used to marshal arguments to and from vfuncs/closures with the marshalers used for direct calls to gi functions. This fixes a reference leak when marshalling GObjects to Python with transfer=full due to the diverging code paths. Replace ability in gobject_new_full to optionally sink objects with ability to optionaly "steal" objects. This fits the premis that binding layers should always sink object initially. The steal argument is then used for marshalling arguments which are transfer=full. Add hacks and comments to work around GTK+ bugs 693393 and 693400.
Comment on attachment 235491 [details] [review] Fix reference leaks with transient floating objects Thanks Simon! I like this a lot, it makes the code quite bit simpler and clearer, and makes assumptions more explicit. > with ability to optionaly "steal" objects. This fits the premis "premise" +static inline gboolean +pygi_marshal_from_py_object (PyObject *pyobj, /*in*/ +static inline PyObject * +pygi_marshal_to_py_object (GIArgument *arg, GITransfer transfer) { Should these really be inline? They are quite nontrivial, and even bound to change in the future (see the HACKS). When having them inline, stuff that builds against pygobject needs to get recompiled.
Created attachment 235496 [details] [review] Fix reference leaks with transient floating objects Move inline functions into respective C files.
Created attachment 235497 [details] [review] Fix reference leaks with transient floating objects Fixed spelling and moved inline functions into respective C files.
Comment on attachment 235497 [details] [review] Fix reference leaks with transient floating objects Thanks!
Attachment 235497 [details] pushed as 5efe2e5 - Fix reference leaks with transient floating objects