GNOME Bugzilla – Bug 772859
[patch x7] Fix memory leaks in implementations of common widgets
Last modified: 2018-05-02 17:36:46 UTC
GTK causes Nautilus to leak over 1MB per window (#772598). (Technically "still reachable" instead of leaked, because of how GTK manages memory). With this patch series, per-window leakage falls to about 100kB. I expect the exact figures are variable, but it's a notable difference.
Created attachment 337599 [details] [review] [PATCH 1/7] applicationwindow: fix leak of help_overlay
Created attachment 337600 [details] [review] [PATCH 2/7] shortcutswindow: fix leak of internal widgets
Created attachment 337601 [details] [review] [PATCH 3/7] shortcutsgroup: fix leak of title
Created attachment 337602 [details] [review] [PATCH 4/7] headerbar: fix leak of start_box/end_box
Created attachment 337603 [details] [review] [PATCH 5/7] headerbar: fix leak of separator
Created attachment 337604 [details] [review] [PATCH 6/7] headerbar: fix leak of label_sizing_box
Created attachment 337605 [details] [review] [PATCH 7/7] scrolledwindow: fix leak of pan_gesture
Review of attachment 337599 [details] [review]: Looks right. Just a few coding style niggles: we put a space before ( in function calls.
Review of attachment 337600 [details] [review]: this will need very careful scrutiny. The whole container setup for the shortcuts widgets is such a disaster
Review of attachment 337601 [details] [review]: ok
Review of attachment 337602 [details] [review]: ok
Review of attachment 337603 [details] [review]: ok
Review of attachment 337604 [details] [review]: ok. Again, please put a space before ( in function calls
Review of attachment 337605 [details] [review]: sure
Review of attachment 337600 [details] [review]: thx for fixups :) (other patches). I agree it is a hard nut to crack. Sadly this one must be what saves the most memory (~ 800kB). At least there's no dynamic variation in the lifetimes. It just goes _init -> _add(widgets) -> _dispose -> _remove(widgets). And it passed valgrind for memory-safety. There are some possible cleanups. Equal scrutiny required, just that it might help to list different options. * exclude dangling pointers by carpet-bombing with weak_pointer(), so any bugs would give nice obvious segfaults. * don't call _dispose() -> widget_destroy() -> _remove(). Instead, have _dispose() call parent_class->remove(). (could do both of these) or * remove override of forall(), remove(), and add(). Override gtk_buildable_add_child() instead. Using GtkBuilder is "the recommended way", and the _add() behaviour is not explicitly documented.
(In reply to Alan Jenkins from comment #15) > > * remove override of forall(), remove(), and add(). Override > gtk_buildable_add_child() instead. Using GtkBuilder is "the recommended > way", and the _add() behaviour is not explicitly documented. Uh, please, no. Any fix for the leak should be done properly, not by removing people's ability to programmatically create these widgets, just for an easy ride. Specifically, there are those of us who don't normally use builder but still want to use GtkShortcuts*. For that case, Benjamin wants to add proper API for these widgets - to replace the current requirement to use raw GObject calls - saying it seems really wrong that it's like this in the first place I've opened a BZ about the lack of API: https://bugzilla.gnome.org/show_bug.cgi?id=770377
(In reply to djb from comment #16) > (In reply to Alan Jenkins from comment #15) > > > > * remove override of forall(), remove(), and add(). Override > > gtk_buildable_add_child() instead. Using GtkBuilder is "the recommended > > way", and the _add() behaviour is not explicitly documented. > > Uh, please, no. > Specifically, there are those of us who don't normally use builder but still > want to use GtkShortcuts*. For that case, Benjamin wants to add proper API > for these widgets - to replace the current requirement to use raw GObject > calls - saying it seems really wrong that it's like this in the first place Your scrutiny is appreciated :). I did consider something, just like gtk_places_sidebar_add_shortcut(). That _would_ be an API. The problem is narrower: it wouldn't be backwards compatible. But I see you mention _existing_ code that doesn't use GtkBuilder :(. I suspect this approach would also fix the issue you found, that show_all() isn't working as normal. (I'm not sure your explanations are consistent - if show_all() doesn't show the sections, then why would you not need to show the second section? And I know the overridden foreach() is freaky, but iterating the sections is exactly what it's supposed to do. So I'm kind of scared to know why show_all() doesn't do the job). One way to notice the "disaster" of shortcutswindow implementation, is that you're not allowed to write it outside of GTK. gtkcontainer literally has a macro called SPECIAL_CONTAINER to identify the "containers inside GTK+ [which] are cheating". And so, it ends up needing this "very special scrutiny". Because it's not enough to understand the public GTK api; you need to reason about what happens when you're "cheating".
(In reply to Alan Jenkins from comment #17) > > Your scrutiny is appreciated :). Likewise, I love seeing leaks getting plugged. :D > I did consider something, just like gtk_places_sidebar_add_shortcut(). That > _would_ be an API. The problem is narrower: it wouldn't be backwards > compatible. But I see you mention _existing_ code that doesn't use > GtkBuilder :(. Pitivi has user-settable accelerators and (via the Python bindings) generates GtkShortcuts* widgets at runtime, which seems like a significant existing use. I've only built proof-of-concepts using these widgets - partly because the lack of API made me nervous about their stability. However, I have code that would be perfectly paired up with dynamically generated Shortcuts widgets, so I'd like to be able to do it with guaranteed stable API and consistent code style. > I suspect this approach would also fix the issue you found, that show_all() > isn't working as normal. (I'm not sure your explanations are consistent - > if show_all() doesn't show the sections, then why would you not need to show > the second section? And I know the overridden foreach() is freaky, but > iterating the sections is exactly what it's supposed to do. So I'm kind of > scared to know why show_all() doesn't do the job). It was Matthias's explanation, not mine. He said the ShortcutsWindow takes over visibility management but respects the initial setting of added children. My interpretation is that the Window can newly show Section if it's switched-to from _another_ Section - but if the Window is opened with _no_ Sections visible, it doesn't show a default Section, so there's nothing to switch from. That said, I'm not so clear on the LTR vs RTL explanation about why this can't be done, but then I know next to nothing about i18n (only that Christian has recently changed things so that accelerator labels are always shown as LTR). > One way to notice the "disaster" of shortcutswindow implementation, is that > you're not allowed to write it outside of GTK. gtkcontainer literally has a > macro called SPECIAL_CONTAINER to identify the "containers inside GTK+ > [which] are cheating". > > And so, it ends up needing this "very special scrutiny". Because it's not > enough to understand the public GTK api; you need to reason about what > happens when you're "cheating". I haven't looked into the widgets in this much depth. I guess if I ever have time to try my hand at a patch for that ticket, I'd find out. I suspect it's a bit over my head at this point, though.
*** Bug 772598 has been marked as a duplicate of this bug. ***
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gtk/issues/684.