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

[Christoph Reiter <creiter src gnome org>]

commit d08e244d011121f8ffd5c21437e0f11fe9545bbf
Author: Mathieu Duponchelle <mathieu centricular com>
Date:   Thu Feb 15 23:32:08 2018 +0100

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

 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, 29 insertions(+), 119 deletions(-)
---
diff --git a/gi/gimodule.c b/gi/gimodule.c
index 5f8853c8..48ddee26 100644
--- a/gi/gimodule.c
+++ b/gi/gimodule.c
@@ -535,11 +535,7 @@ _wrap_pyg_variant_type_from_string (PyObject *self, PyObject *args)
 
     py_type = _pygi_type_import_by_name ("GLib", "VariantType");
 
-    /* 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 */
+    py_variant = _pygi_boxed_new ( (PyTypeObject *) py_type, type_string, FALSE, 0);
 
     return py_variant;
 }
diff --git a/gi/overrides/GLib.py b/gi/overrides/GLib.py
index f47a338e..b1ff24f7 100644
--- a/gi/overrides/GLib.py
+++ b/gi/overrides/GLib.py
@@ -522,6 +522,10 @@ 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 c252bfac..bfcf7ccf 100644
--- a/gi/overrides/GObject.py
+++ b/gi/overrides/GObject.py
@@ -221,9 +221,6 @@ 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 e9014f20..fa690bb3 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, FALSE, size);
+    self = (PyGIBoxed *) _pygi_boxed_new (type, boxed, TRUE, size);
     if (self == NULL) {
         g_slice_free1 (size, boxed);
         goto out;
@@ -149,48 +149,30 @@ _boxed_init (PyObject *self,
 PYGLIB_DEFINE_TYPE("gi.Boxed", PyGIBoxed_Type, PyGIBoxed);
 
 PyObject *
-_pygi_boxed_new (PyTypeObject *pytype,
+_pygi_boxed_new (PyTypeObject *type,
                  gpointer      boxed,
-                 gboolean      copy_boxed,
+                 gboolean      free_on_dealloc,
                  gsize         allocated_slice)
 {
     PyGIBoxed *self;
-    GType gtype;
 
     if (!boxed) {
         Py_RETURN_NONE;
     }
 
-    if (!PyType_IsSubtype (pytype, &PyGIBoxed_Type)) {
+    if (!PyType_IsSubtype (type, &PyGIBoxed_Type)) {
         PyErr_SetString (PyExc_TypeError, "must be a subtype of gi.Boxed");
         return NULL;
     }
 
-    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);
+    self = (PyGIBoxed *) type->tp_alloc (type, 0);
     if (self == NULL) {
         return NULL;
     }
 
-    /* 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;
+    ( (PyGBoxed *) self)->gtype = pyg_type_from_object ( (PyObject *) type);
+    ( (PyGBoxed *) self)->free_on_dealloc = free_on_dealloc;
     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 86793224..5c04b5c9 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 *pytype,
+PyObject * _pygi_boxed_new (PyTypeObject *type,
                             gpointer      boxed,
-                            gboolean      copy_boxed,
+                            gboolean      free_on_dealloc,
                             gsize         allocated_slice);
 
 void * _pygi_boxed_alloc (GIBaseInfo *info,
diff --git a/gi/pygi-property.c b/gi/pygi-property.c
index 9978585f..c4f3e4a3 100644
--- a/gi/pygi-property.c
+++ b/gi/pygi-property.c
@@ -160,6 +160,7 @@ 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);
@@ -168,9 +169,12 @@ 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, GI_TRANSFER_NOTHING);
+        py_value = _pygi_argument_to_object (&arg, type_info, transfer);
 
         if (free_array) {
             g_array_free (arg.v_pointer, FALSE);
diff --git a/gi/pygi-source.c b/gi/pygi-source.c
index 5305260b..2ed38d37 100644
--- a/gi/pygi-source.c
+++ b/gi/pygi-source.c
@@ -241,10 +241,8 @@ pyg_source_new (void)
     source = (PyGRealSource*) g_source_new (&pyg_source_funcs, sizeof (PyGRealSource));
 
     py_type = _pygi_type_import_by_name ("GLib", "Source");
-    /* 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 */
+    /* g_source_new uses malloc, not slices */
+    source->obj = _pygi_boxed_new ( (PyTypeObject *) py_type, source, FALSE, 0);
 
     return source->obj;
 }
diff --git a/gi/pygi-struct-marshal.c b/gi/pygi-struct-marshal.c
index 60d2585a..d594239f 100644
--- a/gi/pygi-struct-marshal.c
+++ b/gi/pygi-struct-marshal.c
@@ -388,11 +388,9 @@ 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_NOTHING && !is_allocated,
+                                      transfer == GI_TRANSFER_EVERYTHING || is_allocated,
                                       is_allocated ?
                                               g_struct_info_get_size(interface_info) : 0);
         }
@@ -450,36 +448,6 @@ 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,
@@ -561,52 +529,16 @@ arg_struct_from_py_setup (PyGIArgCache     *arg_cache,
 static void
 arg_struct_to_py_setup (PyGIArgCache     *arg_cache,
                         GIInterfaceInfo  *iface_info,
-                        GITransfer        transfer,
-                        GIArgInfo        *arg_info)
+                        GITransfer        transfer)
 {
     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;
 }
@@ -638,7 +570,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_info);
+        arg_struct_to_py_setup (cache, iface_info, transfer);
     }
 
     return cache;
diff --git a/tests/test_gi.py b/tests/test_gi.py
index 31085206..8d54004e 100644
--- a/tests/test_gi.py
+++ b/tests/test_gi.py
@@ -2562,6 +2562,7 @@ 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 5828fa90..64fe5bd2 100644
--- a/tests/test_source.py
+++ b/tests/test_source.py
@@ -3,7 +3,6 @@
 from __future__ import absolute_import
 
 import sys
-import gc
 import unittest
 import warnings
 
@@ -129,7 +128,6 @@ class TestSource(unittest.TestCase):
             return s
 
         s = f()
-        gc.collect()
         self.assertTrue(s.is_destroyed())
 
     def test_remove(self):
@@ -211,9 +209,8 @@ class TestSource(unittest.TestCase):
                 self.finalized = True
 
         source = S()
-        self.assertEqual(source.ref_count, 1)
-        source.attach()
-        self.assertEqual(source.ref_count, 2)
+        id = source.attach()
+        print('source id:', id)
         self.assertFalse(self.finalized)
         self.assertFalse(source.is_destroyed())
 
@@ -221,7 +218,6 @@ 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())