GNOME Bugzilla – Bug 701296
gtkfixed accepts toplevel windows into its children list after gtk_widget_set_parent fails
Last modified: 2017-10-13 12:37:17 UTC
gtk_fixed_put [gtkfixed.c:150] is a re-parenting function. It accepts a widget and attempts to set that widgets parent to the GtkFixed widget (gtk_widget_set_parent). After that it adds the widget to the children list of the GtkFixed widget. If gtk_widget_set_parent() fails [for reasons: 1) the widget already has a parent 2) the widget is a toplevel window] then it silently exits. If the parent has not been set for the widget, the widget is *still* added to the children list of the GtkFixed widget. Later on... Events may cause traversals over the GtkFixed children list (such as a EVENT_EXPOSE event] and act on the children. The expose event in particular checks if the GtkFixed children have the parent pointer set to the GtkFixed widget (This stage has failed earlier). This causes a GPF. I have produced a Gtk app to demonstrate this bug. Running it will produce the GPF. Patch included. Thanks, Damien
Created attachment 245660 [details] Reproducable case
Created attachment 245661 [details] [review] patch file
Additional info: The actual GPF... Gtk:ERROR:/build/buildd/gtk+2.0-2.24.10/gtk/gtkcontainer.c:2763:IA__gtk_container_propagate_expose: assertion failed: (child->parent == GTK_WIDGET (container))
Review of attachment 245661 [details] [review]: ::: gtk+-2.24.10_orig/gtk/gtkfixed.c @@ +163,3 @@ child_info->y = y; + if (gtk_widget_set_parent (widget, GTK_WIDGET (fixed))) I don't know why you thought that fixing a local thinko in GtkFixed required an API break for all GtkWidgets, everywhere... The correct thing to do is far simpler: just g_return_if_fail (gtk_widget_get_parent (widget) == NULL), so that the errant non-child does not get added to the internal list of supposed children. @@ +164,3 @@ + if (gtk_widget_set_parent (widget, GTK_WIDGET (fixed))) + { GTK+, GLib, et al. use an extra level of indentation for braces. ::: gtk+-2.24.10_orig/gtk/gtkwidget.c @@ +8914,3 @@ if (gtk_widget_get_has_window (widget)) { + gtk_grab_remove ( widget ); What relation does this have to the current bug? Also, the spacing is wrong. ::: gtk+-2.24.10_orig/gtk/gtkwidget.h @@ +995,3 @@ gboolean redraw_on_allocate); +gboolean gtk_widget_set_parent (GtkWidget *widget, Again, no.
Created attachment 361418 [details] [review] Fixed: If can’t add child, don’t add to child info Fixed: If can’t add child, don’t add to child info If the call to set_parent() failed, we were still adding the child to the internal list of children, despite that it was not really added. That meant we could later try to do invalid stuff with that non-child. Fix that by asserting and giving up if the child that the user is attempting to add is already parented.
Having said that, thanks for spotting this, of course!
(In reply to Daniel Boles from comment #4) > Review of attachment 245661 [details] [review] [review]: > > ::: gtk+-2.24.10_orig/gtk/gtkfixed.c > @@ +163,3 @@ > child_info->y = y; > > + if (gtk_widget_set_parent (widget, GTK_WIDGET (fixed))) > > I don't know why you thought that fixing a local thinko in GtkFixed required > an API break for all GtkWidgets, everywhere... Technically speaking, changing a function returning void to return something is neither an API nor an ABI break. We're still not going to change gtk_widget_set_parent(), though.
Review of attachment 361418 [details] [review]: ::: gtk/gtkfixed.c @@ +236,3 @@ g_return_if_fail (GTK_IS_FIXED (fixed)); g_return_if_fail (GTK_IS_WIDGET (widget)); + g_return_if_fail (gtk_widget_get_parent (widget) == NULL); This should be: g_return_if_fail (_gtk_widget_get_parent (widget) == NULL); to use the fast internal check for GtkWidget fields.
Created attachment 361420 [details] [review] Fixed: If can’t add child, don’t add to child info Good point, thanks for the quick review!
Comment on attachment 361420 [details] [review] Fixed: If can’t add child, don’t add to child info Come to think of it, this will explain why GtkFixed let me accidentally double up child widgets before... I also pushed a fix for the previous dereference of priv before the type check.
I guess this needs to also assert that the passed widget is not a toplevel.
(In reply to Daniel Boles from comment #11) > I guess this needs to also assert that the passed widget is not a toplevel. No widget does this check, and it's kind of possible anyway, if you know what you're doing — that's how Glade implements its own UI design canvas.
Created attachment 361508 [details] [review] Fixed: Handle set_parent failing for other reasons (In reply to Emmanuele Bassi (:ebassi) from comment #12) > No widget does this check However, as Damien noted, gtk_widget_set_parent() does. So, as of now, that would still assert and bail out if receiving an unparent toplevel. However, GtkFixed now only checks that the child is non-null, so it would continue to add the toplevel to its GList, even though the call to set_parent() failed. I think the attached should handle both cases, letting set_parent() report the right errors, and ensuring GtkFixed doesn't push the non-added child to its list in either scenario.
(In reply to Daniel Boles from comment #13) > Created attachment 361508 [details] [review] [review] > Fixed: Handle set_parent failing for other reasons > > (In reply to Emmanuele Bassi (:ebassi) from comment #12) > > No widget does this check > > However, as Damien noted, gtk_widget_set_parent() does. So, as of now, that > would still assert and bail out if receiving an unparent toplevel. However, > GtkFixed now only checks that the child is non-null, so it would continue to > add the toplevel to its GList, even though the call to set_parent() failed. You can add a `g_assert (child->parent == fixed)` if you want to be extra paranoid, but I'll say this again: no widget does this. We don't put a precondition check for every fallible case in every entry point; we put precondition checks in the related functions. The `parent == NULL` check is a safe addition; `gtk_container_add()` does it, but methods that do not call `gtk_container_add()`, like `gtk_box_pack_*()` and `gtk_fixed_put()`, should do the same, at least for consistency. In either case, you'll get a critical warning. > I think the attached should handle both cases, letting set_parent() report > the right errors, and ensuring GtkFixed doesn't push the non-added child to > its list in either scenario. No other widget does this check. Always remember GIGO: Garbage In, Garbage Out.
Makes sense, thanks for explaining. And for fixing the missing include before!