Created attachment 288159 [details] [review]
AuthPrompt: properly remove user widgets

When replacing a user widget, we need to destroy the child, not just
unparent it, or it will leak.

Created attachment 288160 [details] [review]
gdm: disconnect signals

Many signal connections on global objects and on non-widgets were not
disconnected when the unlock screen was destroyed, causing leaks.

Created attachment 288161 [details] [review]
gdm: fix missing parentheses

Incorrect parentheses meant that if the ShellUserVerifier was destroyed before
the call to fprintManager.GetDefaultDeviceRemote(), the reply would result in
an error.

Created attachment 288162 [details] [review]
messageTray: add some missing signal disconnections

Disconnect some signal connections that weren't disconnected on actor destroy
(not the result of comprehensive review.)

Review of attachment 288159 [details] [review]:

so this confused me at first and owen and I chatted on irc about it. The issue here is the UserWidget class takes a couple of signal connections on a long lived object (ActUser object).  These signal connections don't get released until the UserWidget is destroyed.  The UserWidget isn't destroyed until widget actor is destroyed.  The actor can't be destroyed merely by being removed from the container because the user widget references the actor.

Review of attachment 288160 [details] [review]:

thanks for the effort on this. clearly this was a ton of work.

::: js/gdm/authPrompt.js
@@ +138,1 @@
         this._userVerifier.disconnectAll();

don't need this disconnectAll anymore right?

::: js/gdm/loginDialog.js
@@ +936,3 @@
         }
 
+        this._userManagerAddedId = this._userManager.connect('user-added',

userAddedId might be clearer (and likewise for user-removed)

::: js/gdm/util.js
@@ +144,3 @@
 
+        this._smartcardInsertedId = this._smartcardManager.connect('smartcard-inserted',
+                                                                 Lang.bind(this, this._checkForSmartcard));

whitespace issue

Review of attachment 288161 [details] [review]:

::: js/gdm/util.js
@@ +304,3 @@
                     this._haveFingerprintReader = true;
                     this._updateDefaultService();
+                }

ugh

Comment on attachment 288162 [details] [review]
messageTray: add some missing signal disconnections

++

Review of attachment 288161 [details] [review]:

(nit: these are braces, not parens)

(In reply to comment #6)
> Review of attachment 288160 [details] [review]:
> 
> thanks for the effort on this. clearly this was a ton of work.
> 
> ::: js/gdm/authPrompt.js
> @@ +138,1 @@
>          this._userVerifier.disconnectAll();
> 
> don't need this disconnectAll anymore right?

Hmmm - so what you are saying is that if we properly disconnect from all global objects, remove timeouts, handle cancellation for all asynchronous calls, then we might still be connected to signals, but they will never be emitted, and we can just let the JS GC clean everything up. I agree, and checking, it does look like everything gets cleaned up on the ShellUserVerifier side - that was the point of my patch.

So it comes down to what is good style:

 Pro for disconnecting: if there are leaks, reduces the scope of the leak. Putting the disconnectAll() inside the destroy() method would be consistent with object.destroy() and widget.destroy() [though it's unobservable whether a signal is removed or just will never be emitted again]

 Con for disconnecting: disconnecting the signals on the JS side was the wrong way to handle left-over connections and turned noticeable backtraces to hard-to-detect leaks.

Pushed all patches (changing "parentheses to braces" in the commit message :-) ... 
I went ahead and removed the disconnectedAll (and retested for memory leaks), since my
reasoning that the disconnectAll papered over the problems that were leading to the
memory leaks was convincing to me.

Attachment 288159 [details] pushed as d8d046f - AuthPrompt: properly remove user widgets
Attachment 288160 [details] pushed as 8d3ff56 - gdm: disconnect signals
Attachment 288162 [details] pushed as ffdb85e - messageTray: add some missing signal disconnections

I fixed a small regression from this in e1e08f0a684281ba4df35d9ce5bc92f0510fd188