source: Fix source finalization in some corner casesbenzea/gsource-fix2

GLib will call the finalize handler when the GSource is destroyed. We
already enforce destruction from __del__, however, the final reference
might only be dropped later on because the main context (or something
else) is holding another reference. The main case where this happens is
if the source is pending at the time, e.g. if it is being destroyed
while it is dispatching.

After destroy is called on the source, the only callback that can still
be called is finalize. As such, we can simply call finalize immediately
and rely on GLib cleaning up the underlying GSource at a later point.

Note that it can also happen that __del__ is not called at all if the
interpreter quits. In this case, neither __del__ nor finalize will be
called even though the underlying GSource may be finalized correctly.
Compared to the previous behaviour this is a big improvement, as it
would crash previously.

3 files changed, 45 insertions, 30 deletions

diff --git a/gi/overrides/GLib.py b/gi/overrides/GLib.py
index 78d309b6..0425d50f 100644
--- a/gi/overrides/GLib.py
+++ b/gi/overrides/GLib.py
@@ -525,12 +525,16 @@ class Source(GLib.Source):
 
     def __del__(self):
         if hasattr(self, '__pygi_custom_source'):
+            # We destroy and finalize the box from here, as GLib might hold
+            # a reference (e.g. while the source is pending), delaying the
+            # finalize call until a later point.
             self.destroy()
-            # XXX: We have to unref the underlying source while the Python
-            # wrapper is still valid, so the source can call into the
-            # wrapper methods for the finalized callback.
+            self.finalize()
             self._clear_boxed()
 
+    def finalize(self):
+        pass
+
     def set_callback(self, fn, user_data=None):
         if hasattr(self, '__pygi_custom_source'):
             # use our custom pygi_source_set_callback() if for a GSource object
diff --git a/gi/pygi-source.c b/gi/pygi-source.c
index c85386d7..5de8a381 100644
--- a/gi/pygi-source.c
+++ b/gi/pygi-source.c
@@ -149,38 +149,12 @@ source_dispatch(GSource *source, GSourceFunc callback, gpointer user_data)
     return ret;
 }
 
-static void
-source_finalize(GSource *source)
-{
-    PyGRealSource *pysource = (PyGRealSource *)source;
-    PyObject *func, *t;
-    PyGILState_STATE state;
-
-    state = PyGILState_Ensure();
-
-    func = PyObject_GetAttrString(pysource->obj, "finalize");
-    if (func) {
-	t = PyObject_CallObject(func, NULL);
-	Py_DECREF(func);
-
-	if (t == NULL) {
-	    PyErr_Print();
-	} else {
-	    Py_DECREF(t);
-	}
-    } else {
-        PyErr_Clear ();
-    }
-
-    PyGILState_Release(state);
-}
-
 static GSourceFuncs pyg_source_funcs =
 {
     source_prepare,
     source_check,
     source_dispatch,
-    source_finalize
+    NULL
 };
 
 /**
diff --git a/tests/test_source.py b/tests/test_source.py
index 9249892e..40e09cd1 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 self.finalized
+        assert self.dispatched
+
     def test_extra_init_args(self):
         class SourceWithInitArgs(GLib.Source):
             def __init__(self, arg, kwarg=None):