GNOME Bugzilla – Bug 697395
Another round of background leak fixes
Last modified: 2021-07-05 14:43:20 UTC
I've hacked up gjs to print whenever an object is created and whenever it's destroyed. I've combined that with continuously running the garbage collector and found a few more leaks. I'll attach patches for those here.
Created attachment 240802 [details] [review] background: don't leak background objects during quick changes We currently let some backgrounds "fall through the cracks" if a bunch of change notifications come in at once. This commit fixes that.
Created attachment 240803 [details] [review] background: when updating image remove old one from cache We're removing the new content from the cache instead of the old content. This commit fixes that.
Created attachment 240804 [details] [review] background: stop monitoring file after removing from cache
Created attachment 240805 [details] [review] backgroundMenu: drop cursor object with rest of menu actors
There's still one problem left: + StBoxLayout + StBin + ShellGenericContainer + StBin + StDrawingArea + ShellGenericContainer + ShellGenericContainer + StLabel + ShellGenericContainer + StDrawingArea + ShellGenericContainer + StLabel This is the backgroundMenu, so it suggests bug 697119 isn't completely fixed.
Review of attachment 240802 [details] [review]: OK.
Review of attachment 240803 [details] [review]: Oh ... good catch.
Review of attachment 240804 [details] [review]: ::: js/ui/background.js @@ +130,3 @@ + + if (filename && this._fileMonitors[filename]) + delete this._fileMonitors[filename]; Is this enough? i.e does the GC dispose the monitor object?
Review of attachment 240805 [details] [review]: LG.
(In reply to comment #8) > Review of attachment 240804 [details] [review]: > > ::: js/ui/background.js > @@ +130,3 @@ > + > + if (filename && this._fileMonitors[filename]) > + delete this._fileMonitors[filename]; > > Is this enough? i.e does the GC dispose the monitor object? Yup. After adding that patch my gjs output started getting - GInotifyFileMonitor
Attachment 240802 [details] pushed as 9eae743 - background: don't leak background objects during quick changes Attachment 240803 [details] pushed as e98c6ff - background: when updating image remove old one from cache Attachment 240805 [details] pushed as dc98711 - backgroundMenu: drop cursor object with rest of menu actors
Too be more clear about my process, I added to gjs: + g_printerr("+ %s\n", g_type_name(priv->gtype)); in associate_js_gobject and + g_printerr("- %s\n", g_type_name_from_instance( (GTypeInstance*) priv->gobj)); in object_instance_finalize. Then I started the shell and opened the looking glass console and wrote: GLib.timeout_add(GLib.PRIORITY_DEFAULT, 500, function() { System.gc(); return true; }) then opened a shell and typed: while true; do for f in /usr/share/backgrounds/gnome/*; do gsettings set org.gnome.desktop.background picture-uri file://$f; sleep 10; done; done This makes the background change every 10 seconds, but makes the system garbage collect continuously. When the the background is changed I see all the new background objects getting created, and what old background objects get freed.
Review of attachment 240804 [details] [review]: OK.
Attachment 240804 [details] pushed as 0376f22 - background: stop monitoring file after removing from cache
all committed here
(i left open because of comment 5)
Created attachment 240998 [details] [review] popupMenu: fix leak on destroy Menus live past destruction because of their delegate object property. This commit clears the propery on destroy and breaks the reference cycle. Fixes a leak in the background menu when changing backgrounds.
Review of attachment 240998 [details] [review]: LG.
Review of attachment 240998 [details] [review]: Wait a minute. Why isn't the mark and sweep GC taking care of this and disposing the menu?
(In reply to comment #19) > Wait a minute. Why isn't the mark and sweep GC taking care of this and > disposing the menu? I'm not completely sure. What I do know is: 1) this fixes the leak: + this.actor.connect('destroy', Lang.bind(this, function() { + this.actor._delegate = null; + })); + 2) this maintains the leak: + this.actor.connect('destroy', Lang.bind(this, function() { + this.actor._otherProperty = this; + this.actor._delegate = null; + })); +
We use the _delegate pattern all over the Shell (the DND stuff depends on it, even), so we really need to investigate why the cycle isn't being collected.
We also set _delegate to null in most destroy() functions and/or destroy signal handlers. I'm pretty curious too, though.
(In reply to comment #19) > Review of attachment 240998 [details] [review]: > > Wait a minute. Why isn't the mark and sweep GC taking care of this and > disposing the menu? Hmm... good question.
Review of attachment 240998 [details] [review]: Jasper is right. This seem to just paper over the real bug. If we really want it we need a bug file for the GC issue and a comment (unless this turns out to not be a GC bug).
(In reply to comment #24) > Review of attachment 240998 [details] [review]: > > Jasper is right. This seem to just paper over the real bug. > If we really want it we need a bug file for the GC issue and a comment (unless > this turns out to not be a GC bug). Mozilla has a "cycle collector" for exactly this kind of issue. GObject toggle references bridge the two worlds of refcounting and GC, but they can't handle what we have here, which is a circular reference *across* the two worlds. The GObject signal connection forces the function GC-alive, which in turn keeps the "this" object GC-alive. We have a toggle and we're the last ref, so we don't have a keepalive on "this", but the Spidermonkey GC can't see "through" the gobject world. So yes, for now we need to avoid keeping references to "this" inside signal callbacks.
(In reply to comment #25) > The GObject signal connection forces the function GC-alive, I was wrong about this, Jasper pointed out that that's not true since https://git.gnome.org/browse/gjs/commit/gi/closure.c?id=06aa616a8c9b6d356aefc95a6b0c1c317b86c46a
Is this still relevant following the rework of the background code in 3.14?
(In reply to comment #27) > Is this still relevant following the rework of the background code in 3.14? The remaining uncommited patch has nothing to do with the background (just the popup menu)
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/ Thank you for your understanding and your help.