380873 - Increase in leak stats since landing of new password manager

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

From gavin's remarks in bug 374723 comment #24:

This introduced some leaks on fxdebug-linux-tbox:

--NEW-LEAKS-----------------------------------leaks------leaks%
nsPrefBranch                                    120     100.00%
nsVoidArray                                       8     100.00%
XPCWrappedNative                               1512      12.50%
XPCWrappedNativeProto                           672       9.09%
nsStringBuffer                                  104       8.33%
TOTAL                                          2416

If you compare with bug 378618 comment 2, I think this means that the
XPCWrappedNative[Proto] and nsStringBuffer leaks are just the fault of the JS
component loader, while the nsPrefBranch/nsVoidArray leaks were caused by this
added code.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Something like attachment 265014 [details] [diff] [review] from bug 378618 should probably be applied to nsLoginManagerPrompter.js.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Just for the record, Rlk went up from 4.88KB to 5.17KB due to that patch.

Comment 3

[Justin Dolske [:Dolske]]

Attachment: Remove singleton from createInstance() [checked in with no visible effect]

Comment 4

[:Gavin Sharp [email: [e-mail]]]

mozilla/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js 	1.2

Comment 5

[:Gavin Sharp [email: [e-mail]]]

This patch didn't seem to have any effect on Rlk.

Comment 6

[Justin Dolske [:Dolske]]

Attachment: Use weak references with addObserver()

mconnor noted that that this was a likely source of leaks.

Comment 7

[Mike Connor [:mconnor]]

Comment on attachment 268308 [details] [diff] [review]
Use weak references with addObserver()

you need to implement nsISupportsWeakReference for this to work, no?

Comment 8

[Adam Guthrie]

I think as long as it can QI to nsISupportsWeakReference, it should work (which it does). See the patch in bug 340547, for an example.

Comment 9

[Adam Guthrie]

Yeah, it uses XPConnect magic: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp&rev=1.55#282

Comment 10

[Adam Guthrie]

Would it be worth trying a patch for login manager that uses the same logic as the last one in bug 378618?

Comment 11

[Mike Connor [:mconnor]]

Comment on attachment 268308 [details] [diff] [review]
Use weak references with addObserver()

ok, r=me, yay for magic

Comment 12

[Mike Connor [:mconnor]]

did this patch land? did it help enough to get back to even?

Comment 13

[Nickolay_Ponomarev]

No, it didn't land.

Wouldn't it be better to explicitly remove the observer and the progress listener on shutdown instead of relying on the weak references? My understanding is that explicit cleanup is better when we know the exact moment we need to break the cycle. IIRC, without the weak references, pref notification is slightly more efficient, too.

Comment 14

[Adam Guthrie]

Note that sayrer did some leak fixing of his own over in bug 385839. I guess we could still land this patch to see if it shows any further improvements.

Comment 15

[Nickolay_Ponomarev]

The new observer registration added in bug 385839 should be changed to weak too (or removed at shutdown).

Comment 16

[Nickolay_Ponomarev]

From bug 385839 comment 8:
> > My understanding is that you _do_ need to remove the observers 
> Mano says services don't need to that (they used to). I also tried only
> removing the observers, without nulling out the members. This got some of the
> leaks, but not all.

Mano, would you chime in and explain why this is no longer required? We should then update the relevant documentation. And the patch shouldn't be checked in, in that case.

Comment 17

[Justin Dolske [:Dolske]]

Comment on attachment 265303 [details] [diff] [review]
Remove singleton from createInstance() [checked in with no visible effect]

(marking obsolete based on comment #5 -- no effect on leaks seen)

Also, bug 385524 will replace this code... Once the replacement stops leaking. :-)

Comment 18

[Nickolay_Ponomarev]

Looks like bug 385082 fixed certain leaks here.

Comment 19

[Mike Connor [:mconnor]]

moving out, needs to make b1

Comment 20

[Myk Melez [:myk] [@mykmelez]]

Over in bug 385237, I figured out that the cause of a content pref service leak is the nsIFactory that creates the service.  Some testing leads me to conclude that the factories in every JS component are likely to be leaking, which may be the cause of (some of) the leaks reported in the bugs to which I am adding this comment (bug 337050, bug 378618, bug 381239, and bug 380873/bug 380931).

Take a look at bug 385237, comment 2 for more details, and note that the fix for bug 180380 makes all the XPCWrappedNative and XPCWrappedNativeProto leaks (which were what the content pref service was leaking) go away and may similarly fix (some of) the leaks reported in this bug.

Comment 21

[Justin Dolske [:Dolske]]

I think the leaks described here have now been fixed, by other various checkins (mainly the switch to XPCOMUtils) so I'm closing this.

Bug 380931 has an issue about a couple of other leaks that may or may not still exist, so I'll continue looking at leaking in general there.