GNOME Bugzilla – Bug 643192
Closures should be sunk instead of unreffed
Last modified: 2011-03-03 23:56:51 UTC
with latest gedit, activate quickopen, control+alt+o and then esc. It always crashes. See: http://git.gnome.org/browse/gedit/tree/plugins/quickopen/quickopen/popup.py we are probably missing some ref in the closures handling. See the valgrind log: ==24737== Invalid read of size 4 ==24737== at 0x7C54828: g_closure_unref (gclosure.c:590) ==24737== by 0x7C6F384: g_signal_handlers_destroy (gsignal.c:637) ==24737== by 0x7C5623C: g_object_real_dispose (gobject.c:894) ==24737== by 0x7C56599: g_object_unref (gobject.c:2697) ==24737== by 0x5650290: accel_group_weak_ref_detach (gtkaccelgroup.c:304) ==24737== by 0x7C5620F: weak_refs_notify (gobject.c:2231) ==24737== by 0x82C8934: g_datalist_id_set_data_full (gdataset.c:351) ==24737== by 0x7C5897F: g_object_run_dispose (gobject.c:945) ==24737== by 0x3B80605CA3: ffi_call_unix64 (unix64.S:75) ==24737== by 0x3B806056D4: ffi_call (ffi64.c:484) ==24737== by 0x76EBA9F: _g_callable_info_invoke (gicallableinfo.c:496) ==24737== by 0x76ECC0D: g_function_info_invoke (gifunctioninfo.c:273) ==24737== by 0x1635E8EC: _wrap_g_callable_info_invoke (pygi-invoke.c:588) ==24737== by 0x3B892E00DA: PyEval_EvalFrameEx (ceval.c:4382) ==24737== by 0x3B892E199C: PyEval_EvalCodeEx (ceval.c:3312) ==24737== by 0x3B892E02F2: PyEval_EvalFrameEx (ceval.c:4168) ==24737== by 0x3B892E199C: PyEval_EvalCodeEx (ceval.c:3312) ==24737== by 0x3B8926DA9B: function_call (funcobject.c:526) ==24737== by 0x3B892490C2: PyObject_Call (abstract.c:2529) ==24737== by 0x3B89257C1E: instancemethod_call (classobject.c:2578) ==24737== Address 0x10a95e20 is 0 bytes inside a block of size 64 free'd ==24737== at 0x4A0556E: free (vg_replace_malloc.c:366) ==24737== by 0x7C5496C: g_closure_unref (gclosure.c:588) ==24737== by 0x7C6F384: g_signal_handlers_destroy (gsignal.c:637) ==24737== by 0x7C5623C: g_object_real_dispose (gobject.c:894) ==24737== by 0x7C56599: g_object_unref (gobject.c:2697) ==24737== by 0x5650290: accel_group_weak_ref_detach (gtkaccelgroup.c:304) ==24737== by 0x7C5620F: weak_refs_notify (gobject.c:2231) ==24737== by 0x82C8934: g_datalist_id_set_data_full (gdataset.c:351) ==24737== by 0x7C5897F: g_object_run_dispose (gobject.c:945) ==24737== by 0x3B80605CA3: ffi_call_unix64 (unix64.S:75) ==24737== by 0x3B806056D4: ffi_call (ffi64.c:484) ==24737== by 0x76EBA9F: _g_callable_info_invoke (gicallableinfo.c:496) ==24737== by 0x76ECC0D: g_function_info_invoke (gifunctioninfo.c:273) ==24737== by 0x1635E8EC: _wrap_g_callable_info_invoke (pygi-invoke.c:588) ==24737== by 0x3B892E00DA: PyEval_EvalFrameEx (ceval.c:4382) ==24737== by 0x3B892E199C: PyEval_EvalCodeEx (ceval.c:3312) ==24737== by 0x3B892E02F2: PyEval_EvalFrameEx (ceval.c:4168) ==24737== by 0x3B892E199C: PyEval_EvalCodeEx (ceval.c:3312) ==24737== by 0x3B8926DA9B: function_call (funcobject.c:526) ==24737== by 0x3B892490C2: PyObject_Call (abstract.c:2529) ==24737== ==24737== Invalid read of size 4 ==24737== at 0x82B9F82: g_atomic_int_compare_and_exchange (gatomic-gcc.c:44) ==24737== by 0x7C5486B: g_closure_unref (gclosure.c:590) ==24737== by 0x7C6F384: g_signal_handlers_destroy (gsignal.c:637) ==24737== by 0x7C5623C: g_object_real_dispose (gobject.c:894) ==24737== by 0x7C56599: g_object_unref (gobject.c:2697) ==24737== by 0x5650290: accel_group_weak_ref_detach (gtkaccelgroup.c:304) ==24737== by 0x7C5620F: weak_refs_notify (gobject.c:2231) ==24737== by 0x82C8934: g_datalist_id_set_data_full (gdataset.c:351) ==24737== by 0x7C5897F: g_object_run_dispose (gobject.c:945) ==24737== by 0x3B80605CA3: ffi_call_unix64 (unix64.S:75) ==24737== by 0x3B806056D4: ffi_call (ffi64.c:484) ==24737== by 0x76EBA9F: _g_callable_info_invoke (gicallableinfo.c:496) ==24737== by 0x76ECC0D: g_function_info_invoke (gifunctioninfo.c:273) ==24737== by 0x1635E8EC: _wrap_g_callable_info_invoke (pygi-invoke.c:588) ==24737== by 0x3B892E00DA: PyEval_EvalFrameEx (ceval.c:4382) ==24737== by 0x3B892E199C: PyEval_EvalCodeEx (ceval.c:3312) ==24737== by 0x3B892E02F2: PyEval_EvalFrameEx (ceval.c:4168) ==24737== by 0x3B892E199C: PyEval_EvalCodeEx (ceval.c:3312) ==24737== by 0x3B8926DA9B: function_call (funcobject.c:526) ==24737== by 0x3B892490C2: PyObject_Call (abstract.c:2529) ==24737== Address 0x10a95e20 is 0 bytes inside a block of size 64 free'd ==24737== at 0x4A0556E: free (vg_replace_malloc.c:366) ==24737== by 0x7C5496C: g_closure_unref (gclosure.c:588) ==24737== by 0x7C6F384: g_signal_handlers_destroy (gsignal.c:637) ==24737== by 0x7C5623C: g_object_real_dispose (gobject.c:894) ==24737== by 0x7C56599: g_object_unref (gobject.c:2697) ==24737== by 0x5650290: accel_group_weak_ref_detach (gtkaccelgroup.c:304) ==24737== by 0x7C5620F: weak_refs_notify (gobject.c:2231) ==24737== by 0x82C8934: g_datalist_id_set_data_full (gdataset.c:351) ==24737== by 0x7C5897F: g_object_run_dispose (gobject.c:945) ==24737== by 0x3B80605CA3: ffi_call_unix64 (unix64.S:75) ==24737== by 0x3B806056D4: ffi_call (ffi64.c:484) ==24737== by 0x76EBA9F: _g_callable_info_invoke (gicallableinfo.c:496) ==24737== by 0x76ECC0D: g_function_info_invoke (gifunctioninfo.c:273) ==24737== by 0x1635E8EC: _wrap_g_callable_info_invoke (pygi-invoke.c:588) ==24737== by 0x3B892E00DA: PyEval_EvalFrameEx (ceval.c:4382) ==24737== by 0x3B892E199C: PyEval_EvalCodeEx (ceval.c:3312) ==24737== by 0x3B892E02F2: PyEval_EvalFrameEx (ceval.c:4168) ==24737== by 0x3B892E199C: PyEval_EvalCodeEx (ceval.c:3312) ==24737== by 0x3B8926DA9B: function_call (funcobject.c:526) ==24737== by 0x3B892490C2: PyObject_Call (abstract.c:2529)
Hmm, I didn't get this with the latest pygobject and and older build of gedit in jhbuild. Trying to build a newer gedit now.
So I had an idea right before I was about to go to bed which kept me awake - look for the API that we don't use a lot. At first I just thought it might be a bad annotation on an API that takes a callback but then I remembered that the crash is about closures not callbacks. Lo and behold I commented this out: accel_group = Gtk.AccelGroup() accel_group.connect(Gdk.KEY_l, Gdk.ModifierType.CONTROL_MASK, 0, self.on_focus_entry) self.add_accel_group(accel_group) and the crash went away. Looks like an issue with how we handle GClosure lifecycles. will look more into it in the morning.
some more notes before I go to bed: pygi-argument.c:1976 - we unref here but it seems like we are also unreffing on accel group destroy. } else if (g_type_is_a (type, G_TYPE_CLOSURE)) { if (direction == GI_DIRECTION_IN && transfer == GI_TRANSFER_NOTHING) { g_closure_unref (arg->v_pointer); } I get a bit confused with in/transfer none annotations. It seems that we do transfer ownership of the closure to the accel group. Why would we unref the only reference? It seems we don't destroy it on the first unref or at least the closure still works until we close the dialog.
Created attachment 182159 [details] simple test case which shows the issue The segfault is only a symptom of the double free. Best to break on g_closure_unref
looks like the double unref has to do with a circular destroy callback. The accel group is destroyed which calls g_signal_handlers_destroy which in turn unrefs the closure which then goes and calls g_closure_invalidate which calls accel_closure_invalidate which then again unrefs the closure. Might be a GTK+ bug.
The issue is a closure is a floating object that loses a ref when it gets sinked. We have two options here. We can either take a ref and sink it and then unref on cleanup which would be closer to the transfer none notation, or we can just trust the interfaces to sink it. Since there is no way to create a raw gclosure, they are all created when a parameter is marked as a closure, there is no way for them to remain floating. I'm leaning towards this for simplicity sake.
Created attachment 182191 [details] [review] [gi] remove unref for closures since they are floating objects that get sunk * right now we trust that the containers we send the closures into will sink them * we should research this a bit more to see if there is a better way to handle floating closures once we are free to break static binding ABI * for now this is the least invasive of all the options
*** Bug 640069 has been marked as a duplicate of this bug. ***
I added this to 2-28 branch but not to master. However I think we should add it to master after talking to walters who agreed that we should ignore gclosure transfer annotations and just treat it as if the interface will sink the closure. If it doesn't it is a bug in the interface, not pygobject.