diff options
author | Christoph Reiter <creiter@src.gnome.org> | 2017-10-26 17:24:02 +0200 |
---|---|---|
committer | Christoph Reiter <creiter@src.gnome.org> | 2017-10-26 20:09:00 +0200 |
commit | 17b4ba8707a8c6c24cd52e59a1f107dccfa9fd55 (patch) | |
tree | f4e5642b6f681bab853d068068abe671a660ca04 | |
parent | b4bf1b9d936e021b1645c069c2e0a3062cfab62b (diff) | |
download | pygobject-17b4ba8707a8c6c24cd52e59a1f107dccfa9fd55.tar.gz |
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
-rw-r--r-- | gi/pygobject-object.c | 6 | ||||
-rw-r--r-- | 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() |