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 <firstname.lastname@example.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,
(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,
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]
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]
Kris, Tim thinks this is OK:
Dec 14 12:48:09 <rambokid> murrayc: you wanted me to look at some
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.