source: Fix destruction in certain corner casesbenzea/gsource-fix

The GSource destructor assumed that __del__ would clean up the object
and also cause finalize to be called. However, this may not be true in
two scenarios:
 1. On abnormal exit, __del__ might not be called
 2. The source is pending (or being dispatched)
    as the context holds a reference

The fix used here is to make sure to prevent calling the finalize
handler if the object is already destroyed or is being destroyed.

In the abnormal exit case (1), this is reasonable to do. The behaviour
is more problematic for the case where destruction happens while the
source is pending (2). In that case, a custom finalize handler will not
be called as the corresponding python object will be destroyed already.

6 files changed, 57 insertions, 6 deletions

diff --git a/gi/gimodule.c b/gi/gimodule.c
index 766fd341..657e745d 100644
--- a/gi/gimodule.c
+++ b/gi/gimodule.c
@@ -1926,7 +1926,7 @@ _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);
+    py_variant = pygi_boxed_new ( (PyTypeObject *) py_type, type_string, FALSE, 0, NULL);
 
     return py_variant;
 }
diff --git a/gi/pygi-boxed.c b/gi/pygi-boxed.c
index 9deb62a7..016660b9 100644
--- a/gi/pygi-boxed.c
+++ b/gi/pygi-boxed.c
@@ -32,6 +32,7 @@ struct _PyGIBoxed {
     PyGBoxed base;
     gboolean slice_allocated;
     gsize size;
+    gpointer *weak_ref;
 };
 
 static void
@@ -52,6 +53,11 @@ boxed_clear (PyGIBoxed *self)
         }
     }
     pyg_boxed_set_ptr (self, NULL);
+
+    if (self->weak_ref) {
+        *self->weak_ref = NULL;
+        self->weak_ref = NULL;
+    }
 }
 
 static PyObject *
@@ -132,7 +138,7 @@ boxed_new (PyTypeObject *type,
         goto out;
     }
 
-    self = (PyGIBoxed *) pygi_boxed_new (type, boxed, TRUE, size);
+    self = (PyGIBoxed *) pygi_boxed_new (type, boxed, TRUE, size, NULL);
     if (self == NULL) {
         g_slice_free1 (size, boxed);
         goto out;
@@ -171,7 +177,8 @@ PyObject *
 pygi_boxed_new (PyTypeObject *type,
                 gpointer      boxed,
                 gboolean      free_on_dealloc,
-                gsize         allocated_slice)
+                gsize         allocated_slice,
+                gpointer     *weak_ref)
 {
     PyGIBoxed *self;
 
@@ -199,6 +206,7 @@ pygi_boxed_new (PyTypeObject *type,
         self->size = 0;
         self->slice_allocated = FALSE;
     }
+    self->weak_ref = weak_ref;
 
     return (PyObject *) self;
 }
diff --git a/gi/pygi-boxed.h b/gi/pygi-boxed.h
index 18c44483..145c3aa0 100644
--- a/gi/pygi-boxed.h
+++ b/gi/pygi-boxed.h
@@ -33,7 +33,8 @@ extern PyTypeObject PyGIBoxed_Type;
 PyObject * pygi_boxed_new (PyTypeObject *type,
                            gpointer      boxed,
                            gboolean      free_on_dealloc,
-                           gsize         allocated_slice);
+                           gsize         allocated_slice,
+                           gpointer     *weak_ref);
 
 void * pygi_boxed_alloc (GIBaseInfo *info, gsize *size);
 
diff --git a/gi/pygi-source.c b/gi/pygi-source.c
index c85386d7..d9ba0399 100644
--- a/gi/pygi-source.c
+++ b/gi/pygi-source.c
@@ -158,6 +158,9 @@ source_finalize(GSource *source)
 
     state = PyGILState_Ensure();
 
+    if (!pysource->obj || Py_REFCNT(pysource->obj) == 0)
+        goto out;
+
     func = PyObject_GetAttrString(pysource->obj, "finalize");
     if (func) {
 	t = PyObject_CallObject(func, NULL);
@@ -172,6 +175,7 @@ source_finalize(GSource *source)
         PyErr_Clear ();
     }
 
+out:
     PyGILState_Release(state);
 }
 
@@ -296,7 +300,7 @@ pygi_source_new (PyObject *self, PyObject *args)
 
     source = (PyGRealSource*) g_source_new (&pyg_source_funcs, sizeof (PyGRealSource));
     /* g_source_new uses malloc, not slices */
-    boxed = pygi_boxed_new ( (PyTypeObject *) py_type, source, TRUE, 0);
+    boxed = pygi_boxed_new ( (PyTypeObject *) py_type, source, TRUE, 0, (gpointer*)&source->obj);
     Py_DECREF (py_type);
     if (!boxed) {
         g_source_unref ((GSource *)source);
diff --git a/gi/pygi-struct-marshal.c b/gi/pygi-struct-marshal.c
index a3eb2802..e5706fdf 100644
--- a/gi/pygi-struct-marshal.c
+++ b/gi/pygi-struct-marshal.c
@@ -423,7 +423,8 @@ pygi_arg_struct_to_py_marshaller (GIArgument *arg,
                                      arg->v_pointer,
                                      transfer == GI_TRANSFER_EVERYTHING || is_allocated,
                                      is_allocated ?
-                                            g_struct_info_get_size(interface_info) : 0);
+                                            g_struct_info_get_size(interface_info) : 0,
+                                     NULL);
         }
     } else if (g_type_is_a (g_type, G_TYPE_POINTER)) {
         if (py_type == NULL ||
diff --git a/tests/test_source.py b/tests/test_source.py
index 9249892e..4f0c5632 100644
--- a/tests/test_source.py
+++ b/tests/test_source.py
@@ -264,6 +264,43 @@ class TestSource(unittest.TestCase):
         assert not self.dispatched
         assert context.find_source_by_id(id_) is None
 
+    def test_python_unref_during_dispatch(self):
+        # Tests a Python derived Source which is free'd in the context of
+        # Python, while being dispatched
+        # i.e. finalize is never called as the python object is destroyed
+        self.dispatched = False
+        self.finalized = False
+
+        class S(GLib.Source):
+            def __init__(s, func=None):
+                s.func = func
+
+            def prepare(s):
+                return (True, 1)
+
+            def check(s):
+                pass
+
+            def dispatch(s, callback, args):
+                self.dispatched = True
+                self.source = None
+                return False
+
+            def finalize(s):
+                self.finalized = True
+
+        context = GLib.MainContext.new()
+        self.source = S()
+        id_ = self.source.attach(context)
+
+        while context.iteration(may_block=False):
+            pass
+
+        assert self.source is None
+        assert context.find_source_by_id(id_) is None
+        assert not self.finalized
+        assert self.dispatched
+
     def test_extra_init_args(self):
         class SourceWithInitArgs(GLib.Source):
             def __init__(self, arg, kwarg=None):