File bnc963664-sle-background-memory-leaks-misc-fixes.patch of Package gnome-shell.2367
Index: gnome-shell-3.10.4/js/ui/background.js
===================================================================
--- gnome-shell-3.10.4.orig/js/ui/background.js
+++ gnome-shell-3.10.4/js/ui/background.js
@@ -32,6 +32,10 @@ const ANIMATION_MIN_WAKEUP_INTERVAL = 1.
let _backgroundCache = null;
+// NOTE: the caching role is for various BMs like the normal desktop background,
+// overview background, screen shield background, thumbnail backgrounds and etc,
+// *NOT* for different backgrounds across selections. This is a major
+// misunderstanding I had.
const BackgroundCache = new Lang.Class({
Name: 'BackgroundCache',
@@ -40,6 +44,7 @@ const BackgroundCache = new Lang.Class({
this._images = [];
this._pendingFileLoads = [];
this._fileMonitors = {};
+ // NOTE: in recent GS, `_animations` is schema-keyed hash.
this._animations = [];
},
@@ -53,6 +58,12 @@ const BackgroundCache = new Lang.Class({
let content = null;
let candidateContent = null;
+ // FIX: add exact match check to avoid unnecessary copy
+ //
+ // NOTE: the use of `candidateIsFullMatch` helps reduce the memory but
+ // the content removing code is dumb and might lead to premature content
+ // removal.
+ let candidateIsFullMatch = false;
for (let i = 0; i < this._patterns.length; i++) {
if (this._patterns[i].get_shading() != params.shadingType)
continue;
@@ -69,11 +80,17 @@ const BackgroundCache = new Lang.Class({
if (params.effects != this._patterns[i].effects)
continue;
+ candidateIsFullMatch = true;
break;
}
if (candidateContent) {
- content = candidateContent.copy(params.monitorIndex, params.effects);
+ if (candidateIsFullMatch) {
+ content = candidateContent;
+ } else {
+ content = candidateContent.copy(params.monitorIndex, params.effects);
+ this._patterns.push(content);
+ }
} else {
content = new Meta.Background({ meta_screen: global.screen,
monitor: params.monitorIndex,
@@ -84,9 +101,11 @@ const BackgroundCache = new Lang.Class({
} else {
content.load_gradient(params.shadingType, params.color, params.secondColor);
}
+
+ // FIX: push the new content, only when it's NEW
+ this._patterns.push(content);
}
- this._patterns.push(content);
return content;
},
@@ -112,6 +131,14 @@ const BackgroundCache = new Lang.Class({
monitor.disconnect(signalId);
this.emit('file-changed', filename);
+
+ // FIX: also remove this file monitor
+ // here. It should get removed while
+ // the related content is removed
+ // from `_images` but as noted in
+ // `_removeImageContent` there are
+ // some problems in relying on it.
+ delete this._fileMonitors[filename];
}));
this._fileMonitors[filename] = monitor;
@@ -124,15 +151,30 @@ const BackgroundCache = new Lang.Class({
contentList.splice(index, 1);
},
+ // FIX add animation cache management code, when background `_destroy`, it
+ // should call this function if it's an Animation.
+ removeAnimation: function(animation) {
+ delete this._fileMonitors[animation.filename];
+
+ this._animations = this._animations.filter(function(anime, idx){
+ return anime !== animation;
+ });
+ },
+
removePatternContent: function(content) {
this._removeContent(this._patterns, content);
},
removeImageContent: function(content) {
- let filename = content.get_filename();
-
- if (filename && this._fileMonitors[filename])
- delete this._fileMonitors[filename];
+ // TODO: one file can be loaded as different content and thus the
+ // following direct removal is too simplistic. We need to track refs to
+ // file to properly remove them. Here, we do NOT remove file watches (a
+ // leak). Enable this to sacrifice file watching in exchange of less
+ // memory drain see bnc#963664 (until we have proper ref count).
+ //
+ // let filename = content.get_filename();
+ // if (filename && this._fileMonitors[filename])
+ // delete this._fileMonitors[filename];
this._removeContent(this._images, content);
},
@@ -143,6 +185,10 @@ const BackgroundCache = new Lang.Class({
if (!caller.cancellable)
return;
+ // NOTE: connect to a signal `cancelled`, looks weird but seems to be
+ // valid, see https://www.roojs.com/seed/gir-1.2-gtk-3.0/gjs/Gio.Cancellable.html#expand
+ //
+ // This use of `connect` seems to be a special case
caller.cancellable.connect(Lang.bind(this, function() {
let idx = fileLoad.callers.indexOf(caller);
fileLoad.callers.splice(idx, 1);
@@ -183,39 +229,62 @@ const BackgroundCache = new Lang.Class({
cancellable: new Gio.Cancellable(),
callers: [] };
this._attachCallerToFileLoad(caller, fileLoad);
+ // FIX: add this fileLoad to `_pendingFileLoads`, o/w we'll not be above
+ // to group callers together
+ this._pendingFileLoads.push(fileLoad);
+ // NOTE: do not set `monitorIndex` so later it will get re-copied with
+ // right caller info
let content = new Meta.Background({ meta_screen: global.screen });
- content.load_file_async(params.filename,
- params.style,
- params.cancellable,
- Lang.bind(this,
- function(object, result) {
- try {
- content.load_file_finish(result);
-
- this._monitorFile(params.filename);
- } catch(e) {
- content = null;
- }
-
- for (let i = 0; i < fileLoad.callers.length; i++) {
- let caller = fileLoad.callers[i];
- if (caller.onFinished) {
- let newContent;
-
- if (content) {
- newContent = content.copy(caller.monitorIndex, caller.effects);
- this._images.push(newContent);
- }
-
- caller.onFinished(newContent);
- }
- }
-
- let idx = this._pendingFileLoads.indexOf(fileLoad);
- this._pendingFileLoads.splice(idx, 1);
- }));
+ content.load_file_async(
+ params.filename,
+ params.style,
+ params.cancellable,
+ Lang.bind(this,
+ function(object, result) {
+ try {
+ content.load_file_finish(result);
+
+ this._monitorFile(params.filename);
+ } catch(e) {
+ content = null;
+ }
+
+ // TODO: from my log, there is only one caller for fileLoad
+ // but this piece of code can run up to 6 times, with some of
+ // them having effects set to 2(Overview BG). This leads to
+ // multiple same entries in `cache._images`
+
+ // FIX: a temporary area to avoid duplicates in cache `_images`.
+ let newContents = [];
+ let newContent = content;
+ for (let i = 0; i < fileLoad.callers.length; i++) {
+ let caller = fileLoad.callers[i];
+
+ if (content) {
+ newContent = content.copy(caller.monitorIndex, caller.effects);
+
+ // only cache this content when other callers have NOT cached it.
+ let shouldCache = ! newContents.some(function(ctn) {
+ return ( (ctn.monitor === caller.monitorIndex)
+ && (ctn.effects === caller.effects) );
+ });
+ if (shouldCache) {
+ newContents.push(newContent);
+ this._images.push(newContent);
+ }
+ }
+
+ // run `onFinished` despite caching condition
+ if (caller.onFinished) {
+ caller.onFinished(newContent);
+ }
+ }
+
+ let idx = this._pendingFileLoads.indexOf(fileLoad);
+ this._pendingFileLoads.splice(idx, 1);
+ }));
},
getImageContent: function(params) {
@@ -229,6 +298,7 @@ const BackgroundCache = new Lang.Class({
let content = null;
let candidateContent = null;
+ let candidateIsFullMatch = false;
for (let i = 0; i < this._images.length; i++) {
if (this._images[i].get_style() != params.style)
continue;
@@ -242,19 +312,24 @@ const BackgroundCache = new Lang.Class({
candidateContent = this._images[i];
+ // NOTE `effects` field does exist, just not enlisted in the LookingGlass....
if (params.effects != this._images[i].effects)
continue;
+ candidateIsFullMatch = true;
break;
}
if (candidateContent) {
- content = candidateContent.copy(params.monitorIndex, params.effects);
+ if (candidateIsFullMatch) {
+ content = candidateContent;
+ } else {
+ content = candidateContent.copy(params.monitorIndex, params.effects);
+ this._images.push(content);
+ }
if (params.cancellable && params.cancellable.is_cancelled())
content = null;
- else
- this._images.push(content);
if (params.onFinished)
params.onFinished(content);
@@ -276,25 +351,22 @@ const BackgroundCache = new Lang.Class({
let i;
let animation = null;
for (i = 0; i < this._animations.length; i++) {
- if (this._animations[i].filename == params.filename) {
+ if (this._animations[i].filename === params.filename) {
animation = this._animations[i];
- if (params.onLoaded) {
- let id = GLib.idle_add(GLib.PRIORITY_DEFAULT, Lang.bind(this, function() {
- params.onLoaded(this._animation);
- return GLib.SOURCE_REMOVE;
- }));
- GLib.Source.set_name_by_id(id, '[gnome-shell] params.onLoaded');
- }
+
+ // FIX: a match has been found
+ break;
}
}
if (animation == null) {
animation = new Animation({ filename: params.filename });
this._animations.push(animation);
+
+ // FIX: only monitor new animation's file
+ this._monitorFile(params.filename);
}
animation.load(Lang.bind(this, function() {
- this._monitorFile(params.filename);
-
if (params.onLoaded) {
let id = GLib.idle_add(GLib.PRIORITY_DEFAULT, Lang.bind(this, function() {
params.onLoaded(animation);
@@ -336,12 +408,18 @@ const Background = new Lang.Class({
// contains a single image for static backgrounds and
// two images (from and to) for slide shows
this._images = {};
+ // FIX: declare `_animation` here to avoid confusion
+ this._animation = null;
+ // FIX: declare here to avoid confusion
+ this._updateAnimationTimeoutId = 0;
this._brightness = 1.0;
this._vignetteSharpness = 0.2;
this._cancellable = new Gio.Cancellable();
this.isLoaded = false;
+ // NOTE looks weird to me. shouldn't `changed` signal on settings be attached to BM instead?
+ // However, the end result is the same and should not affect functionality.
this._settingsChangedSignalId = this._settings.connect('changed', Lang.bind(this, function() {
this.emit('changed');
}));
@@ -364,6 +442,10 @@ const Background = new Lang.Class({
}
this._fileWatches = null;
+ if (this._animation) {
+ this._cache.removeAnimation(this._animation);
+ this._animation = null;
+ }
if (this._pattern) {
if (this._pattern.content)
this._cache.removePatternContent(this._pattern.content);
@@ -389,6 +471,9 @@ const Background = new Lang.Class({
if (this._settingsChangedSignalId != 0)
this._settings.disconnect(this._settingsChangedSignalId);
this._settingsChangedSignalId = 0;
+
+ // FIX: reset `isLoaded`
+ this.isLoaded = false;
},
_setLoaded: function() {
@@ -397,10 +482,14 @@ const Background = new Lang.Class({
this.isLoaded = true;
- GLib.idle_add(GLib.PRIORITY_DEFAULT, Lang.bind(this, function() {
+ // NOTE: the extra waiting seems to avoid blocking the main loop as
+ // there might be multiple heavy handlers attached to `loaded` signal.
+ let id = GLib.idle_add(GLib.PRIORITY_DEFAULT, Lang.bind(this, function() {
this.emit('loaded');
- return false;
+ return GLib.SOURCE_REMOVE;
}));
+
+ GLib.Source.set_name_by_id(id, '[gnome-shell] this.emit');
},
_loadPattern: function() {
@@ -438,6 +527,7 @@ const Background = new Lang.Class({
this._fileWatches[filename] = signalId;
},
+ // NOTE maybe more properly, `_ensureImageActor`
_ensureImage: function(index) {
if (this._images[index])
return;
@@ -450,13 +540,14 @@ const Background = new Lang.Class({
this._images[index] = actor;
},
+ // NOTE maybe more properly, `_updateImageContent`
_updateImage: function(index, content, filename) {
content.brightness = this._brightness;
content.vignette_sharpness = this._vignetteSharpness;
let image = this._images[index];
if (image.content)
- this._cache.removeImageContent(content);
+ this._cache.removeImageContent(image.content);
image.content = content;
this._watchCacheFile(filename);
},
@@ -536,29 +627,33 @@ const Background = new Lang.Class({
if (interval > GLib.MAXUINT32)
return;
- this._updateAnimationTimeoutId = GLib.timeout_add(GLib.PRIORITY_DEFAULT,
- interval,
- Lang.bind(this, function() {
- this._updateAnimationTimeoutId = 0;
- this._updateAnimation();
- return false;
- }));
+ this._updateAnimationTimeoutId = GLib.timeout_add(
+ GLib.PRIORITY_DEFAULT,
+ interval,
+ Lang.bind(this, function() {
+ this._updateAnimationTimeoutId = 0;
+ this._updateAnimation();
+ return GLib.SOURCE_REMOVE;
+ }));
+
+ GLib.Source.set_name_by_id(this._updateAnimationTimeoutId,
+ '[gnome-shell] this._updateAnimation');
},
_loadAnimation: function(filename) {
this._cache.getAnimation({ filename: filename,
- onLoaded: Lang.bind(this, function(animation) {
- this._animation = animation;
+ onLoaded: Lang.bind(this, function(animation) {
+ this._animation = animation;
- if (!this._animation || this._cancellable.is_cancelled()) {
- this._setLoaded();
- return;
- }
-
- this._updateAnimation();
- this._watchCacheFile(filename);
- })
- });
+ if (!this._animation || this._cancellable.is_cancelled()) {
+ this._setLoaded();
+ return;
+ }
+
+ this._updateAnimation();
+ this._watchCacheFile(filename);
+ })
+ });
},
_loadImage: function(filename) {
@@ -587,8 +682,9 @@ const Background = new Lang.Class({
_load: function () {
this._cache = getBackgroundCache();
- this._loadPattern(this._cache);
+ this._loadPattern();
+ // NOTE: the following says: no image will be used so only show the pattern
this._style = this._settings.get_enum(BACKGROUND_STYLE_KEY);
if (this._style == GDesktopEnums.BackgroundStyle.NONE) {
this._setLoaded();
@@ -666,7 +762,11 @@ const SystemBackground = new Lang.Class(
},
_onDestroy: function() {
- this._cache.removeImageContent(this.actor.content);
+ // FIX: upstream cfef107114
+ let content = this.actor.content;
+
+ if (content)
+ this._cache.removeImageContent(content);
},
});
Signals.addSignalMethods(SystemBackground.prototype);
@@ -755,30 +855,39 @@ const BackgroundManager = new Lang.Class
}
},
- _updateBackground: function(background) {
+ _updateBackground: function() {
let newBackground = this._createBackground();
- newBackground.vignetteSharpness = background.vignetteSharpness;
- newBackground.brightness = background.brightness;
- newBackground.visible = background.visible;
+ newBackground.vignetteSharpness = this.background.vignetteSharpness;
+ newBackground.brightness = this.background.brightness;
+ newBackground.visible = this.background.visible;
newBackground.loadedSignalId = newBackground.connect('loaded',
Lang.bind(this, function() {
newBackground.disconnect(newBackground.loadedSignalId);
newBackground.loadedSignalId = 0;
- Tweener.addTween(background.actor,
+ // NOTE: new background is loaded *asynchronously*, while
+ // `this._newBackground` holds the *newest* background. The
+ // following check make sure what gets loaded is still relevant.
+ //
+ // Backported from upstream commits: 933f38390ba1512aa and d6197b0904fa.
+ if (this._newBackground != newBackground) {
+ /* Not interesting, we queued another load */
+ newBackground.actor.destroy();
+ return;
+ }
+ Tweener.addTween(this.background.actor,
{ opacity: 0,
time: FADE_ANIMATION_TIME,
transition: 'easeOutQuad',
onComplete: Lang.bind(this, function() {
- if (this._newBackground == newBackground) {
- this.background = newBackground;
- this._newBackground = null;
- } else {
- newBackground.actor.destroy();
- }
-
- background.actor.destroy();
-
+ this.background.actor.destroy();
+ this.background = newBackground;
+ this._newBackground = null;
+
+ // FIX: "changed" signal is emitted
+ // and old background destroyed *only
+ // when* changes really happen
+ // see c#24 for the discussion.
this.emit('changed');
})
});
@@ -805,13 +914,14 @@ const BackgroundManager = new Lang.Class
background.changeSignalId = background.connect('changed', Lang.bind(this, function() {
background.disconnect(background.changeSignalId);
background.changeSignalId = 0;
- this._updateBackground(background);
+ this._updateBackground();
}));
background.actor.connect('destroy', Lang.bind(this, function() {
if (background.changeSignalId)
background.disconnect(background.changeSignalId);
+ // NOTE: loadedSignalId is set in _updateBackground
if (background.loadedSignalId)
background.disconnect(background.loadedSignalId);
}));
Index: gnome-shell-3.10.4/src/shell-global.c
===================================================================
--- gnome-shell-3.10.4.orig/src/shell-global.c
+++ gnome-shell-3.10.4/src/shell-global.c
@@ -1417,6 +1417,13 @@ run_leisure_functions (gpointer data)
if (global->work_count > 0)
return FALSE;
+ /* MEM-FIX: run gc at tweener's end, this reduces the performance but avoid memory leaks
+ * see http://bazaar.launchpad.net/~ubuntu-branches/ubuntu/wily/gnome-shell/wily/view/head:/debian/patches/revert-disable-periodic-gc.patch
+ *
+ * NOTE: a bit overkill to fix bg memory problem, what I want is simply run gc after every background change not every tweener.
+ */
+ gjs_context_gc (global->js_context);
+
/* No leisure closures, so we are done */
if (global->leisure_closures == NULL)
return FALSE;
