GNOME Bugzilla – Bug 678737
Memory leaks and fixes
Last modified: 2012-06-24 23:21:22 UTC
A grab bag of assorted fixes for the shell. Some are about stopping things like reference cycles. Others are about making our tests and things easier to valgrind.
Created attachment 217143 [details] [review] st-widget: Chain up in the dispose handler for accessibility objects
Created attachment 217144 [details] [review] st-widget: Fix st_widget_set_theme
Created attachment 217145 [details] [review] st-theme-node: Don't hold a ref to the theme context or the theme This would just be a circular reference, which we need to avoid.
Created attachment 217146 [details] [review] st-widget: Free the inline style
Created attachment 217147 [details] [review] test-theme: Fix St requires GTK+ to be initted.
Created attachment 217148 [details] [review] test-theme: Clean up after ourselves
Created attachment 217149 [details] [review] run-js-test: GC twice after running a test When running with Valgrind, this helps us ensure that we're managing memory correctly. We need to GC twice as finalizing an object in the sweep can unroot objects which were already marked. Technically, it could be that we'll need to GC more than twice, but GCing twice should hopefully last us for now.
Created attachment 217150 [details] [review] tests: Don't use the default stage
Created attachment 217151 [details] [review] tests: Run each test in a function As the global object of a context is rooted, if we want the GC to act on these objects we need to take them out of the globals.
Review of attachment 217143 [details] [review]: .
Review of attachment 217144 [details] [review]: The fact that nobody noticed means that is never called. Is it worth keeping it? (LGTM anyway)
Review of attachment 217145 [details] [review]: .
Review of attachment 217146 [details] [review]: .
Review of attachment 217147 [details] [review]: It checks the result of clutter_init() but not gtk_init(). Why so?
Review of attachment 217148 [details] [review]: I wonder if something like glocal_object (https://mail.gnome.org/archives/gtk-devel-list/2012-May/msg00078.html) would be useful here.
Review of attachment 217149 [details] [review]: .
Review of attachment 217150 [details] [review]: ::: tests/interactive/borders.js @@ +8,1 @@ let stage = Clutter.Stage.get_default(); new Clutter.Stage()? ::: tests/interactive/calendar.js @@ +8,3 @@ const UI = imports.testcommon.ui; +let stage = Clutter.Stage.get_default({ width: 400, height: 400 }); new Clutter.Stage()
Review of attachment 217151 [details] [review]: .
Review of attachment 217147 [details] [review]: gtk_init will exit(1) if it fails. The equivalent to clutter_init is gtk_init_check.
Review of attachment 217148 [details] [review]: It would, but so would a lot of things, and I'm not going to block this patch on that.
Review of attachment 217144 [details] [review]: Probably not, but it's less effort to fix it up than delete it.
Created attachment 217158 [details] [review] tests: Don't use the default stage
Review of attachment 217158 [details] [review]: This, and the other two, are ACN.
Attachment 217143 [details] pushed as 560daec - st-widget: Chain up in the dispose handler for accessibility objects Attachment 217144 [details] pushed as a347a72 - st-widget: Fix st_widget_set_theme Attachment 217145 [details] pushed as 7eaf231 - st-theme-node: Don't hold a ref to the theme context or the theme Attachment 217146 [details] pushed as 69e1503 - st-widget: Free the inline style Attachment 217147 [details] pushed as 9e25e13 - test-theme: Fix Attachment 217148 [details] pushed as ab75faa - test-theme: Clean up after ourselves Attachment 217149 [details] pushed as 3ce9ad0 - run-js-test: GC twice after running a test Attachment 217151 [details] pushed as c7196a5 - tests: Run each test in a function Attachment 217158 [details] pushed as 0d82ce5 - tests: Don't use the default stage