GNOME Bugzilla – Bug 683896
Clean up global resources when the display is closed
Last modified: 2012-10-31 10:57:32 UTC
We keep a bunch of resources around while the toolkit is active and the display is open. It would be nice to clean these up when the display is closed so that, for one, we can have more accurate reporting of leaks with valgrind.
Created attachment 224153 [details] [review] Bind the themes to the lifecycle of the screen
Created attachment 224154 [details] [review] Don't leak a ref to the settings
Created attachment 224155 [details] [review] Destroy the icon factory with the screen
Created attachment 224156 [details] [review] Don't leak a ref to the tooltip window
Created attachment 224157 [details] [review] Destroy the legacy style with the screen
Review of attachment 224154 [details] [review]: This one looks straightforward.
Review of attachment 224156 [details] [review]: Nice catch...I don't see any reason why we need to take an extra reference to the GtkWindow here.
Review of attachment 224155 [details] [review]: ::: gtk/gtkiconfactory.c @@ +415,3 @@ + GdkScreen *screen = gdk_screen_get_default (); + + if (screen) Maybe it doesn't really matters, but can it ever happen that the screen is NULL at this point? Maybe it's better to tie this to the default GtkIconTheme instead? I don't completely like the fact that an ensure function becomes a get that can fail.
Review of attachment 224153 [details] [review]: ::: gtk/gtkcssprovider.h @@ +90,3 @@ +GtkCssProvider * gtk_css_provider_get_named_for_screen (GdkScreen *screen, + const gchar *name, + const gchar *variant); I'm not sure we need these methods as public API, and it looks like GtkSettings only needs gtk_css_provider_get_named_for_screen(); I think it would be fine for now to add that one to gtkcssproviderprivate.h and only use it internally.
Review of attachment 224157 [details] [review]: ::: gtk/deprecated/gtkstyle.c @@ +4048,3 @@ + return gtk_widget_get_default_style_for_screen (screen); + else + return NULL; Again I don't really like the fact that we're slightly changing the semantics of the function here. If this could indeed be called with no default screen set yet, I think we should still return a new GtkStyle; maybe we can keep the static pointer around and defer the gobject data association to the first call where screen is != NULL?
Attachment 224153 [details] pushed as 1f5dea9 - Bind the themes to the lifecycle of the screen Attachment 224154 [details] pushed as 8a9a394 - Don't leak a ref to the settings Attachment 224155 [details] pushed as 9bd408a - Destroy the icon factory with the screen Attachment 224156 [details] pushed as 92ddf14 - Don't leak a ref to the tooltip window Attachment 224157 [details] pushed as e044676 - Destroy the legacy style with the screen
Thanks! However I think there might have been an issue with the way 1f5dea9 was rebased. We may have lost the gtk_css_provider_get_named symbol.
Created attachment 224495 [details] [review] Add back gtk_css_provider_get_named Was in the original patch but was not in 1f5dea9 probably due to a bad rebase.
The first of these caused problems, and I've reverted it: http://git.gnome.org/browse/gtk+/commit/?id=ab3d6a0b0a7a48914a30187c12c8219a8bcc3295 If we want to avoid leaking that hash table, tying it to the screen seems really arbitrary, anyway. Maybe we need a hookable shutdown.
copy/paste from IRC: <Company> what I would like is a gtk_css_provider_load_named() function <Company> that way you can load your own instance of a named theme for whatever purpose <Company> and, you can unload and reload it <Company> and then deprecate get_named() for it <Company> that gives us: <Company> + others can load themes for whatever reason and do whatever they want with it <Company> + others can hook into the loading process to catch errors etc <Company> + you can reload the theme (after making changes to it) <Company> + you have no "memory leak" <Company> but it also gives you: <Company> - it's harder to get at the currently used system theme <Company> - themes aren't shared Of course, we'd make the settings object use this function instead of gtk_css_provider_get_named(), so we would never reach that hash table in normal operations
I've pushed a branch at http://git.gnome.org/browse/gtk+/log/?h=wip/683896 that should fix any leaks. Testing appreciated. Also, it adds the possibility to export gtk_theme_provider_load_named() in the public API and deprecating gtk_theme_provider_get_named(), but that's another discussion.