After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 678737 - Memory leaks and fixes
Memory leaks and fixes
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2012-06-24 20:46 UTC by Jasper St. Pierre (not reading bugmail)
Modified: 2012-06-24 23:21 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
st-widget: Chain up in the dispose handler for accessibility objects (773 bytes, patch)
2012-06-24 20:46 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
st-widget: Fix st_widget_set_theme (1.05 KB, patch)
2012-06-24 20:46 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
st-theme-node: Don't hold a ref to the theme context or the theme (1.54 KB, patch)
2012-06-24 20:46 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
st-widget: Free the inline style (740 bytes, patch)
2012-06-24 20:46 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
test-theme: Fix (699 bytes, patch)
2012-06-24 20:46 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
test-theme: Clean up after ourselves (1.64 KB, patch)
2012-06-24 20:46 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
run-js-test: GC twice after running a test (959 bytes, patch)
2012-06-24 20:46 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
tests: Don't use the default stage (9.95 KB, patch)
2012-06-24 20:46 UTC, Jasper St. Pierre (not reading bugmail)
reviewed Details | Review
tests: Run each test in a function (72.93 KB, patch)
2012-06-24 20:46 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
tests: Don't use the default stage (10.00 KB, patch)
2012-06-24 22:17 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review

Description Jasper St. Pierre (not reading bugmail) 2012-06-24 20:46:09 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.
Comment 1 Jasper St. Pierre (not reading bugmail) 2012-06-24 20:46:11 UTC
Created attachment 217143 [details] [review]
st-widget: Chain up in the dispose handler for accessibility objects
Comment 2 Jasper St. Pierre (not reading bugmail) 2012-06-24 20:46:14 UTC
Created attachment 217144 [details] [review]
st-widget: Fix st_widget_set_theme
Comment 3 Jasper St. Pierre (not reading bugmail) 2012-06-24 20:46:17 UTC
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.
Comment 4 Jasper St. Pierre (not reading bugmail) 2012-06-24 20:46:20 UTC
Created attachment 217146 [details] [review]
st-widget: Free the inline style
Comment 5 Jasper St. Pierre (not reading bugmail) 2012-06-24 20:46:22 UTC
Created attachment 217147 [details] [review]
test-theme: Fix

St requires GTK+ to be initted.
Comment 6 Jasper St. Pierre (not reading bugmail) 2012-06-24 20:46:25 UTC
Created attachment 217148 [details] [review]
test-theme: Clean up after ourselves
Comment 7 Jasper St. Pierre (not reading bugmail) 2012-06-24 20:46:28 UTC
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.
Comment 8 Jasper St. Pierre (not reading bugmail) 2012-06-24 20:46:31 UTC
Created attachment 217150 [details] [review]
tests: Don't use the default stage
Comment 9 Jasper St. Pierre (not reading bugmail) 2012-06-24 20:46:34 UTC
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.
Comment 10 Giovanni Campagna 2012-06-24 21:46:46 UTC
Review of attachment 217143 [details] [review]:

.
Comment 11 Giovanni Campagna 2012-06-24 21:49:20 UTC
Review of attachment 217144 [details] [review]:

The fact that nobody noticed means that is never called. Is it worth keeping it?
(LGTM anyway)
Comment 12 Giovanni Campagna 2012-06-24 21:49:49 UTC
Review of attachment 217145 [details] [review]:

.
Comment 13 Giovanni Campagna 2012-06-24 21:50:17 UTC
Review of attachment 217146 [details] [review]:

.
Comment 14 Giovanni Campagna 2012-06-24 21:51:29 UTC
Review of attachment 217147 [details] [review]:

It checks the result of clutter_init() but not gtk_init(). Why so?
Comment 15 Giovanni Campagna 2012-06-24 21:54:14 UTC
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.
Comment 16 Giovanni Campagna 2012-06-24 22:05:18 UTC
Review of attachment 217149 [details] [review]:

.
Comment 17 Giovanni Campagna 2012-06-24 22:06:44 UTC
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()
Comment 18 Giovanni Campagna 2012-06-24 22:08:20 UTC
Review of attachment 217151 [details] [review]:

.
Comment 19 Jasper St. Pierre (not reading bugmail) 2012-06-24 22:16:13 UTC
Review of attachment 217147 [details] [review]:

gtk_init will exit(1) if it fails. The equivalent to clutter_init is gtk_init_check.
Comment 20 Jasper St. Pierre (not reading bugmail) 2012-06-24 22:16:36 UTC
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.
Comment 21 Jasper St. Pierre (not reading bugmail) 2012-06-24 22:17:06 UTC
Review of attachment 217144 [details] [review]:

Probably not, but it's less effort to fix it up than delete it.
Comment 22 Jasper St. Pierre (not reading bugmail) 2012-06-24 22:17:55 UTC
Created attachment 217158 [details] [review]
tests: Don't use the default stage
Comment 23 Giovanni Campagna 2012-06-24 23:14:58 UTC
Review of attachment 217158 [details] [review]:

This, and the other two, are ACN.
Comment 24 Jasper St. Pierre (not reading bugmail) 2012-06-24 23:20:58 UTC
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