GNOME Bugzilla – Bug 731444
gtkmm::builder - derived widget's destructor is not called -> memory leaks
Last modified: 2014-06-19 09:26:29 UTC
Created attachment 278187 [details] Simple program to illustrate the bug. Retrieving derived widgets using "Gtk::Builder::get_widget_derived" results in memory leaks. Steps to reproduce: -Create a simplified user interface with a window containing only one button. -Retrieve both widgets (window and button) using "get_widget_derived" since they are modified by the code. -The manual states that "toplevel" widgets (windows and dialogs) must be deleted by the user, so delete the window as prescribed. -As the button is inside a container (the window) I expect the button to be managed and therefore being deleted automatically. This is not the case. The destructor of the button is never called. The results are big memory leaks and the fact that the code in the destrctor is never executed. Attached you'll find a simple test program that you can compile using g++ `pkg-config --libs --cflags gtkmm-3.0` gladetest.cpp There you will directly see the problem. It seems that get_widget and get_widget_derived handle refernce counting in a wrong way.
Created attachment 278282 [details] Modified test case This version of the test case prints reference counts. It can test the deletion of either a DerivedButton or a Gtk::Button. See also https://mail.gnome.org/archives/gtkmm-list/2014-June/msg00023.html Don't you exaggerate the severity of this bug? It does cause memory leaks, but those leaks are not always serious. Many GUI programs create many widgets at the start of the program, and keep them until the program ends. In such programs it's no serious bug that the widgets are not deleted. It's different if the program creates widgets for a popup window when the window is shown, deletes the window when it's hidden, and creates a new window the next time it's shown. Then the leaked memory can be a serious problem.
Created attachment 278283 [details] [review] patch: Gtk::Builder: Fix ref count in get_widget() and get_widget_derived() This patch fixes the bug. It's risky to push this fix. It may break working programs that have adapted to the present (erroneous) behaviour of Builder by executing widget->unreference(); A comment from some gtkmm developer would be appreciated. Note that even with this fix, the DerivedButton destructor is not called when 'delete main_win' is executed. The DerivedButton destructor is called only when both main_win and the Builder have been deleted. That is expected behaviour.
(In reply to comment #1) > Created an attachment (id=278282) [details] > Modified test case > > This version of the test case prints reference counts. It can test the > deletion of either a DerivedButton or a Gtk::Button. > > See also https://mail.gnome.org/archives/gtkmm-list/2014-June/msg00023.html > > Don't you exaggerate the severity of this bug? It does cause memory leaks, > but those leaks are not always serious. Yes maybe I exaggerate the bug, I don't know, I'm not an expert. I think it depents on the specific program/code if the bug is critical or not...My program was just relying on the destructor to be called. > Many GUI programs create many widgets at the start of the program, and keep > them until the program ends. In such programs it's no serious bug that the > widgets are not deleted. > It's different if the program creates widgets for a popup window when the > window is shown, deletes the window when it's hidden, and creates a new window > the next time it's shown. Then the leaked memory can be a serious problem.
(In reply to comment #2) > Created an attachment (id=278283) [details] [review] > patch: Gtk::Builder: Fix ref count in get_widget() and get_widget_derived() > > This patch fixes the bug. > > It's risky to push this fix. It may break working programs that have adapted > to the present (erroneous) behaviour of Builder by executing > widget->unreference(); > > A comment from some gtkmm developer would be appreciated. I think it would be fine to push this patch, and any tests. Anybody adding an explicit unreference() would know that they are doing something unusual and fragile that should not be necessary. Also, we are at the start of an release cycle, so we have time to try things out. Many thanks to both of you for finding this and fixing it.
I've pushed the patch. The test case is not well suited for inclusion in gtkmm/tests as it is. (It makes accesses to deallocated memory, with unpredictable results. It only tests one of get_widget() and get_widget_derived() at a time, and must be modified and rebuilt to test the other method.) I'll modify it. I keep this bug open until that's done.
This has triggered an issue in Glom with get_widget_derived(). When using Gtk::Builder to get a non top-level widget, without also getting its parent window (which I guess would "own"/manage it, so you'd delete the window to delete all its widgets) then the widget is deleted when the GtkBuilder is deleted, because the GtkBuilder is the only thing that has a reference to it. I can think of a couple of possible fixes for this: 1. Say that the Gtk::Builder owns the widget if you are not also getting the top-level window, and that, in that case, you must keep the Gtk::Builder alive. 2. reference() the widget if it is not managed/floating. We have some getter for this, I think. Then it would stay alive, as before, until it is explicitly deleted by the app (what I do now in Glom), which would destroy it regardless of any reference count. But I wonder if that would work if we get a child widget before we get the parent widget from the Gtk::Builder.
(In reply to comment #6) > When using > Gtk::Builder to get a non top-level widget, without also getting its parent > window I don't think it matters whether you get the parent window or not. After I pushed the patch in this bug, no reference is added to any widget when get_widget() or get_widget_derived() is called. What matters is if the non top- level widget has a top-level window parent (or grand-parent, or grand-grand- parent and so on). Top-level windows (probably GtkWindow and widgets that inherit from GtkWindow) and all their descendants get two references, one for GtkBuilder, one for the top-level window. Orphaned widgets that are not top-level windows, get only one reference. > 1. Say that the Gtk::Builder owns the widget if you are not also getting the > top-level window, and that, in that case, you must keep the Gtk::Builder > alive. Rather, say that the Gtk::Builder owns the widget if it's not a top-level window, and is not owned (directly or indirectly) by a top-level window, and is not added to a container widget which is not owned by Builder. I prefer this solution, but obviously it requires changes in Glom and probably in other gtkmm applications. It looks like an API break, although Gtk::Builder's behaviour would be closer to the description of Gtk::Builder and GtkBuilder than it has been up to now. > 2. reference() the widget if it is not managed/floating. My short tests indicate that no widgets in a GtkBuilder have floating references. Managed or not managed is indicated by Gtk::Object::referenced_. When Gtk::Builder gets a widget with gtk_builder_get_object(), I think the best way to decide if the widget belongs to a top-level window is to call gtk_widget_is_toplevel(gtk_widget_get_toplevel(widget)). (Confusingly, "toplevel" has different meanings in these two functions. gtk_widget_get_toplevel() gets the top of a container hierarchy. It can be the tested widget itself, or any type of container widget. gtk_widget_is_toplevel() tests if it's a top-level window.)
So, as long as I add the non-Window Widget to a parent widget, I can let the Gtk::Builder die and not bother about deleting the widget later, just as if I had done Gtk::manage(new TheWidget())? And if I don't add it to a Window, it will last as long as the Gtk::Builder, and I don't need to delete it then either. So, I never have to delete a non-Window widget that I get from Gtk::Builder? I'm not too bothered by the API/ABI breakage question here. It's not a particularly common thing to do.
That would mean that I have to be careful to add it to a parent widget before the Gtk::Builder has been deleted, but I guess I can live with that. In my case, I have a local Gtk::Builder instance in a helper function that just gets the Widget*, and I think the Widget is dying before the caller can use it. If I pass in the parent window then I can avoid that, if my comment above is correct.
(In reply to comment #8) > So, as long as I add the non-Window Widget to a parent widget, I can let the > Gtk::Builder die and not bother about deleting the widget later, just as if I > had done Gtk::manage(new TheWidget())? I haven't tested, but I'm convinced it works like that. If you add the widget to a container widget, the container will add a reference, and the widget will be deleted only when both the builder and the container are deleted. > And if I don't add it to a Window, it will last as long as the Gtk::Builder, > and I don't need to delete it then either. Right. > So, I never have to delete a non-Window widget that I get from Gtk::Builder? The description of GtkBuilder explains that pretty well. The description of Gtk::Builder is not quite as clear. We could copy a few more sentences from the GtkBuilder description to Gtk::Builder, now when Gtk::Builder behaves like GtkBuilder.
I've pushed a commit with the new test case gtkmm/tests/builder, and a commit with more documentation of Gtk::Builder.