GNOME Bugzilla – Bug 641525
ref leak fix in pygobject causes a lot of invalid objects in gtk
Last modified: 2011-02-09 16:48:18 UTC
Recently, pygobject git master got a fix to properly clean up references to unowned objects [1]. While this was a valid fix and fixes a lot of memory errors, it also uncovered a lot of bugs in e. g. the GTK gobject-introspection annotations (most of which got fixed in GTK now), and apparently also in pygtk. We now get a lot of failure reports for all kinds of pygtk apps, leading to segfaults, bus errors, etc, all of which are due to trying to access already freed objects. I found a rather small example which reproduces an instance of this: ------------- 8< ---------------- python -c 'import gtk; r=gtk.ImageMenuItem("foo"); i=gtk.image_new_from_icon_name("gtk-ok", gtk.ICON_SIZE_MENU); r.set_image(i)' -c:1: Warning: invalid unclassed pointer in cast to `GtkWidget' -c:1: GtkWarning: IA__gtk_widget_set_parent: assertion `GTK_IS_WIDGET (widget)' failed -c:1: Warning: g_object_set: assertion `G_IS_OBJECT (object)' failed (-c:7847): GLib-GObject-CRITICAL **: g_object_get_qdata: assertion `G_IS_OBJECT (object)' failed (-c:7847): GLib-GObject-CRITICAL **: g_object_set_qdata_full: assertion `G_IS_OBJECT (object)' failed (-c:7847): GLib-GObject-CRITICAL **: g_object_get_qdata: assertion `G_IS_OBJECT (object)' failed (-c:7847): GLib-GObject-CRITICAL **: g_object_set_qdata_full: assertion `G_IS_OBJECT (object)' failed (-c:7847): GLib-GObject-CRITICAL **: g_object_get_qdata: assertion `G_IS_OBJECT (object)' failed (-c:7847): GLib-GObject-CRITICAL **: g_object_set_qdata_full: assertion `G_IS_OBJECT (object)' failed (-c:7847): GLib-GObject-CRITICAL **: g_object_set_qdata_full: assertion `G_IS_OBJECT (object)' failed (-c:7847): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed ------------- 8< ---------------- This doesn't cause a segfault yet, but the assertion failures already show that the GtkMenuItem and GtkImage references are badly handled. As soon as pygobject will release (which should be soon), this is going to affect a lot of pygtk apps. [1] http://git.gnome.org/browse/pygobject/commit/?id=f0a0b6c2eda89622de2b1e5ebb6a48103ad72a42
Also happens on pygtk git head.
Would be great if we could get a test case for this in the suite.
Tomeu, I'd love to provide one. Given how comprehensive pygtk's test suite already is, I'd be really surprised if it wouldn't already uncover this problem, as it currently breaks so much pygtk programs. But the test suite doesn't currently run: $ make check [...]
+ Trace 225870
suite.addTest(loader.loadTestsFromName(name))
module = __import__('.'.join(parts_copy))
class PObject(gobject.GObject):
enum = gobject.property(type=gtk.WindowType, default=gtk.WINDOW_TOPLEVEL)
self.type = self._type_from_python(type)
raise TypeError("Unsupported type: %r" % (type_,))
make[2]: *** [check-local] Fehler 1 make[2]: Verlasse Verzeichnis '/home/martin/upstream/pygtk/tests' I wonder why a gtk._gtk.WindowType object isn't caught by this case: elif isinstance(type_, type) and issubclass(type_, _gobject.GObject): return type_.__gtype__ It's certainly meant to be?
When I replace "issubclass(type_, _gobject.GObject)" with "hasattr(type_, '__gtype__')", I get a little further, but only to the next NotImplementedError. I've got the feeling that something rather fundamental changed in pygobject or pygtk recently, but I rewound pygobject by half a year, and pygtk to 2.22.0, and it still fails the same way. I'm using gtk+, pygtk, and pygobject from git head, so I don't have any distro specific changes here; but just to make sure, do you see the same?
Apparently, pygtk's tests have been broken for some time now. See https://bugzilla.gnome.org/show_bug.cgi?id=636589 for more info (haven't had the time yet to investigate myself, sorry).
Created attachment 180283 [details] test case I got the tests to run by removing tests/test_enum.py. If you drop attached file into pygtk's tests/ directory, "make check" will abort/crash with pygobject git head, and succeed with the last pygobject release, or with git head with commit f0a0b6c2eda reverted.
Martin, could you test the patch I attached to bug 636589? Thanks!
Created attachment 180285 [details] GtkImage factory test case Actually it's even simpler. It doesn't seem specific to GtkImageMenuItem, but to GtkImage itself, which would explain why it breaks so much stuff (I guess pretty much every program uses GtkImage). A mere factory method call shows the bad ref handling: $ python -c 'import gtk; i=gtk.image_new_from_icon_name("gtk-ok", gtk.ICON_SIZE_MENU)' (-c:17639): GLib-GObject-CRITICAL **: g_object_get_qdata: assertion `G_IS_OBJECT (object)' failed (-c:17639): GLib-GObject-CRITICAL **: g_object_set_qdata_full: assertion `G_IS_OBJECT (object)' failed (-c:17639): GLib-GObject-CRITICAL **: g_object_get_qdata: assertion `G_IS_OBJECT (object)' failed (-c:17639): GLib-GObject-CRITICAL **: g_object_set_qdata_full: assertion `G_IS_OBJECT (object)' failed (-c:17639): GLib-GObject-CRITICAL **: g_object_get_qdata: assertion `G_IS_OBJECT (object)' failed (-c:17639): GLib-GObject-CRITICAL **: g_object_set_qdata_full: assertion `G_IS_OBJECT (object)' failed (-c:17639): GLib-GObject-CRITICAL **: g_object_set_qdata_full: assertion `G_IS_OBJECT (object)' failed (-c:17639): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed If you try to print it in addition, it replicates the segfault: $ python -c 'import gtk; i=gtk.image_new_from_icon_name("gtk-ok", gtk.ICON_SIZE_MENU); print i' Speicherzugriffsfehler (Speicherabzug geschrieben) Simpler test case attached.
For the record, I confirm that reverting http://git.gnome.org/browse/pygobject/commit/?id=7bc4122897d9d05172a2bd5b56bded87e2afaec4 in pygobject also fixes both above tests and also virt-manager. Thanks Steve for pointing out!
Sorry about the last comment, got my $PYTHONPATH messed up. So, to clarify: * with only 7bc4122897d9 reverted, I still get a virt-manager crash, but meld now works fine. The GtkImage test case now complains about Gtk-WARNING **: A floating object was finalized. This means that someone called g_object_unref() on an object that had only a floating reference; the initial floating reference is not owned by anyone and must be removed with g_object_ref_sink(). which confirms that it's due to ref count issues (which we already knew). But at least that doesn't crash that test any more. * with only f0a0b6c2ed reverted, or with both reverted (no difference at all here), both the GtkImage test case as well as virt-manager/meld/etc. work fine again.
Created attachment 180293 [details] GtkImage factory test case This is a slightly more complete test case which also tests the creation of a gtk.Image() directly. This seems to work fine with pygobject git head, so it seems that the reference handling problem lies in the factory wrapper method gtk.image_new_from_icon_name(). However, there is no custom code for that in pygtk, just the declaration in gtk/gtk-base.defs. When looking into the autogenerated code for that, we see that it's indeed unref'ed: static PyObject * _wrap_gtk_image_new_from_icon_name(PyObject *self, PyObject *args, PyObject *kwargs) { [...] ret = gtk_image_new_from_icon_name(icon_name, size); py_ret = pygobject_new((GObject *)ret); if (ret != NULL) g_object_unref(ret); return py_ret; } This seems wrong? Why is it calling g_object_unref(ret) here?
May I suggest to add an assertion on the __grefcount__ member of the returned object in your test cases? Since the only reference of your object is owned by python, obj.__grefcount__ should be 1. If you return an object reffed by someone else, obj.__grefcount__ should be 2 or more.
Discussed that with Steve (the author of the pygobject patches) on IRC. 2011-02-07 13:54:31 pitti nud: http://paste.ubuntu.com/563838/ 2011-02-07 13:54:44 pitti nud: this indeed unrefs it 2011-02-07 13:55:51 nud so "oh look ma we leak an image here" 2011-02-07 13:55:51 pitti nud: updated bug again 2011-02-07 13:56:15 pitti ok, probably because it thinks that pygobject_new() refs it? 2011-02-07 13:56:31 pitti nud: did that get changed in your patches? 2011-02-07 13:56:42 nud it used to 2011-02-07 13:57:10 nud the GInitiallyUnowned patch takes out a systematic ref for initiallyunowned objects 2011-02-07 13:57:33 nud so this unref was probably here to bring back the balance into the force 2011-02-07 13:58:53 pitti ah, so that was probably the workaround for the pygobject bug which now got fixed? Unfortunately I'm unable to see where that g_object_unref() is generated. git grep shows me several instances in gtk/gdkpixbuf.override which will need to get fixed as well, but not the "generic" wrapper creator. How is above code generated?
Looks like it's written in pygobject's codegen: codegen/argtypes.py line 532 in pygobject master.
Created attachment 180296 [details] [review] codegen: remove systematic unreffing of created objects. pygobject should now be handling these cases just fine by itself.
Created attachment 180299 [details] GtkImage factory test case (In reply to comment #12) > May I suggest to add an assertion on the __grefcount__ member of the returned > object in your test cases? Done now. As we suspected, it's correct (1) for the gtk.Image() case (test_create), and wrong (2) for the gtk.image_new_from_icon_name() case (test_factory_from_icon_name). With Steve's patch from comment 15 applied, the test case now succeeds, and virt-manager does not crash any more either. meld still crashes, presumably it uses some other API (or unsets $PYTHONPATH). So in any case Steve's codegen fix looks correct to me to "counteract" the previous two ref leak patches, and greatly help to improve the situation already. I'll debug the meld crash now and also see to fix the other now invalid unref() cases in pygtk.
Created attachment 180301 [details] reference count test case I renamed the test case to test_ref.py (since it's checking ref counting) and added two more tests to it for overrides of gtk.gdk.Pixbuf (constructor and factory method again), as there are overrides in pygtk for those, using g_object_unref(). These seem to work correctly, though. Should I commit this test case to pygtk now?
I would love to see it there, but I'm not a maintainer :)
For the record, meld works fine with this fix as well; it's just messing with $PYTHONPATH and thus didn't pick up my local pygtk trunk build before. AFAICS the dispute (from Johan Dahlin) is that pygobject shouldn't break "ABI" (which in his opinion includes ref count behaviour_ at all, but these patches are already committed. Does anyone see a reason not to commit Steve's followup fix in comment 15? This makes pygtk work very well again here, and looks obvious as a followup fix for f0a0b6c2 and 7bc4122897d9.
Created attachment 180479 [details] [review] Ensure the sink functions are only ran once. This mitigates in a not-so-clean way the leak experienced in pygobject for wrappers created multiple times, without breaking pygtk.
Created attachment 180482 [details] [review] Revert "Fix reference leaks for GInitiallyUnowned objects" This reverts commit f0a0b6c2eda89622de2b1e5ebb6a48103ad72a42. The test cases have been kept.
Created attachment 180483 [details] [review] Revert "Fix wrong refcount when calling introspected widget constructors" This reverts commit 7bc4122897d9d05172a2bd5b56bded87e2afaec4.
I'm moving this back to pygobject, as from the discussions it was pretty much decided to not break the ABI for the impending pygobject release. I confirm that Steve's latest patch in comment 20 fixes the crash. With both the original leak fixes (7bc41228 and f0a0b6c) reverted (just the code, not the tests), I have three test case failures. The patch above reduces that to one: ====================================================================== FAIL: test_floating (test_everything.TestEverything) ---------------------------------------------------------------------- Traceback (most recent call last):
+ Trace 225916
self.assertEquals(e.__grefcount__, 1)
The other two are fixed now.
Created attachment 180484 [details] [review] Revert "Fix wrong refcount when calling introspected widget constructors" This reverts commit 7bc4122897d9d05172a2bd5b56bded87e2afaec4. I accidentally removed the tests
Created attachment 180486 [details] [review] Decrease the refcount for GInitiallyUnowned constructors. This mimicks the weird legacy pygtk behaviour and makes all the tests pass despite the proper fixes having been reverted.
This problem has been fixed in our software repository. The fix will go into the next software release. Thank you for your bug report.