GNOME Bugzilla – Bug 693400
gtkcellrenderertext: Sink floating entry before using as signal argument
Last modified: 2017-10-14 20:25:49 UTC
The GtkCellRendererText creates a GtkEntry and sends out the "editing-started" signal before the entry is ever sunk. This creates problems with language bindings because bindings always sink floating objects. By the time the signal closure is complete, the entry will have been destroyed because the bindings object wrapper sunk and assumed ownership of the floating reference. This originally started as a pygobject bug and was fixed by always adding an additional ref during object marshaling, this however created a reference leak. See also: bug 661359 and bug 675726
Created attachment 235485 [details] [review] gtkcellrenderertext: Sink floating entry before using as signal argument Sink the GtkEntry assigned to the private structure of GtkCellRendererText before signals contianing it as an argument are sent out. This keeps language bindings from sinking the reference and then destroying the entry when the signal closure is finished.
Review of attachment 235485 [details] [review]: Other than that, looks good to me. The reference handling in the cell editing code was always a bit shaky. ::: gtk/gtkcellrenderertext.c @@ +754,3 @@ + if (priv->entry) + g_object_unref (priv->entry); + g_clear_object @@ +1923,3 @@ + g_object_unref (priv->entry); + priv->entry = NULL; + } here too: g_clear_object @@ +2055,3 @@ + if (priv->entry) + g_object_unref (priv->entry); I think this is not necessary - the api contract requires that editing_done is called before start_editing can be called again.
Created attachment 235673 [details] [review] gtkcellrenderertext: Sink floating entry before using as signal argument Fixed up based on review feeback.
Review of attachment 235673 [details] [review]: ok
Attachment 235673 [details] pushed as 283c974 - gtkcellrenderertext: Sink floating entry before using as signal argument
Hi, the g_clear_object in the second chunk of this patch seems to be in the wrong place. It's causing destruction of the GtkCellEditable, but we immediately proceed to try to disconnect its signal handlers, as well as get its text and path to pass on to the 'edited' signal handlers for the cell renderer. I'm experiencing this in 3.14 (though it appears that it would afflict anything newer as well) Placing it after the signal emission resolves the issues. Attaching a fix that does just that. Thanks!
Created attachment 305108 [details] [review] Fixes bad placement of g_clear_object
Trace just to document the crash... (nemo:2775): GLib-GObject-WARNING **: instance with invalid (NULL) class pointer (nemo:2775): GLib-GObject-CRITICAL **: g_signal_handler_disconnect: assertion 'G_TYPE_CHECK_INSTANCE (instance)' failed (nemo:2775): GLib-GObject-WARNING **: instance with invalid (NULL) class pointer (nemo:2775): GLib-GObject-CRITICAL **: g_signal_handler_disconnect: assertion 'G_TYPE_CHECK_INSTANCE (instance)' failed (nemo:2775): GLib-GObject-CRITICAL **: g_object_get: assertion 'G_IS_OBJECT (object)' failed (nemo:2775): GLib-GObject-CRITICAL **: g_object_get_data: assertion 'G_IS_OBJECT (object)' failed (nemo:2775): Gtk-CRITICAL **: gtk_entry_get_text: assertion 'GTK_IS_ENTRY (entry)' failed Program received signal SIGSEGV, Segmentation fault. cell_renderer_edited (cell=<optimized out>, path_str=0x0, new_text=0x0, view=0xcee360) at nemo-list-view.c:1423 1423 if (new_text[0] == '\0') { (gdb) bt
+ Trace 235161
If I run with G_DEBUG=fatal-warnings, I can see the warnings and assertions leading up to it are from gtk_cell_renderer_text_editing_done() screaming about the unrefed GtkCellEditable.
Review of attachment 305108 [details] [review]: This changes the sense of the function: it now fails to g_clear_object(&priv->entry) if editing was cancelled. That's probably not a good thing. Looking at what other CellRenderer implementations do in this method may be instructive; I didn't look far yet. Anyway, if this still causes problems, it should get a new bug report. (or, at the very least, reopen the one you're piling on to, so that it stands a chance of being found)