GNOME Bugzilla – Bug 123923
2.3 crashes when setting GtkTreeModel::row_inserted "default signal handler"
Last modified: 2011-02-04 16:16:03 UTC
GTK+ 2.3 crashes when used with TreeViews in gtkmm. The change happened between 20030815 and 20030901. I think this is the change that caused it: Sat Aug 16 16:22:23 2003 Kristian Rietveld <kris@gtk.org> Fix major bug in row ref handling, so the new combo box will actually work right (:. Bug #107748. Patch written with help from Tim Janik. The basic idea is to update the row refs in a closure, before the actual signal is emitted (rather than having the model connect signal handlers). * gtk/gtktreemodel.c (gtk_tree_model_base_init): change g_signal_new calls for row_inserted, row_deleted and rows_reordered to use the new marshallers, (row_inserted_marshall), (row_deleted_marshall), (rows_reordered_marshall): the new marshallers, (gtk_tree_row_ref_inserted_callback): renamed to gtk_tree_row_ref_inserted since it isn't a callback anymore and gets called by the marshaller now, (gtk_tree_row_ref_deleted_callback): likewise, (gtk_tree_row_ref_reordered_callback): likewise, (connect_ref_callbacks), (disconnect_ref_callbacks): removed, (gtk_tree_row_reference_new_proxy), (gtk_tree_row_reference_free), (gtk_tree_row_reference_inserted), (gtk_tree_row_reference_deleted), (gtk_tree_row_reference_reordered): updated. The GtkTreeModel::render() vfunc (also known as "default signal handler") is never actually set in GTK+ itself, and was never set in previous versions either. But when used by gtkmm, it did work before. Attached (gtk_crash.patch) is a patch that sets it to an empty function in GtkListStore. This shows that the crash happens just by exercising the code after if(callback). You can see the crash by starting the gtk-demo and then choosing the ListStore part of the demo. Here (backtrace.txt) is a backtrace, but I think it is misleading. The attached patch (gtk_crash.patch) used g_warning() calls to show an infinite loop as row_inserted_marshal() is called repeatedly. I hope someone has a clue about this so I don't have to dig too deep into the closure mechanism.
Created attachment 20487 [details] [review] gtk_crash.patch (Sillyness, to reproduce the bug.)
Created attachment 20488 [details] [review] backtrace.txt
I mean, the "row_inserted" signal, not the "render" signal.
The row_deleted and rows_reordered signals seem to have the same problem, though they are obviously harder to trigger.
This seems quite simple. The existing implementations seem completely silly, and don't actually call the callbacks. I think this patch is what you meant to do. It seems to work, and gtkmm works again.
Created attachment 22199 [details] [review] treemodel_marshaller.patch
Kris, Tim thinks this is OK: Dec 14 12:48:09 <rambokid> murrayc: you wanted me to look at some marshaller issue? Dec 14 12:48:27 <murrayc> rambokid: Yeah, hang on, I'll find the bug. Dec 14 12:50:03 <murrayc> rambokid: This is the problem, with a simple patch to show it, and my possible solution: Dec 14 12:50:03 <murrayc> http://bugzilla.gnome.org/show_bug.cgi?id=123923 Dec 14 12:50:23 <murrayc> rambokid: But it seems like too much for just a default signal handler anyway. Dec 14 12:51:06 <murrayc> rambokid: And this is the bug that introduced that marshaller code in the first place (you looked at this at the time): http://bugzilla.gnome.org/show_bug.cgi?id=107748 Dec 14 12:51:27 <murrayc> rambokid: Thanks for any advice. Dec 14 12:54:58 <-- coltox has quit (molp kloptzu retroii) Dec 14 13:02:19 <rambokid> murrayc: please apply, you diagnosed correctly. closure->marshal() calling itself is kind of a cut-n-paste error that wasn't triggered. Dec 14 13:04:40 <murrayc> rambokid: That marshaller does not seem to be called for normal signal handlers, just the default signal handler. Does that makes sense. It seems like too much code. Dec 14 13:06:15 <rambokid> murrayc: the problem here was that we didn't have a default function originally and later figured, we need to update th etree state before the signal handlers and default handlers of the interface are being called. Dec 14 13:06:50 <rambokid> as a workaround, we went for the hack of updating tree state from the default handler marshaller (and have the signal be emitted RUN_FIRST) Dec 14 13:07:40 <rambokid> so, we don't want the marshaller be used for normal signal handlers, we implement the marshaller for the default handler by hand, to be able to sneak in some code Dec 14 13:08:26 <murrayc> rambokid: OK, so there was reasoning. Thanks a lot for making us feel more confident about it. I wish people would write more comments in GTK+ code. Dec 14 13:11:23 <rambokid> that code portion certainly deserves a comment, as it (ab-)uses the closure concept to some extend For future reference, it was the patch in bug 107748 that originally added the marshaller:
So, Kris, I'll apply this soon.
Sure go ahead. Thanks for figuring out/fixing.
Applied.