[pygobject] Revert "Revert "Refactor boxed wrapper memory management strategy""

[Christoph Reiter <creiter src gnome org>]

commit a506d5e3c64321c43a4ce7c2a72ca8d36e985999
Author: Christoph Reiter <reiter christoph gmail com>
Date:   Tue Feb 13 23:26:21 2018 +0100

    Revert "Revert "Refactor boxed wrapper memory management strategy""
    
    This reverts commit daefdfa3e4dc97b4ae38250358d722f09764cc9b.

 gi/gimodule.c            |  6 +++-
 gi/overrides/GLib.py     |  4 ---
 gi/overrides/GObject.py  |  3 ++
 gi/pygi-boxed.c          | 32 +++++++++++++++-----
 gi/pygi-boxed.h          |  4 +--
 gi/pygi-property.c       |  6 +---
 gi/pygi-source.c         |  6 ++--
 gi/pygi-struct-marshal.c | 78 ++++++++++++++++++++++++++++++++++++++++++++----
 tests/test_gi.py         |  1 -
 tests/test_source.py     |  8 +++--
 10 files changed, 119 insertions(+), 29 deletions(-)
---
diff --git a/gi/gimodule.c b/gi/gimodule.c
index 48ddee26..5f8853c8 100644
--- a/gi/gimodule.c
+++ b/gi/gimodule.c
@@ -535,7 +535,11 @@ _wrap_pyg_variant_type_from_string (PyObject *self, PyObject *args)
 
     py_type = _pygi_type_import_by_name ("GLib", "VariantType");
 
-    py_variant = _pygi_boxed_new ( (PyTypeObject *) py_type, type_string, FALSE, 0);
+    /* Pass the string directly and force a boxed copy. This works because
+     * GVariantType is just a char pointer. */
+    py_variant = _pygi_boxed_new ( (PyTypeObject *) py_type, type_string,
+                                   TRUE, /* copy_boxed */
+                                   0);   /* slice_allocated */
 
     return py_variant;
 }
diff --git a/gi/overrides/GLib.py b/gi/overrides/GLib.py
index b1ff24f7..f47a338e 100644
--- a/gi/overrides/GLib.py
+++ b/gi/overrides/GLib.py
@@ -522,10 +522,6 @@ class Source(GLib.Source):
     def __init__(self, *args, **kwargs):
         return super(Source, self).__init__()
 
-    def __del__(self):
-        if hasattr(self, '__pygi_custom_source'):
-            self.unref()
-
     def set_callback(self, fn, user_data=None):
         if hasattr(self, '__pygi_custom_source'):
             # use our custom pyg_source_set_callback() if for a GSource object
diff --git a/gi/overrides/GObject.py b/gi/overrides/GObject.py
index bfcf7ccf..c252bfac 100644
--- a/gi/overrides/GObject.py
+++ b/gi/overrides/GObject.py
@@ -221,6 +221,9 @@ class Value(GObjectModule.Value):
         if self._free_on_dealloc and self.g_type != TYPE_INVALID:
             self.unset()
 
+        # We must call base class __del__() after unset.
+        super(Value, self).__del__()
+
     def set_boxed(self, boxed):
         # Workaround the introspection marshalers inability to know
         # these methods should be marshaling boxed types. This is because
diff --git a/gi/pygi-boxed.c b/gi/pygi-boxed.c
index fa690bb3..e9014f20 100644
--- a/gi/pygi-boxed.c
+++ b/gi/pygi-boxed.c
@@ -113,7 +113,7 @@ _boxed_new (PyTypeObject *type,
         goto out;
     }
 
-    self = (PyGIBoxed *) _pygi_boxed_new (type, boxed, TRUE, size);
+    self = (PyGIBoxed *) _pygi_boxed_new (type, boxed, FALSE, size);
     if (self == NULL) {
         g_slice_free1 (size, boxed);
         goto out;
@@ -149,30 +149,48 @@ _boxed_init (PyObject *self,
 PYGLIB_DEFINE_TYPE("gi.Boxed", PyGIBoxed_Type, PyGIBoxed);
 
 PyObject *
-_pygi_boxed_new (PyTypeObject *type,
+_pygi_boxed_new (PyTypeObject *pytype,
                  gpointer      boxed,
-                 gboolean      free_on_dealloc,
+                 gboolean      copy_boxed,
                  gsize         allocated_slice)
 {
     PyGIBoxed *self;
+    GType gtype;
 
     if (!boxed) {
         Py_RETURN_NONE;
     }
 
-    if (!PyType_IsSubtype (type, &PyGIBoxed_Type)) {
+    if (!PyType_IsSubtype (pytype, &PyGIBoxed_Type)) {
         PyErr_SetString (PyExc_TypeError, "must be a subtype of gi.Boxed");
         return NULL;
     }
 
-    self = (PyGIBoxed *) type->tp_alloc (type, 0);
+    gtype = pyg_type_from_object ((PyObject *)pytype);
+
+    /* Boxed objects with slice allocation means they come from caller allocated
+     * out arguments. In this case copy_boxed does not make sense because we
+     * already own the slice allocated memory and we should be receiving full
+     * ownership transfer. */
+    if (copy_boxed) {
+        g_assert (allocated_slice == 0);
+        boxed = g_boxed_copy (gtype, boxed);
+    }
+
+    self = (PyGIBoxed *) pytype->tp_alloc (pytype, 0);
     if (self == NULL) {
         return NULL;
     }
 
-    ( (PyGBoxed *) self)->gtype = pyg_type_from_object ( (PyObject *) type);
-    ( (PyGBoxed *) self)->free_on_dealloc = free_on_dealloc;
+    /* We always free on dealloc because we always own the memory due to:
+     *   1) copy_boxed == TRUE
+     *   2) allocated_slice > 0
+     *   3) otherwise the mode is assumed "transfer everything".
+     */
+    ((PyGBoxed *)self)->free_on_dealloc = TRUE;
+    ((PyGBoxed *)self)->gtype = gtype;
     pyg_boxed_set_ptr (self, boxed);
+
     if (allocated_slice > 0) {
         self->size = allocated_slice;
         self->slice_allocated = TRUE;
diff --git a/gi/pygi-boxed.h b/gi/pygi-boxed.h
index 5c04b5c9..86793224 100644
--- a/gi/pygi-boxed.h
+++ b/gi/pygi-boxed.h
@@ -34,9 +34,9 @@ typedef struct {
 
 extern PyTypeObject PyGIBoxed_Type;
 
-PyObject * _pygi_boxed_new (PyTypeObject *type,
+PyObject * _pygi_boxed_new (PyTypeObject *pytype,
                             gpointer      boxed,
-                            gboolean      free_on_dealloc,
+                            gboolean      copy_boxed,
                             gsize         allocated_slice);
 
 void * _pygi_boxed_alloc (GIBaseInfo *info,
diff --git a/gi/pygi-property.c b/gi/pygi-property.c
index c4f3e4a3..9978585f 100644
--- a/gi/pygi-property.c
+++ b/gi/pygi-property.c
@@ -160,7 +160,6 @@ pygi_get_property_value (PyGObject *instance, GParamSpec *pspec)
         GITypeInfo *type_info = NULL;
         gboolean free_array = FALSE;
         GIArgument arg = { 0, };
-        GITransfer transfer = GI_TRANSFER_NOTHING;
 
         type_info = g_property_info_get_type (property_info);
         arg = _pygi_argument_from_g_value (&value, type_info);
@@ -169,12 +168,9 @@ pygi_get_property_value (PyGObject *instance, GParamSpec *pspec)
         if (g_type_info_get_tag (type_info) == GI_TYPE_TAG_ARRAY) {
             arg.v_pointer = _pygi_argument_to_array (&arg, NULL, NULL, NULL,
                                                      type_info, &free_array);
-        } else if (g_type_is_a (pspec->value_type, G_TYPE_BOXED)) {
-          arg.v_pointer = g_value_dup_boxed (&value);
-          transfer = GI_TRANSFER_EVERYTHING;
         }
 
-        py_value = _pygi_argument_to_object (&arg, type_info, transfer);
+        py_value = _pygi_argument_to_object (&arg, type_info, GI_TRANSFER_NOTHING);
 
         if (free_array) {
             g_array_free (arg.v_pointer, FALSE);
diff --git a/gi/pygi-source.c b/gi/pygi-source.c
index 2ed38d37..5305260b 100644
--- a/gi/pygi-source.c
+++ b/gi/pygi-source.c
@@ -241,8 +241,10 @@ pyg_source_new (void)
     source = (PyGRealSource*) g_source_new (&pyg_source_funcs, sizeof (PyGRealSource));
 
     py_type = _pygi_type_import_by_name ("GLib", "Source");
-    /* g_source_new uses malloc, not slices */
-    source->obj = _pygi_boxed_new ( (PyTypeObject *) py_type, source, FALSE, 0);
+    /* Full ownership transfer of the source, this will be free'd with g_boxed_free. */
+    source->obj = _pygi_boxed_new ( (PyTypeObject *) py_type, source,
+                                    FALSE, /* copy_boxed */
+                                    0);    /* slice_allocated */
 
     return source->obj;
 }
diff --git a/gi/pygi-struct-marshal.c b/gi/pygi-struct-marshal.c
index d594239f..60d2585a 100644
--- a/gi/pygi-struct-marshal.c
+++ b/gi/pygi-struct-marshal.c
@@ -388,9 +388,11 @@ pygi_arg_struct_to_py_marshal (GIArgument *arg,
                                                               arg->v_pointer);
     } else if (g_type_is_a (g_type, G_TYPE_BOXED)) {
         if (py_type) {
+            /* Force a boxed copy if we are not transfered ownership and the
+             * memory is not caller allocated. */
             py_obj = _pygi_boxed_new ((PyTypeObject *) py_type,
                                       arg->v_pointer,
-                                      transfer == GI_TRANSFER_EVERYTHING || is_allocated,
+                                      transfer == GI_TRANSFER_NOTHING && !is_allocated,
                                       is_allocated ?
                                               g_struct_info_get_size(interface_info) : 0);
         }
@@ -448,6 +450,36 @@ arg_struct_to_py_marshal_adapter (PyGIInvokeState   *state,
                                           iface_cache->is_foreign);
 }
 
+static PyObject *
+arg_boxed_to_py_marshal_pass_by_ref (PyGIInvokeState   *state,
+                                     PyGICallableCache *callable_cache,
+                                     PyGIArgCache      *arg_cache,
+                                     GIArgument        *arg)
+{
+    PyObject *py_obj = NULL;
+    PyGIInterfaceCache *iface_cache = (PyGIInterfaceCache *)arg_cache;
+
+    if (arg->v_pointer == NULL) {
+        Py_RETURN_NONE;
+    }
+
+    if (g_type_is_a (iface_cache->g_type, G_TYPE_BOXED)) {
+        if (iface_cache->py_type) {
+            py_obj = _pygi_boxed_new ((PyTypeObject *) iface_cache->py_type,
+                                      arg->v_pointer,
+                                      FALSE, /* copy_boxed */
+                                      0);    /* slice_alloc */
+            ((PyGBoxed *)py_obj)->free_on_dealloc = FALSE;
+        }
+    } else {
+        PyErr_Format (PyExc_NotImplementedError,
+                      "expected boxed type but got %s",
+                      g_type_name (iface_cache->g_type));
+    }
+
+    return py_obj;
+}
+
 static void
 arg_foreign_to_py_cleanup (PyGIInvokeState *state,
                            PyGIArgCache    *arg_cache,
@@ -529,16 +561,52 @@ arg_struct_from_py_setup (PyGIArgCache     *arg_cache,
 static void
 arg_struct_to_py_setup (PyGIArgCache     *arg_cache,
                         GIInterfaceInfo  *iface_info,
-                        GITransfer        transfer)
+                        GITransfer        transfer,
+                        GIArgInfo        *arg_info)
 {
     PyGIInterfaceCache *iface_cache = (PyGIInterfaceCache *)arg_cache;
 
+    /* HACK to force GtkTreeModel:iter_next() and iter_previous() vfunc implementations
+     * to receive their Gtk.TreeIter argument as pass-by-reference. We create a new
+     * PyGIBoxed wrapper which does not copy the memory and also does not free it.
+     * This is needed to hack the noted vfunc implementations so they can continue
+     * working with bug https://bugzilla.gnome.org/show_bug.cgi?id=722899
+     * being fixed. This hack should be removed once GTK+ has fixed bug
+     * https://bugzilla.gnome.org/show_bug.cgi?id=734465
+     * and we've moved to a new major version.
+     */
+    if (arg_info && g_strcmp0 (iface_cache->type_name, "Gtk.TreeIter") == 0) {
+
+        /* GICallbackInfo */
+        GIBaseInfo *info = g_base_info_get_container (arg_info);
+        if (info && g_base_info_get_type (info) == GI_INFO_TYPE_CALLBACK &&
+                (g_strcmp0 (g_base_info_get_name (info), "iter_next") == 0 ||
+                 g_strcmp0 (g_base_info_get_name (info), "iter_previous") == 0)) {
+
+            /* GITypeInfo */
+            info = g_base_info_get_container (info);
+            if (info && g_base_info_get_type (info) == GI_INFO_TYPE_TYPE &&
+                    g_type_info_get_tag ((GITypeInfo *)info) == GI_TYPE_TAG_INTERFACE) {
+
+                /* GIFieldInfo */
+                info = g_base_info_get_container (info);
+                if (info && g_base_info_get_type (info) == GI_INFO_TYPE_FIELD) {
+
+                    /* GIStructInfo */
+                    info = g_base_info_get_container (info);
+                    if (info && g_base_info_get_type (info) == GI_INFO_TYPE_STRUCT &&
+                            g_strcmp0 (g_base_info_get_name (info), "TreeModelIface") == 0) {
+                        arg_cache->to_py_marshaller = arg_boxed_to_py_marshal_pass_by_ref;
+                    }
+                }
+            }
+        }
+    }
+
     if (arg_cache->to_py_marshaller == NULL) {
         arg_cache->to_py_marshaller = arg_struct_to_py_marshal_adapter;
     }
 
-    iface_cache->is_foreign = g_struct_info_is_foreign ( (GIStructInfo*)iface_info);
-
     if (iface_cache->is_foreign)
         arg_cache->to_py_cleanup = arg_foreign_to_py_cleanup;
 }
@@ -570,7 +638,7 @@ pygi_arg_struct_new_from_info (GITypeInfo      *type_info,
     }
 
     if (direction & PYGI_DIRECTION_TO_PYTHON) {
-        arg_struct_to_py_setup (cache, iface_info, transfer);
+        arg_struct_to_py_setup (cache, iface_info, transfer, arg_info);
     }
 
     return cache;
diff --git a/tests/test_gi.py b/tests/test_gi.py
index 8d54004e..31085206 100644
--- a/tests/test_gi.py
+++ b/tests/test_gi.py
@@ -2562,7 +2562,6 @@ class TestPythonGObject(unittest.TestCase):
 
     @unittest.skipUnless(hasattr(GIMarshallingTests, 'callback_owned_boxed'),
                          'requires newer version of GI')
-    @unittest.expectedFailure  # bug 722899
     def test_callback_owned_box(self):
         def callback(box, data):
             self.box = box
diff --git a/tests/test_source.py b/tests/test_source.py
index 64fe5bd2..5828fa90 100644
--- a/tests/test_source.py
+++ b/tests/test_source.py
@@ -3,6 +3,7 @@
 from __future__ import absolute_import
 
 import sys
+import gc
 import unittest
 import warnings
 
@@ -128,6 +129,7 @@ class TestSource(unittest.TestCase):
             return s
 
         s = f()
+        gc.collect()
         self.assertTrue(s.is_destroyed())
 
     def test_remove(self):
@@ -209,8 +211,9 @@ class TestSource(unittest.TestCase):
                 self.finalized = True
 
         source = S()
-        id = source.attach()
-        print('source id:', id)
+        self.assertEqual(source.ref_count, 1)
+        source.attach()
+        self.assertEqual(source.ref_count, 2)
         self.assertFalse(self.finalized)
         self.assertFalse(source.is_destroyed())
 
@@ -218,6 +221,7 @@ class TestSource(unittest.TestCase):
             pass
 
         source.destroy()
+        self.assertEqual(source.ref_count, 1)
         self.assertTrue(self.dispatched)
         self.assertFalse(self.finalized)
         self.assertTrue(source.is_destroyed())