Created attachment 33005 [details] [review]
couples of fixes

- ref the model
- initialise the has_entry field
- add a finalize method, chains up to GtkCellRendererText::finalize

2004-10-24  Matthias Clasen  <mclasen@redhat.com>

	* gtk/gtkcellrenderercombo.c: Use G_DEFINE_TYPE, intialize the
	has_entry property, ref the model and add a finalizer.  (#156325,
	Olivier Andrieu)

reopening, as I found other problems.

I made a small test program and I got a couple segfaults. Valgrind then
uncovered a couple of bugs:

- a trivial memory leak in gtkcellrenderercombo.c
- an 'invalid read' in gtkentry.c, apparently an out-of-bounds access in an array
- and the combo box of the cell renderer being finalized too soon (causing the
segfault).

In the test program, which uses a combo renderer whith an entry, the combo box
send the 'editing-done' which triggers the 'edited' signal sent by the renderer,
which is used to modify the model. This causes the treeview to call
gtk_tree_view_stop_editing() from its gtk_tree_view_row_changed() handler. This
causes the combo box widget to be removed, bringing its refcount to zero.
Problem is, after the editing-done signal, the renderer sends a remove-widget
signal but the combo box has already been finalized. 

Anyway ... I ended up modifying gtk_combo_box_start_editing(), using
g_signal_connect_object to get an additional refcount on the combobox. Seems to
work.

Created attachment 33038 [details]
small test program

Created attachment 33039 [details] [review]
attempted fix

it works fine when the combo renderer has no entry (has_entry property set to
false), so maybe the change in gtk_combo_box_start_editing() is only needed in
the else branch.

I think Owen fixed the entry buffer overrun in Pango recently.
I have fixed the leak. Regarding the finalization issue, I
can't reproduce a crash with your test app here. I'll look at it
again later at home.

I have to admit that the ::edited ::editing-done ::remove-widget logic
has been very confusing to me when I wrote this cell renderer, so maybe
it *is* broken.

In future, please open separate bugs for new issues.

Here are the valgrind errors I get:

==6197== Invalid read of size 4
==6197==    at 0x1B98BED7: gtk_cell_editable_key_press (gtkcombobox.c:4719)
==6197==    by 0x1BA0F3A1: _gtk_marshal_BOOLEAN__BOXED (gtkmarshalers.c:83)
==6197==    by 0x1BCA0918: IA__g_closure_invoke (gclosure.c:437)
==6197==    by 0x1BCB0CC1: signal_emit_unlocked_R (gsignal.c:2442)
==6197==    by 0x1BCB02A8: IA__g_signal_emit_valist (gsignal.c:2211)
==6197==    by 0x1BCB0458: IA__g_signal_emit (gsignal.c:2245)
==6197==    by 0x1BAF14F9: gtk_widget_event_internal (gtkwidget.c:3587)
==6197==    by 0x1BAF119D: IA__gtk_widget_event (gtkwidget.c:3393)
==6197==    by 0x1BAFEC42: IA__gtk_window_propagate_key_event (gtkwindow.c:4549)
==6197==    by 0x1BAFECEE: gtk_window_key_press_event (gtkwindow.c:4579)
==6197==  Address 0x1C26F888 is 0 bytes inside a block of size 212 free'd
==6197==    at 0x1B902FB1: free (vg_replace_malloc.c:153)
==6197==    by 0x1BCF2011: IA__g_free (gmem.c:187)
==6197==    by 0x1BCB554D: IA__g_type_free_instance (gtype.c:1636)
==6197==    by 0x1BCA2904: g_object_last_unref (gobject.c:581)
==6197==    by 0x1BCA5306: IA__g_object_unref (gobject.c:1591)
==6197==    by 0x1BCA581C: g_value_object_free_value (gobject.c:1699)
==6197==    by 0x1BCBB42E: IA__g_value_unset (gvalue.c:155)
==6197==    by 0x1BCB041B: IA__g_signal_emit_valist (gsignal.c:2231)
==6197==    by 0x1BCB0570: IA__g_signal_emit_by_name (gsignal.c:2269)
==6197==    by 0x1B95F0E6: IA__gtk_cell_editable_editing_done
(gtkcelleditable.c:114)
==6197==    by 0x1B98BEC6: gtk_cell_editable_key_press (gtkcombobox.c:4718)

I can't reproduce this. For me, the combobox always has a ref_count > 1 when I
come to stop_editing. Also your scenario above doesn't quite  match the code.
If you look at gtk_tree_view_stop_editing() and gtk_tree_view_remove_widget(),
you will notice that the treeview is careful to prevent a row change during
stop_editing from removing the widget.

Did you try to run it with valgrind ?

In stop_editing the refcount is >1 because there are several signal emissions
higher in the stack that increased the refcount. The refcount does not reach
zero in stop_editing, but remove_widget decrease it and it reaches zero later.
That's because in gtk_combo_box_start_editing() the cell_editable is passed as
user_data to the key_press_event signal handler, so it's refcount is not
increased when this signal is processed.

Here's a gdb script. At the end, just before
gtk_cell_renderer_combo_editing_done returns, I have a refcount of 1.

Created attachment 33114 [details]
gdb script printing the refcount of the combobox

gdb ./testrenderercombo
(gdb) source testrenderercombo.gdb

Ok, I see this now. Really a mess. But I don't see how using
g_signal_connect_object () would really fix the problem - it doesn't add a
reference, only a weak ref, and ensures that the signal is no longer emitted if
the object is gone.  Does the following patch fix your problem ? 

Created attachment 33126 [details] [review]
alternative patch

g_signal_connect_object() really does fix the problem I think. It adds a real
ref for the duration of the callback, not a weak ref, cf. the documentation of
g_object_watch_closure().

Your patch should also fix the problem but I'd say that the connect_object() is
cleaner. popup_idle() in gtkcombobox.c also use connect_object() and the setup
is kind of similar.

Ok, you are right. 

2004-10-28  Matthias Clasen  <mclasen@redhat.com>

	* gtk/gtkcombobox.c (gtk_combo_box_start_editing): Use
	g_signal_connect_object() to prevent premature finalization
	of the cell_editable while the key_press_event signal is
	handled.  (#156325, Olivier Andrieu)