Created attachment 256261 [details] [review]
background: Disconnect settings signal handler on destroy

We connect to the changed signal in _init() but never actually disconnect from
it. The callback has a reference to "this" which results into the background
object not getting garbage collected.

Fix that leaks by disconnecting in _destroy()

Review of attachment 256261 [details] [review]:

::: js/ui/background.js
@@ +322,3 @@
+        this._settingsChangedSignalId = 0;
+
+        if (this._settings)

Why the if (this._settings) ? We didn't do that in the old case.

(In reply to comment #2)
> Review of attachment 256261 [details] [review]:
> 
> ::: js/ui/background.js
> @@ +322,3 @@
> +        this._settingsChangedSignalId = 0;
> +
> +        if (this._settings)
> 
> Why the if (this._settings) ? We didn't do that in the old case.

Because of:

        params = Params.parse(params, { monitorIndex: 0,
                                        layoutManager: Main.layoutManager,
                                        effects: Meta.BackgroundEffects.NONE,
                                        settings: null });

Notice the settings: null  ... (Maybe I should split that out).

Created attachment 256266 [details] [review]
background: Disconnect settings signal handler on destroy

We connect to the changed signal in _init() but never actually disconnect from
it. The callback has a reference to "this" which results into the background
object not getting garbage collected.

Fix that leaks by disconnecting in _destroy()

Created attachment 256267 [details] [review]
background: Check if settings is non null before using it

Settings can be null if not passed to _init so don't unconditionally connect
to its changed signal.

Review of attachment 256267 [details] [review]:

No. Even with the check, we'll just crash below when we try to get the actual settings in _load. We expect an actual settings instance, we just never construct it in the fallback because it would be wasteful.

Review of attachment 256266 [details] [review]:

::: js/ui/background.js
@@ +320,3 @@
         this.isLoaded = false;
 
+        this._settingsChangedSignalId = 0;

No need for this line.

(In reply to comment #6)
> Review of attachment 256267 [details] [review]:
> 
> No. Even with the check, we'll just crash below when we try to get the actual
> settings in _load. We expect an actual settings instance, we just never
> construct it in the fallback because it would be wasteful.

Oh ok indeed.

Pushed with the useless line removed.

Attachment 256266 [details] pushed as 8b0e846 - background: Disconnect settings signal handler on destroy

(In reply to comment #1)
> Created an attachment (id=256261) [details] [review]
> background: Disconnect settings signal handler on destroy
> 
> We connect to the changed signal in _init() but never actually disconnect from
> it. The callback has a reference to "this" which results into the background
> object not getting garbage collected.
> 
> Fix that leaks by disconnecting in _destroy()

Ray did the same patch at some point, and I blocked back then, because this analysis is wrong.
Signal callbacks are not rooted, so they can't keep objects alive, unless there is another C reference.

(In reply to comment #10)
> Ray did the same patch at some point, and I blocked back then, because this
> analysis is wrong.
> Signal callbacks are not rooted, so they can't keep objects alive, unless there
> is another C reference.

We still should not be emitting signals long after our actor is dead though. It's only polite.

<drago01> gcampax: ok and re https://bugzilla.gnome.org/show_bug.cgi?id=709263 ... that just means that the commit message is wrong right?
<Services> Bug 709263: normal, Normal, ---, gnome-shell-maint, REOPENED, background: Disconnect settings signal handler on destroy
<drago01> gcampax: or are you asking for a revert?
<drago01> gcampax: if yes why?
<gcampax> drago01, I'm just saying that it doesn't fix a leak
<gcampax> yes, disconnecting the handler is probably good for what Jasper said
<drago01> gcampax: ok, wasn't sure because you reopened the bug
<gcampax> nah, I just reopened to make sure the comment was seen


We can't really change the commit message (ok we could but rewriting history is a no go) ... so nothing really left here.