GNOME Bugzilla – Bug 786048
GtkNotebook does not fully remove disposed children
Last modified: 2018-02-06 14:19:27 UTC
Created attachment 357263 [details] Test case In gtk+-3 notebook pages are children of the notebook. In gtk+-4 they are now children of a GtkStack due to this commit: https://git.gnome.org/browse/gtk+/commit/?id=6f2145be002598968126ce1e69499182e0579e3a The attached test case shows what can happen to wrapped widgets in gtkmm, when the wrapping C++ objects are constructed and destructed. The test contains a GtkNotebook with 3 GtkLabels as pages. When a wrapping Gtk::Label is destructed, g_object_run_dispose(label) is called. gtk_widget_dispose() calls gtk_container_remove() to have the child widget removed from the container. This works fine in gtk+-3, where the GtkNotebook is the parent container. It removes its child page. In gtk+-4, the child is instead removed from a GtkStack, which is it's parent container. The notebook is not informed, I believe, and it keeps a pointer to the now deleted page. This is disastrous when the notebook is deleted, and tries to delete its already deleted child.
I can't remember which one it was but I fixed something similar to this in another widget; you just have to remove the children manually in dispose(). IIRC there's a broader problem with gtk_widget_destroy which could previously been used to properly remove every widget from the widget tree but now breaks in exactly the way you describe here.
Created attachment 364618 [details] [review] patch: GtkNotebook: Remove an unparented child It looks like this patch fixes the bug. But I don't know all the details of GtkNotebook. It's possible that I have overlooked something. Anyway the notebook must be informed when one of its child pages is unparented from the notebook's stack widget, and the unparenting is not initiated by the notebook itself.
That's a wrong patch because it tries to actively works around brokenness elsewhere. The bug here is that gtk_widget_dispose() does try to unparent itself. Instead it should assert that parent == NULL, because the parent holds a reference. And the bug in your example is that you think you can release references by calling g_object_run_dispose(). That is not how GObject refcounting works.
My test case shows what happens when widgets are deleted in gtkmm, the C++ binding of gtk+. g_object_run_dispose() has been used in this way for about 7 years in gtk+-3. It replaced the removed gtk_object_destroy(). https://git.gnome.org/browse/gtkmm/commit/?id=683b63dc7d9498221e8de1349e78c3e79bcc96a2 The problem with GtkNotebook in gtk+-4, as I see it, is that it's no longer possible to remove one of its pages with a call such as gtk_container_remove (GTK_CONTAINER (gtk_widget_get_parent (child)), child); Such a call removes the child from the GtkStack used by GtkNotebook. The notebook itself is not informed, and keeps holding a pointer to the removed child widget.
Yes, this all came to the foreground with our work on allowing GtkWidgets to contain children. That started during GTK3 and is now the default and encouraged way to create composite widgets. The result is that gtk_widget_get_parent() does not necessarily return one of the containers the widget was added to (in this case 2: the notebook and the notebook's stack), but the real parent: The widget responsible for realizing, mapping, size_allocate()ing and so on for this widget. So calling gtk_container_remove (gtk_widget_get_parent (widget), widget); in GTK4 is wrong (it was usually okay in GTK3 and may sometimes still work in GTK4). However, calling gtk_widget_destroy() on anything but a toplevel window has always (and I think I want to even include GTK1 here) been questionable. You do not know who holds a reference to a widget and does expect it to keep on functioning, so you can't just destroy it. All you can do is release any reference you have and reverse any operations you did with the widget (like remove it from a container you added it to). So I would expect a C++ binding to treat GtkWidgets - like any GObject - similar to a shared_ptr and not as something you can just destroy.
> So I would expect a C++ binding to treat GtkWidgets - like any GObject - > similar to a shared_ptr and not as something you can just destroy. That's how many GObjects are treated, but widgets and a few other types of objects, all derived from GInitiallyUnowned, are treated differently. I don't know why. I just know that a change would require massive changes in gtkmm and every application that uses gtkmm. > All you can do is release any reference you have and reverse any operations > you did with the widget (like remove it from a container you added it to). I tried to do just that. It solves the problem with GtkNotebook. If a child of a notebook is removed with gtk_container_remove (GTK_CONTAINER (notebook), child); the child is destroyed if its last reference is dropped when it's removed. But it doesn't work for GtkSearchBar. A child of a GtkSearchBar that has been added with gtk_container_add (GTK_CONTAINER (searchbar), child); can't be removed with gtk_container_remove (GTK_CONTAINER (searchbar), child); But it can be removed with gtk_container_remove (GTK_CONTAINER (gtk_widget_get_parent (child)), child); Is this something that gtkmm has to accept, or is it a bug in GtkSearchBar?
That should definitely work for GtkSearchBar and every other GtkContainer. What doesn't work about that code?
GtkSearchBar overrides the vfunc GtkContainer.add() and adds the child to priv->box_center. It does not override GtkContainer.remove(). The base class GtkBin takes care of the call to gtk_container_remove(). But GtkBin, correctly, does not recognize the child as its own child, and does not remove it.
I was more asking about the observed behavior at runtime, but that should be fixed by https://git.gnome.org/browse/gtk+/commit/?id=b726f60f9039b647d6c6b7fe4f417923d65140a2.
At runtime, before the latest fixes in GtkSearchBar: (gtkmm-demo:3974): Gtk-CRITICAL **: 09:35:08.819: gtk_bin_remove: assertion 'priv->child == child' failed Fixed by the latest GtkSearchBar patches. Remaining known problems with disposing widgets, are problems in gtkmm, not affecting gtk+. So I close this bug.