[pygobject] Fix ref count leak when creating pygobject wrappers for input args

[Simon Feltman <sfeltman src gnome org>]

commit 9bc3e6807f6c14fb0e132a90ff8f9984229896f6
Author: Mike Gorse <mgorse suse com>
Date:   Mon Jan 21 16:45:52 2013 -0600

    Fix ref count leak when creating pygobject wrappers for input args
    
    Only sink input references for closures and vfuncs when transfer is
    everything. This fixes cases where incoming floating references for
    callbacks need to maintain their floating state throughout the
    callback so they don't leak a strong reference. Re-introduce a
    working "sink" argument to pygobject_new_full which allows for this.
    Change existing callers to always sink in order maintain behavior.
    
    Co-Authored-By: Simon Feltman <sfeltman src gnome org>
    
    https://bugzilla.gnome.org/show_bug.cgi?id=675726

 gi/_gobject/gobjectmodule.c     |    5 ++-
 gi/_gobject/pygobject.c         |    7 +++-
 gi/_gobject/pygobject.h         |    2 +
 gi/pygi-argument.c              |   13 +++----
 tests/test_object_marshaling.py |   76 +++++++++++++++++++++++++++++++++++---
 5 files changed, 85 insertions(+), 18 deletions(-)
---
diff --git a/gi/_gobject/gobjectmodule.c b/gi/_gobject/gobjectmodule.c
index dcfde38..0731193 100644
--- a/gi/_gobject/gobjectmodule.c
+++ b/gi/_gobject/gobjectmodule.c
@@ -1034,7 +1034,7 @@ pygobject__g_instance_init(GTypeInstance   *instance,
            * now */
         PyGILState_STATE state;
         state = pyglib_gil_state_ensure();
-        wrapper = pygobject_new_full(object, FALSE, g_class);
+        wrapper = pygobject_new_full(object, TRUE, 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 +1477,7 @@ pyg_object_new (PyGObject *self, PyObject *args, PyObject *kwargs)
 
     if (obj) {
         pygobject_sink (obj);
-	self = (PyGObject *) pygobject_new_full((GObject *)obj, FALSE, NULL);
+	self = (PyGObject *) pygobject_new_full((GObject *)obj, TRUE, NULL);
         g_object_unref(obj);
     } else
         self = NULL;
@@ -2050,6 +2050,7 @@ struct _PyGObject_Functions pygobject_api_functions = {
   pygobject_register_wrapper,
   pygobject_lookup_class,
   pygobject_new,
+  pygobject_new_full,
 
   pyg_closure_new,
   pygobject_watch_closure,
diff --git a/gi/_gobject/pygobject.c b/gi/_gobject/pygobject.c
index add71e2..36d0874 100644
--- a/gi/_gobject/pygobject.c
+++ b/gi/_gobject/pygobject.c
@@ -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. DEPRECATED.
+ * @sink: whether to sink any floating reference found on the GObject.
  * @g_class: the GObjectClass
  *
  * This function gets a reference to a wrapper for the given GObject
@@ -1002,7 +1002,10 @@ pygobject_new_full(GObject *obj, gboolean sink, gpointer g_class)
 	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 */
-	g_object_ref_sink(obj);
+        if (sink)
+	    g_object_ref_sink(obj);
+        else
+	    g_object_ref(obj);
 	pygobject_register_wrapper((PyObject *)self);
 	PyObject_GC_Track((PyObject *)self);
     }
diff --git a/gi/_gobject/pygobject.h b/gi/_gobject/pygobject.h
index 8879fd0..3f637ce 100644
--- a/gi/_gobject/pygobject.h
+++ b/gi/_gobject/pygobject.h
@@ -92,6 +92,7 @@ struct _PyGObject_Functions {
     void (* register_wrapper)(PyObject *self);
     PyTypeObject *(* lookup_class)(GType type);
     PyObject *(* newgobj)(GObject *obj);
+    PyObject *(* newgobj_full)(GObject *obj, gboolean sink, GType type);
 
     GClosure *(* closure_new)(PyObject *callback, PyObject *extra_args,
 			      PyObject *swap_data);
@@ -202,6 +203,7 @@ struct _PyGObject_Functions *_PyGObject_API;
 #define pygobject_register_wrapper  (_PyGObject_API->register_wrapper)
 #define pygobject_lookup_class      (_PyGObject_API->lookup_class)
 #define pygobject_new               (_PyGObject_API->newgobj)
+#define pygobject_new_full          (_PyGObject_API->newgobj_full)
 #define pyg_closure_new             (_PyGObject_API->closure_new)
 #define pygobject_watch_closure     (_PyGObject_API->object_watch_closure)
 #define pyg_closure_set_exception_handler (_PyGObject_API->closure_set_exception_handler)
diff --git a/gi/pygi-argument.c b/gi/pygi-argument.c
index 2935bc6..53e0b5c 100644
--- a/gi/pygi-argument.c
+++ b/gi/pygi-argument.c
@@ -1867,16 +1867,13 @@ _pygi_argument_to_object (GIArgument  *arg,
                       break;
                     }
 
-                    /* since we will unref the object when the
-                     * wrapper is destroyed and we don't want
-                     * GTK removing the object while the
-                     * wrapper is live, we take a gobject reference
-                     * when one is not transfered to us
+                    /* Only sink incoming objects if the transfer everything.
+                     * See: https://bugzilla.gnome.org/show_bug.cgi?id=675726
                      */
-                    if (transfer == GI_TRANSFER_NOTHING)
-                        g_object_ref (G_OBJECT(arg->v_pointer));
+                    object = pygobject_new_full (arg->v_pointer,
+                                                 /*sink=*/ transfer == GI_TRANSFER_EVERYTHING,
+                                                 /*type=*/ NULL);
 
-                    object = pygobject_new (arg->v_pointer);
                     break;
                 default:
                     g_assert_not_reached();
diff --git a/tests/test_object_marshaling.py b/tests/test_object_marshaling.py
index 87f7043..e44feff 100644
--- a/tests/test_object_marshaling.py
+++ b/tests/test_object_marshaling.py
@@ -95,7 +95,6 @@ class TestVFuncsWithObjectArg(unittest.TestCase):
         Object = GObject.Object
         ObjectRef = weakref.ref
 
-    @unittest.expectedFailure  # bug 675726
     def test_vfunc_self_arg_ref_count(self):
         # Check to make sure vfunc "self" arguments don't leak.
         vfuncs = self.VFuncs()
@@ -162,7 +161,6 @@ class TestVFuncsWithObjectArg(unittest.TestCase):
         gc.collect()
         self.assertTrue(vfuncs.object_ref() is None)
 
-    @unittest.expectedFailure  # bug 675726
     def test_vfunc_in_object_transfer_none(self):
         vfuncs = self.VFuncs()
         ref_count, is_floating = vfuncs.get_ref_info_for_vfunc_in_object_transfer_none(self.VFuncs.Object)
@@ -252,7 +250,6 @@ class TestVFuncsWithFloatingArg(unittest.TestCase):
         gc.collect()
         self.assertTrue(vfuncs.object_ref() is None)
 
-    @unittest.expectedFailure  # bug 675726, should maintain floating ref
     def test_vfunc_in_object_transfer_none_with_floating(self):
         vfuncs = self.VFuncs()
         ref_count, is_floating = vfuncs.get_ref_info_for_vfunc_in_object_transfer_none(self.VFuncs.Object)
@@ -377,7 +374,7 @@ class TestVFuncsWithHeldObjectArg(unittest.TestCase):
 
         gc.collect()
 
-        # Ref count inside vfunc from the perpsective of Python
+        # Ref count inside vfunc from the perspective of Python
         self.assertEqual(vfuncs.in_object_grefcount, 2)  # initial + python wrapper
         self.assertFalse(vfuncs.in_object_is_floating)
 
@@ -400,7 +397,7 @@ class TestVFuncsWithHeldObjectArg(unittest.TestCase):
 
         gc.collect()
 
-        # Ref count inside vfunc from the perpsective of Python
+        # Ref count inside vfunc from the perspective of Python
         self.assertEqual(vfuncs.in_object_grefcount, 1)  # python wrapper takes ownership of the gobject
         self.assertFalse(vfuncs.in_object_is_floating)
 
@@ -502,7 +499,7 @@ class TestVFuncsWithHeldFloatingArg(unittest.TestCase):
         ref_count, is_floating = vfuncs.get_ref_info_for_vfunc_in_object_transfer_none(self.VFuncs.Object)
         gc.collect()
 
-        # Ref count inside vfunc from the perpsective of Python
+        # Ref count inside vfunc from the perspective of Python
         self.assertTrue(vfuncs.in_object_is_floating)
         self.assertEqual(vfuncs.in_object_grefcount, 2)  # python wrapper sinks and owns the gobject
 
@@ -538,3 +535,70 @@ class TestVFuncsWithHeldFloatingArg(unittest.TestCase):
         del vfuncs.object_ref
         gc.collect()
         self.assertTrue(held_object_ref() is None)
+
+
+class TestPropertyHoldingObject(unittest.TestCase):
+    @unittest.expectedFailure  # https://bugzilla.gnome.org/show_bug.cgi?id=675726
+    def test_props_getter_holding_object_ref_count(self):
+        holder = GIMarshallingTests.PropertiesObject()
+        held = GObject.Object()
+
+        self.assertEqual(holder.__grefcount__, 1)
+        self.assertEqual(held.__grefcount__, 1)
+
+        holder.set_property('some-object', held)
+        self.assertEqual(holder.__grefcount__, 1)
+
+        initial_ref_count = held.__grefcount__
+        holder.props.some_object
+        gc.collect()
+        self.assertEqual(held.__grefcount__, initial_ref_count)
+
+    def test_get_property_holding_object_ref_count(self):
+        holder = GIMarshallingTests.PropertiesObject()
+        held = GObject.Object()
+
+        self.assertEqual(holder.__grefcount__, 1)
+        self.assertEqual(held.__grefcount__, 1)
+
+        holder.set_property('some-object', held)
+        self.assertEqual(holder.__grefcount__, 1)
+
+        initial_ref_count = held.__grefcount__
+        holder.get_property('some-object')
+        gc.collect()
+        self.assertEqual(held.__grefcount__, initial_ref_count)
+
+    @unittest.expectedFailure  # https://bugzilla.gnome.org/show_bug.cgi?id=675726
+    def test_props_setter_holding_object_ref_count(self):
+        holder = GIMarshallingTests.PropertiesObject()
+        held = GObject.Object()
+
+        self.assertEqual(holder.__grefcount__, 1)
+        self.assertEqual(held.__grefcount__, 1)
+
+        # Setting property should only increase ref count by 1
+        holder.props.some_object = held
+        self.assertEqual(holder.__grefcount__, 1)
+        self.assertEqual(held.__grefcount__, 2)
+
+        # Clearing should pull it back down
+        holder.props.some_object = None
+        self.assertEqual(held.__grefcount__, 1)
+
+    @unittest.expectedFailure  # https://bugzilla.gnome.org/show_bug.cgi?id=675726
+    def test_set_property_holding_object_ref_count(self):
+        holder = GIMarshallingTests.PropertiesObject()
+        held = GObject.Object()
+
+        self.assertEqual(holder.__grefcount__, 1)
+        self.assertEqual(held.__grefcount__, 1)
+
+        # Setting property should only increase ref count by 1
+        holder.set_property('some-object', held)
+        self.assertEqual(holder.__grefcount__, 1)
+        self.assertEqual(held.__grefcount__, 2)
+
+        # Clearing should pull it back down
+        holder.set_property('some-object', None)
+        self.assertEqual(held.__grefcount__, 1)