Created attachment 240404 [details] [review]
background: disconnect settings object when destroyed

We don't want the settings object to outlive the background
actor. It keeps the background object around longer than
necessary.

Created attachment 240405 [details] [review]
backgroundMenu: destroy menu when actor is destroyed

This fixes a leak.

Review of attachment 240404 [details] [review]:

Well, if the background object is GCed, so are the settings, because nothing else keeps them alive.

Review of attachment 240405 [details] [review]:

I copied this mostly from shellEntry; it seems that has the same leak?

::: js/ui/backgroundMenu.js
@@ +58,3 @@
+
+    actor.connect('destroy', function() {
+                      actor._backgroundMenu.destroy()

semi

Review of attachment 240405 [details] [review]:

Ok

Review of attachment 240404 [details] [review]:

The background object isn't guaranteed to be GCed at destroy time. In fact, the GC doesn't run that often now because we disabled periodic GC because of the deadlock problem.

(In reply to comment #6)
> Review of attachment 240404 [details] [review]:
> 
> The background object isn't guaranteed to be GCed at destroy time. In fact, the
> GC doesn't run that often now because we disabled periodic GC because of the
> deadlock problem.

Yes, but it's not a leak fix if we don't run a GC.

(In reply to comment #3)
> Review of attachment 240404 [details] [review]:
> 
> Well, if the background object is GCed, so are the settings, because nothing
> else keeps them alive.

The background object is not going to get GC'd as long as there is a closure referencing it, right?  The main impetus for the fix, though, is the settings object is staying alive and so it's toggle reference is toggling at an inopportune moment (from the wrong thread) causing the javascript context to write all over itself and deadlocks to happen. See bug 670200

Closures installed for signals don't keep alive the object on which they're connected (we trace from the object to the closures instead of rooting).
And I don't see how disconnecting from the signal would fix the problem you mention.

(In reply to comment #9)
> Closures installed for signals don't keep alive the object on which they're
> connected (we trace from the object to the closures instead of rooting).
So, I think what you think i'm saying is:

this._settings.connect('changed', function() {...}) keeps this._settings alive until the signal is disconnected.  I'm not saying that. I'm saying

this._settings.connect('changed', function() {...}) keeps the function(){...} object alive until the the signal is disconnected. Also, the function(){...} object keeps all of the objects it references alive until it dies. One of the objects it references in this particular case is the background object.  So i'm saying the background object won't be garbage collected until the signal is disconnected.  Since the background object won't be garbage collected until the signal is disconnected, this._settings won't be mopped up either. As I understand things anyway.

Concretely, without the patch, the shell locks up when the garbage collector kicks in while changing the background with this (abridged) stack trace:

+ Trace 231717

Connecting a signal keeps the function(){...} object reachable until disconnected *if* the this._settings is also reachable.
If nothing else keeps this._settings reachable (and I see no code that it would), you have a cycle that goes this._settings -> function(){...} -> this (the Background object) -> this._settings, with no inbound edges and thus ready to be garbage collected.
The crash you mention has nothing to do with the connected signal (although disconnecting probably makes it less likely)

Oh, I think I see what you're saying now.

When the GC runs, it's going to start from the rooted objects and descend through all referenced objects marking them as live. If the only thing referencing the background object is the signal handler, the background object will never get marked live because the signal handler is only referenced by the settings object which is only referenced by the background object; there's no path back to a rooted object.

Comment on attachment 240404 [details] [review]
background: disconnect settings object when destroyed

let's just fix the gc problem and avoid this.

Attachment 240405 [details] pushed as 3c1f389 - backgroundMenu: destroy menu when actor is destroyed