Commit 5efe2e5c authored by Simon Feltman's avatar Simon Feltman
Browse files

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 premise
that binding layers should always sink objects 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.

https://bugzilla.gnome.org/show_bug.cgi?id=687522
parent bd54b8ab
......@@ -996,7 +996,11 @@ pygobject_constructv(PyGObject *self,
pygobject_init_wrapper_set((PyObject *) self);
obj = g_object_newv(pyg_type_from_object((PyObject *) self),
n_parameters, parameters);
if (g_object_is_floating (obj))
self->private_flags.flags |= PYGOBJECT_GOBJECT_WAS_FLOATING;
pygobject_sink (obj);
pygobject_init_wrapper_set(NULL);
if (self->obj == NULL) {
self->obj = obj;
......@@ -1034,7 +1038,9 @@ pygobject__g_instance_init(GTypeInstance *instance,
* now */
PyGILState_STATE state;
state = pyglib_gil_state_ensure();
wrapper = pygobject_new_full(object, TRUE, g_class);
wrapper = pygobject_new_full(object,
/*steal=*/ FALSE,
g_class);
/* float the wrapper ref here because we are going to orphan it
* so we don't destroy the wrapper. The next call to pygobject_new_full
......@@ -1477,7 +1483,7 @@ pyg_object_new (PyGObject *self, PyObject *args, PyObject *kwargs)
if (obj) {
pygobject_sink (obj);
self = (PyGObject *) pygobject_new_full((GObject *)obj, TRUE, NULL);
self = (PyGObject *) pygobject_new((GObject *)obj);
g_object_unref(obj);
} else
self = NULL;
......
......@@ -154,7 +154,7 @@ void pygobject_register_class (PyObject *dict,
PyObject *bases);
void pygobject_register_wrapper (PyObject *self);
PyObject * pygobject_new (GObject *obj);
PyObject * pygobject_new_full (GObject *obj, gboolean sink, gpointer g_class);
PyObject * pygobject_new_full (GObject *obj, gboolean steal, gpointer g_class);
void pygobject_sink (GObject *obj);
PyTypeObject *pygobject_lookup_class (GType gtype);
void pygobject_watch_closure (PyObject *self, GClosure *closure);
......
......@@ -951,7 +951,7 @@ pygobject_lookup_class(GType gtype)
/**
* pygobject_new_full:
* @obj: a GObject instance.
* @sink: whether to sink any floating reference found on the GObject.
* @steal: whether to steal a ref from the GObject or add (sink) a new one.
* @g_class: the GObjectClass
*
* This function gets a reference to a wrapper for the given GObject
......@@ -962,19 +962,30 @@ pygobject_lookup_class(GType gtype)
* Returns: a reference to the wrapper for the GObject.
*/
PyObject *
pygobject_new_full(GObject *obj, gboolean sink, gpointer g_class)
pygobject_new_full(GObject *obj, gboolean steal, gpointer g_class)
{
PyGObject *self;
if (obj == NULL) {
Py_INCREF(Py_None);
return Py_None;
Py_RETURN_NONE;
}
/* we already have a wrapper for this object -- return it. */
/* If the GObject already has a PyObject wrapper stashed in its qdata, re-use it.
*/
self = (PyGObject *)g_object_get_qdata(obj, pygobject_wrapper_key);
if (self != NULL) {
pygobject_ref_sink(self);
/* Note the use of "pygobject_ref_sink" here only deals with PyObject
* wrapper ref counts and has nothing to do with GObject.
*/
pygobject_ref_sink(self);
/* If steal is true, we also want to decref the incoming GObjects which
* already have a Python wrapper because the wrapper is already holding a
* strong reference.
*/
if (steal)
g_object_unref (obj);
} else {
/* create wrapper */
PyGObjectData *inst_data = pyg_object_peek_inst_data(obj);
......@@ -1000,13 +1011,15 @@ pygobject_new_full(GObject *obj, gboolean sink, gpointer g_class)
self->weakreflist = NULL;
self->private_flags.flags = 0;
self->obj = obj;
/* if we are creating a wrapper around a newly created object, it can have
a floating ref (e.g. for methods like Gtk.Button.new()). Bug 640868 */
if (sink)
g_object_ref_sink(obj);
else
g_object_ref(obj);
pygobject_register_wrapper((PyObject *)self);
/* If we are not stealing a ref or the object is floating,
* add a regular ref or sink the object. */
if (g_object_is_floating (obj))
self->private_flags.flags |= PYGOBJECT_GOBJECT_WAS_FLOATING;
if (!steal || self->private_flags.flags & PYGOBJECT_GOBJECT_WAS_FLOATING)
g_object_ref_sink (obj);
pygobject_register_wrapper((PyObject *)self);
PyObject_GC_Track((PyObject *)self);
}
......@@ -1017,7 +1030,9 @@ pygobject_new_full(GObject *obj, gboolean sink, gpointer g_class)
PyObject *
pygobject_new(GObject *obj)
{
return pygobject_new_full(obj, TRUE, NULL);
return pygobject_new_full(obj,
/*steal=*/FALSE,
NULL);
}
static void
......@@ -2343,7 +2358,7 @@ pygobject_weak_ref_call(PyGObjectWeakRef *self, PyObject *args, PyObject *kw)
return NULL;
if (self->obj)
return pygobject_new_full(self->obj, FALSE, NULL);
return pygobject_new(self->obj);
else {
Py_INCREF(Py_None);
return Py_None;
......
......@@ -24,7 +24,8 @@ struct _PyGClosure {
typedef enum {
PYGOBJECT_USING_TOGGLE_REF = 1 << 0,
PYGOBJECT_IS_FLOATING_REF = 1 << 1
PYGOBJECT_IS_FLOATING_REF = 1 << 1,
PYGOBJECT_GOBJECT_WAS_FLOATING = 1 << 2,
} PyGObjectFlags;
/* closures is just an alias for what is found in the
......@@ -187,8 +188,9 @@ struct _PyGObject_Functions {
gpointer data);
gboolean (*gerror_exception_check) (GError **error);
PyObject* (*option_group_new) (GOptionGroup *group);
GType (* type_from_object_strict) (PyObject *obj, gboolean strict);
PyObject *(* newgobj_full)(GObject *obj, gboolean sink, gpointer g_class);
GType (* type_from_object_strict) (PyObject *obj, gboolean strict);
PyObject *(* newgobj_full)(GObject *obj, gboolean steal, gpointer g_class);
};
#ifndef _INSIDE_PYGOBJECT_
......
......@@ -31,6 +31,10 @@
#include <pyglib-python-compat.h>
#include <pyglib.h>
#include "pygi-marshal-from-py.h"
#include "pygi-marshal-to-py.h"
static gboolean
gi_argument_to_gssize (GIArgument *arg_in,
GITypeTag type_tag,
......@@ -1329,17 +1333,10 @@ array_success:
}
case GI_INFO_TYPE_INTERFACE:
case GI_INFO_TYPE_OBJECT:
if (object == Py_None) {
arg.v_pointer = NULL;
break;
}
arg.v_pointer = pygobject_get (object);
if (transfer == GI_TRANSFER_EVERYTHING) {
g_object_ref (arg.v_pointer);
}
/* An error within this call will result in a NULL arg */
pygi_marshal_from_py_object (object, &arg, transfer);
break;
default:
g_assert_not_reached();
}
......@@ -1856,23 +1853,26 @@ _pygi_argument_to_object (GIArgument *arg,
}
case GI_INFO_TYPE_INTERFACE:
case GI_INFO_TYPE_OBJECT:
if (arg->v_pointer == NULL) {
object = Py_None;
Py_INCREF (object);
break;
}
if (G_IS_PARAM_SPEC (arg->v_pointer)) {
object = pyg_param_spec_new (arg->v_pointer);
break;
}
/* Only sink incoming objects if the transfer everything.
* See: https://bugzilla.gnome.org/show_bug.cgi?id=675726
/* HACK:
* The following hack is to work around GTK+ sending signals which
* contain floating widgets in them. This assumes control of how
* references are added by the PyGObject wrapper and avoids the sink
* behavior by explicitly passing GI_TRANSFER_EVERYTHING as the transfer
* mode and then re-forcing the object as floating afterwards.
*
* This can be deleted once the following ticket is fixed:
* https://bugzilla.gnome.org/show_bug.cgi?id=693400
*/
object = pygobject_new_full (arg->v_pointer,
/*sink=*/ transfer == GI_TRANSFER_EVERYTHING,
/*type=*/ NULL);
if (arg->v_pointer &&
!G_IS_PARAM_SPEC (arg->v_pointer) &&
transfer == GI_TRANSFER_NOTHING &&
g_object_is_floating (arg->v_pointer)) {
g_object_ref (arg->v_pointer);
object = pygi_marshal_to_py_object (arg, GI_TRANSFER_EVERYTHING);
g_object_force_floating (arg->v_pointer);
} else {
object = pygi_marshal_to_py_object (arg, transfer);
}
break;
default:
......
......@@ -1760,11 +1760,7 @@ _pygi_marshal_from_py_interface_object (PyGIInvokeState *state,
return FALSE;
}
arg->v_pointer = pygobject_get(py_arg);
if (arg_cache->transfer == GI_TRANSFER_EVERYTHING)
g_object_ref (arg->v_pointer);
return TRUE;
return pygi_marshal_from_py_object (py_arg, arg, arg_cache->transfer);
}
gboolean
......@@ -1855,3 +1851,58 @@ gboolean _pygi_marshal_from_py_interface_instance (PyGIInvokeState *state,
return TRUE;
}
gboolean
pygi_marshal_from_py_object (PyObject *pyobj, /*in*/
GIArgument *arg, /*out*/
GITransfer transfer) {
GObject *gobj;
if (pyobj == Py_None) {
arg->v_pointer = NULL;
return TRUE;
}
gobj = pygobject_get (pyobj);
if (transfer == GI_TRANSFER_EVERYTHING) {
/* An easy case of adding a new ref that the caller will take ownership of.
* Pythons existing ref to the GObject will be managed normally with the wrapper.
*/
g_object_ref (gobj);
} else if (pyobj->ob_refcnt == 1 && gobj->ref_count == 1) {
/* If both object ref counts are only 1 at this point (the reference held
* in a return tuple), we assume the GObject will be free'd before reaching
* its target and become invalid. So instead of getting invalid object errors
* we add a new GObject ref.
*/
g_object_ref (gobj);
if (((PyGObject *)pyobj)->private_flags.flags & PYGOBJECT_GOBJECT_WAS_FLOATING) {
/* HACK:
* We want to re-float instances that were floating and the Python
* wrapper assumed ownership. With the additional caveat that there
* are not any strong references beyond the return tuple.
* This should be removed once the following ticket is fixed:
* https://bugzilla.gnome.org/show_bug.cgi?id=693393
*/
g_object_force_floating (gobj);
} else {
PyObject *repr = PyObject_Repr (pyobj);
gchar *msg = g_strdup_printf ("Expecting to marshal a borrowed reference for %s, "
"but nothing in Python is holding a reference to this object. "
"See: https://bugzilla.gnome.org/show_bug.cgi?id=687522",
PYGLIB_PyUnicode_AsString(repr));
Py_DECREF (repr);
if (PyErr_WarnEx (PyExc_RuntimeWarning, msg, 2)) {
g_free (msg);
return FALSE;
}
g_free (msg);
}
}
arg->v_pointer = gobj;
return TRUE;
}
......@@ -184,6 +184,12 @@ gboolean _pygi_marshal_from_py_interface_instance (PyGIInvokeState *state,
PyObject *py_arg,
GIArgument *arg);
/* Simplified marshalers shared between vfunc/closure and direct function calls. */
gboolean pygi_marshal_from_py_object (PyObject *pyobj, /*in*/
GIArgument *arg, /*out*/
GITransfer transfer);
G_END_DECLS
#endif /* __PYGI_MARSHAL_from_py_PY__ */
......@@ -877,28 +877,7 @@ _pygi_marshal_to_py_interface_object (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
GIArgument *arg)
{
PyObject *py_obj;
if (arg->v_pointer == NULL) {
py_obj = Py_None;
Py_INCREF (py_obj);
return py_obj;
}
if (G_IS_PARAM_SPEC(arg->v_pointer))
{
py_obj = pyg_param_spec_new (arg->v_pointer);
if (arg_cache->transfer == GI_TRANSFER_EVERYTHING)
g_param_spec_unref (arg->v_pointer);
}
else
{
py_obj = pygobject_new (arg->v_pointer);
if (arg_cache->transfer == GI_TRANSFER_EVERYTHING)
g_object_unref (arg->v_pointer);
}
return py_obj;
return pygi_marshal_to_py_object(arg, arg_cache->transfer);
}
PyObject *
......@@ -913,3 +892,25 @@ _pygi_marshal_to_py_interface_union (PyGIInvokeState *state,
"Marshalling for this type is not implemented yet");
return py_obj;
}
PyObject *
pygi_marshal_to_py_object (GIArgument *arg, GITransfer transfer) {
PyObject *pyobj;
if (arg->v_pointer == NULL) {
pyobj = Py_None;
Py_INCREF (pyobj);
} else if (G_IS_PARAM_SPEC(arg->v_pointer)) {
pyobj = pyg_param_spec_new (arg->v_pointer);
if (transfer == GI_TRANSFER_EVERYTHING)
g_param_spec_unref (arg->v_pointer);
} else {
pyobj = pygobject_new_full (arg->v_pointer,
/*steal=*/ transfer == GI_TRANSFER_EVERYTHING,
/*type=*/ NULL);
}
return pyobj;
}
......@@ -139,6 +139,11 @@ PyObject *_pygi_marshal_to_py_interface_union (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
GIArgument *arg);
/* Simplified marshalers shared between vfunc/closure and direct function calls. */
PyObject *pygi_marshal_to_py_object (GIArgument *arg,
GITransfer transfer);
G_END_DECLS
#endif /* __PYGI_MARSHAL_TO_PY_H__ */
......@@ -5,6 +5,7 @@ import unittest
import weakref
import gc
import sys
import warnings
from gi.repository import GObject
from gi.repository import GIMarshallingTests
......@@ -109,31 +110,36 @@ class TestVFuncsWithObjectArg(unittest.TestCase):
gc.collect()
self.assertTrue(vfuncs_ref() is None)
@unittest.skip('bug 687522, should float returned object')
def test_vfunc_return_object_transfer_none(self):
# This tests a problem case where the vfunc returns a GObject owned solely by Python
# but the argument is marked as transfer-none.
# In this case pygobject marshaling should add an additional ref and give a warning
# of a potential leak.
# In this case pygobject marshaling adds an additional ref and gives a warning
# of a potential leak. If this occures it is really a bug in the underlying library
# but pygobject tries to react to this in a reasonable way.
vfuncs = self.VFuncs()
ref_count, is_floating = vfuncs.get_ref_info_for_vfunc_return_object_transfer_none()
with warnings.catch_warnings(record=True) as warn:
warnings.simplefilter('always')
ref_count, is_floating = vfuncs.get_ref_info_for_vfunc_out_object_transfer_none()
self.assertTrue(issubclass(warn[0].category, RuntimeWarning))
# The ref count of the GObject returned to the caller (get_ref_info_for_vfunc_return_object_transfer_none)
# should be a single floating ref
self.assertEqual(ref_count, 1)
self.assertTrue(is_floating)
self.assertFalse(is_floating)
gc.collect()
self.assertTrue(vfuncs.object_ref() is None)
@unittest.skip('bug 687522, should float returned object')
def test_vfunc_out_object_transfer_none(self):
# Same as above except uses out arg instead of return
vfuncs = self.VFuncs()
ref_count, is_floating = vfuncs.get_ref_info_for_vfunc_out_object_transfer_none()
with warnings.catch_warnings(record=True) as warn:
warnings.simplefilter('always')
ref_count, is_floating = vfuncs.get_ref_info_for_vfunc_out_object_transfer_none()
self.assertTrue(issubclass(warn[0].category, RuntimeWarning))
self.assertEqual(ref_count, 1)
self.assertTrue(is_floating)
self.assertFalse(is_floating)
gc.collect()
self.assertTrue(vfuncs.object_ref() is None)
......@@ -174,7 +180,6 @@ class TestVFuncsWithObjectArg(unittest.TestCase):
self.assertTrue(vfuncs.object_ref() is None)
@unittest.expectedFailure # bug 675726
def test_vfunc_in_object_transfer_full(self):
vfuncs = self.VFuncs()
ref_count, is_floating = vfuncs.get_ref_info_for_vfunc_in_object_transfer_full(self.VFuncs.Object)
......@@ -200,7 +205,6 @@ class TestVFuncsWithFloatingArg(unittest.TestCase):
Object = GObject.InitiallyUnowned
ObjectRef = weakref.ref
@unittest.skip('bug 687522, should float returned object')
def test_vfunc_return_object_transfer_none_with_floating(self):
# Python is expected to return a single floating reference without warning.
vfuncs = self.VFuncs()
......@@ -214,7 +218,6 @@ class TestVFuncsWithFloatingArg(unittest.TestCase):
gc.collect()
self.assertTrue(vfuncs.object_ref() is None)
@unittest.skip('bug 687522, should float returned object')
def test_vfunc_out_object_transfer_none_with_floating(self):
# Same as above except uses out arg instead of return
vfuncs = self.VFuncs()
......@@ -226,26 +229,24 @@ class TestVFuncsWithFloatingArg(unittest.TestCase):
gc.collect()
self.assertTrue(vfuncs.object_ref() is None)
@unittest.expectedFailure # bug 687522, should float returned object
def test_vfunc_return_object_transfer_full_with_floating(self):
vfuncs = self.VFuncs()
ref_count, is_floating = vfuncs.get_ref_info_for_vfunc_return_object_transfer_full()
# The vfunc caller receives full ownership of a single ref which should be floating.
# The vfunc caller receives full ownership of a single ref.
self.assertEqual(ref_count, 1)
self.assertTrue(is_floating)
self.assertFalse(is_floating)
gc.collect()
self.assertTrue(vfuncs.object_ref() is None)
@unittest.expectedFailure # bug 687522, should float returned object
def test_vfunc_out_object_transfer_full_with_floating(self):
# Same as above except uses out arg instead of return
vfuncs = self.VFuncs()
ref_count, is_floating = vfuncs.get_ref_info_for_vfunc_out_object_transfer_full()
self.assertEqual(ref_count, 1)
self.assertTrue(is_floating)
self.assertFalse(is_floating)
gc.collect()
self.assertTrue(vfuncs.object_ref() is None)
......@@ -367,7 +368,6 @@ class TestVFuncsWithHeldObjectArg(unittest.TestCase):
gc.collect()
self.assertTrue(held_object_ref() is None)
@unittest.expectedFailure # bug 675726, leaks
def test_vfunc_in_object_transfer_none_with_held_object(self):
vfuncs = self.VFuncs()
ref_count, is_floating = vfuncs.get_ref_info_for_vfunc_in_object_transfer_none(self.VFuncs.Object)
......@@ -382,15 +382,14 @@ class TestVFuncsWithHeldObjectArg(unittest.TestCase):
self.assertEqual(ref_count, 2) # kept after vfunc + held python wrapper
self.assertFalse(is_floating)
# Current ref count
self.assertEqual(vfuncs.object_ref().__grefcount__, 2) # initial + python wrapper
# Current ref count after C cleans up its reference
self.assertEqual(vfuncs.object_ref().__grefcount__, 1)
held_object_ref = weakref.ref(vfuncs.object_ref())
del vfuncs.object_ref
gc.collect()
self.assertTrue(held_object_ref() is None)
@unittest.expectedFailure # bug 675726, leaks
def test_vfunc_in_object_transfer_full_with_held_object(self):
vfuncs = self.VFuncs()
ref_count, is_floating = vfuncs.get_ref_info_for_vfunc_in_object_transfer_full(self.VFuncs.Object)
......@@ -493,7 +492,6 @@ class TestVFuncsWithHeldFloatingArg(unittest.TestCase):
gc.collect()
self.assertTrue(held_object_ref() is None)
@unittest.expectedFailure # bug 675726, should maintain floating
def test_vfunc_in_floating_transfer_none_with_held_floating(self):
vfuncs = self.VFuncs()
ref_count, is_floating = vfuncs.get_ref_info_for_vfunc_in_object_transfer_none(self.VFuncs.Object)
......@@ -507,8 +505,8 @@ class TestVFuncsWithHeldFloatingArg(unittest.TestCase):
self.assertTrue(is_floating)
self.assertEqual(ref_count, 2) # floating + held by wrapper
# Current ref count
self.assertEqual(vfuncs.object_ref().__grefcount__, 2) # floating + held by wrapper
# Current ref count after C cleans up its reference
self.assertEqual(vfuncs.object_ref().__grefcount__, 1)
held_object_ref = weakref.ref(vfuncs.object_ref())
del vfuncs.object_ref
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment