From 17b4ba8707a8c6c24cd52e59a1f107dccfa9fd55 Mon Sep 17 00:00:00 2001
From: Christoph Reiter <creiter@src.gnome.org>
Date: Thu, 26 Oct 2017 17:24:02 +0200
Subject: pygobject-object: Fix Python GC collecting a ref cycle too early

PyGObject traverses its closures in tp_traverse, but the lifetime of the closures
is tied to the lifetime of the GObject and not the wrapper. This confuses
the Python GC when it sees a ref cycle and tries to break it up with tp_clear.
Since tp_clear will not invalidate the closure and only invalidate the Python
wrapper the closure callback gets called with the now cleared/invalid object.

Instead let the GC only check the Python objects referenced by the closure when tp_clear
would actually free them and as a result break the cycle. This is only the case when
the wrapped object would be freed by tp_clear which is when its reference count is at 1.

Original patch by Gustavo Carneiro:
    https://bugzilla.gnome.org/show_bug.cgi?id=546802#c5

https://bugzilla.gnome.org/show_bug.cgi?id=731501
---
 gi/pygobject-object.c |  6 ++++--
 tests/test_signal.py  | 29 ++++++++++++++++++++++++++++-
 2 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/gi/pygobject-object.c b/gi/pygobject-object.c
index ca82ffb9..729bb4dc 100644
--- a/gi/pygobject-object.c
+++ b/gi/pygobject-object.c
@@ -1168,8 +1168,10 @@ pygobject_traverse(PyGObject *self, visitproc visit, void *arg)
     if (self->inst_dict) ret = visit(self->inst_dict, arg);
     if (ret != 0) return ret;
 
-    if (data) {
-
+    /* Only let the GC track the closures when tp_clear() would free them.
+     * https://bugzilla.gnome.org/show_bug.cgi?id=731501
+     */
+    if (data && self->obj->ref_count == 1) {
         for (tmp = data->closures; tmp != NULL; tmp = tmp->next) {
             PyGClosure *closure = tmp->data;
 
diff --git a/tests/test_signal.py b/tests/test_signal.py
index 4e81c1eb..b2f121a6 100644
--- a/tests/test_signal.py
+++ b/tests/test_signal.py
@@ -5,7 +5,7 @@ import unittest
 import sys
 import weakref
 
-from gi.repository import GObject, GLib, Regress
+from gi.repository import GObject, GLib, Regress, Gio
 from gi import _signalhelper as signalhelper
 import testhelper
 from compathelper import _long
@@ -1466,5 +1466,32 @@ class TestRefCountsIntrospected(unittest.TestCase, _RefCountTestBase):
     Object = Regress.TestObj
 
 
+class TestClosureRefCycle(unittest.TestCase):
+
+    def test_closure_ref_cycle_unreachable(self):
+        # https://bugzilla.gnome.org/show_bug.cgi?id=731501
+
+        called = []
+
+        def on_add(store, *args):
+            called.append(store)
+
+        store = Gio.ListStore()
+        store.connect_object('items-changed', on_add, store)
+
+        # Remove all Python references to the object and keep it alive
+        # on the C level.
+        x = Gio.FileInfo()
+        x.set_attribute_object('store', store)
+        del store
+        gc.collect()
+
+        # get it back and trigger the signal
+        x.get_attribute_object('store').append(Gio.FileInfo())
+
+        self.assertEqual(len(called), 1)
+        self.assertTrue(called[0].__grefcount__ > 0)
+
+
 if __name__ == '__main__':
     unittest.main()
-- 
cgit v1.2.1