[pygobject] Fix memory management problems with struct arguments to signals

[Simon Feltman <sfeltman src gnome org>]

commit d70b300c7415dd7b20ff88b09fe835690da19831
Author: Simon Feltman <sfeltman src gnome org>
Date:   Sat Sep 6 23:58:25 2014 -0700

    Fix memory management problems with struct arguments to signals
    
    Replicate struct marshaling logic for determining if struct arguments
    to signals should be passed by reference to callbacks.
    Maintain a list of these structs and apply an in-place copy of the struct
    pointer if the struct wrapper is held longer than the duration of the
    Python callback. This allows for both mutation of struct arguments from
    callbacks as well as memory safety incase a callbacks holds onto the struct.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=736175

 gi/pygi-boxed.c             |   20 ++++++++++++++++++++
 gi/pygi-boxed.h             |    2 ++
 gi/pygi-signal-closure.c    |   43 +++++++++++++++++++++++++++++++++++++------
 tests/test_overrides_gtk.py |    1 -
 tests/test_signal.py        |   27 +++++++++++++++++++++++++++
 5 files changed, 86 insertions(+), 7 deletions(-)
---
diff --git a/gi/pygi-boxed.c b/gi/pygi-boxed.c
index a1494b6..d5faeb1 100644
--- a/gi/pygi-boxed.c
+++ b/gi/pygi-boxed.c
@@ -206,6 +206,26 @@ _pygi_boxed_get_free_on_dealloc(PyGIBoxed *self, void *closure)
   return PyBool_FromLong( ((PyGBoxed *)self)->free_on_dealloc );
 }
 
+/**
+ * _pygi_boxed_copy_in_place:
+ *
+ * Replace the boxed pointer held by this wrapper with a boxed copy
+ * freeing the previously held pointer (when free_on_dealloc is TRUE).
+ * This can be used in cases where Python is passed a reference which
+ * it does not own and the wrapper is held by the Python program
+ * longer than the duration of a callback it was passed to.
+ */
+void
+_pygi_boxed_copy_in_place (PyGIBoxed *self)
+{
+    PyGBoxed *pygboxed = (PyGBoxed *)self;
+    gpointer copy = g_boxed_copy (pygboxed->gtype, pyg_boxed_get_ptr (self));
+
+    boxed_del (self);
+    pyg_boxed_set_ptr (pygboxed, copy);
+    pygboxed->free_on_dealloc = TRUE;
+}
+
 static PyGetSetDef pygi_boxed_getsets[] = {
     { "_free_on_dealloc", (getter)_pygi_boxed_get_free_on_dealloc, (setter)0 },
     { NULL, 0, 0 }
diff --git a/gi/pygi-boxed.h b/gi/pygi-boxed.h
index 189a02c..c8f40b7 100644
--- a/gi/pygi-boxed.h
+++ b/gi/pygi-boxed.h
@@ -34,6 +34,8 @@ PyObject * _pygi_boxed_new (PyTypeObject *pytype,
 void * _pygi_boxed_alloc (GIBaseInfo *info,
                           gsize *size);
 
+void _pygi_boxed_copy_in_place  (PyGIBoxed *self);
+
 void _pygi_boxed_register_types (PyObject *m);
 
 G_END_DECLS
diff --git a/gi/pygi-signal-closure.c b/gi/pygi-signal-closure.c
index 3cf8486..652ac99 100644
--- a/gi/pygi-signal-closure.c
+++ b/gi/pygi-signal-closure.c
@@ -79,6 +79,8 @@ pygi_signal_closure_marshal(GClosure *closure,
     GISignalInfo *signal_info;
     gint n_sig_info_args;
     gint sig_info_highest_arg;
+    GSList *list_item = NULL;
+    GSList *pass_by_ref_structs = NULL;
 
     state = PyGILState_Ensure();
 
@@ -131,20 +133,28 @@ pygi_signal_closure_marshal(GClosure *closure,
                                                          &free_array);
             }
 
-            /* Hack to ensure struct output args are passed-by-reference allowing
+            /* Hack to ensure struct arguments are passed-by-reference allowing
              * callback implementors to modify the struct values. This is needed
              * for keeping backwards compatibility and should be removed in future
              * versions which support signal output arguments as return values.
              * See: https://bugzilla.gnome.org/show_bug.cgi?id=735486
+             *
+             * Note the logic here must match the logic path taken in _pygi_argument_to_object.
              */
-            if (type_tag == GI_TYPE_TAG_INTERFACE &&
-                    g_arg_info_get_direction (&arg_info) == GI_DIRECTION_OUT) {
+            if (type_tag == GI_TYPE_TAG_INTERFACE) {
                 GIBaseInfo *info = g_type_info_get_interface (&type_info);
                 GIInfoType info_type = g_base_info_get_type (info);
 
-                if (info_type == GI_INFO_TYPE_STRUCT) {
+                if (info_type == GI_INFO_TYPE_STRUCT ||
+                        info_type == GI_INFO_TYPE_BOXED ||
+                        info_type == GI_INFO_TYPE_UNION) {
+
                     GType gtype = g_registered_type_info_get_g_type ((GIRegisteredTypeInfo *) info);
-                    if (g_type_is_a (gtype, G_TYPE_BOXED)) {
+                    gboolean is_foreign = (info_type == GI_INFO_TYPE_STRUCT) &&
+                                          (g_struct_info_is_foreign ((GIStructInfo *) info));
+
+                    if (!is_foreign && !g_type_is_a (gtype, G_TYPE_VALUE) &&
+                            g_type_is_a (gtype, G_TYPE_BOXED)) {
                         pass_struct_by_ref = TRUE;
                     }
                 }
@@ -153,8 +163,12 @@ pygi_signal_closure_marshal(GClosure *closure,
             }
 
             if (pass_struct_by_ref) {
+                /* transfer everything will ensure the struct is not copied when wrapped. */
                 item = _pygi_argument_to_object (&arg, &type_info, GI_TRANSFER_EVERYTHING);
-                ((PyGBoxed *)item)->free_on_dealloc = FALSE;
+                if (item && PyObject_IsInstance (item, (PyObject *) &PyGIBoxed_Type)) {
+                    ((PyGBoxed *)item)->free_on_dealloc = FALSE;
+                    pass_by_ref_structs = g_slist_prepend (pass_by_ref_structs, item);
+                }
 
             } else {
                 item = _pygi_argument_to_object (&arg, &type_info, GI_TRANSFER_NOTHING);
@@ -196,7 +210,24 @@ pygi_signal_closure_marshal(GClosure *closure,
     }
     Py_DECREF(ret);
 
+    /* Run through the list of structs which have been passed by reference and
+     * check if they are being held longer than the duration of the callback
+     * execution. This is determined if the ref count is greater than 1.
+     * A single ref is held by the argument list and any more would mean the callback
+     * stored a ref somewhere else. In this case we make an internal copy of
+     * the boxed struct so Python can own the memory to it.
+     */
+    list_item = pass_by_ref_structs;
+    while (list_item) {
+        PyObject *item = list_item->data;
+        if (item->ob_refcnt > 1) {
+            _pygi_boxed_copy_in_place ((PyGIBoxed *)item);
+        }
+        list_item = g_slist_next (list_item);
+    }
+
  out:
+    g_slist_free (pass_by_ref_structs);
     Py_DECREF(params);
     PyGILState_Release(state);
 }
diff --git a/tests/test_overrides_gtk.py b/tests/test_overrides_gtk.py
index 711382d..d3351d4 100644
--- a/tests/test_overrides_gtk.py
+++ b/tests/test_overrides_gtk.py
@@ -1858,7 +1858,6 @@ class TestTextBuffer(unittest.TestCase):
         self.assertEqual(start.get_offset(), 6)
         self.assertEqual(end.get_offset(), 11)
 
-    @unittest.expectedFailure  # https://bugzilla.gnome.org/show_bug.cgi?id=736175
     def test_insert_text_signal_location_modification(self):
         # Regression test for: https://bugzilla.gnome.org/show_bug.cgi?id=736175
 
diff --git a/tests/test_signal.py b/tests/test_signal.py
index 21d17c5..40cfb4f 100644
--- a/tests/test_signal.py
+++ b/tests/test_signal.py
@@ -1206,6 +1206,33 @@ class TestIntrospectedSignals(unittest.TestCase):
         self.assertEqual(obj.emit('sig-with-array-prop', [1, 2, GObject.G_MAXUINT]), None)
         self.assertEqual(obj.callback_arr, [1, 2, GObject.G_MAXUINT])
 
+    def test_held_struct_ref(self):
+        held_structs = []
+
+        def callback(obj, struct):
+            # The struct held by Python will become a copy after this callback exits.
+            struct.some_int = 42
+            struct.some_int8 = 42
+            held_structs.append(struct)
+
+        struct = Regress.TestSimpleBoxedA()
+        obj = Regress.TestObj()
+
+        self.assertEqual(struct.some_int, 0)
+        self.assertEqual(struct.some_int8, 0)
+
+        obj.connect('test-with-static-scope-arg', callback)
+        obj.emit('test-with-static-scope-arg', struct)
+
+        # The held struct will be a copy of the modified struct.
+        self.assertEqual(len(held_structs), 1)
+        held_struct = held_structs[0]
+        self.assertEqual(held_struct.some_int, 42)
+        self.assertEqual(held_struct.some_int8, 42)
+
+        # Boxed equality checks pointers by default.
+        self.assertNotEqual(struct, held_struct)
+
 
 class _ConnectObjectTestBase(object):
     # Notes: