Closed Bug 380873 Opened 14 years ago Closed 14 years ago
Increase in leak stats since landing of new password manager
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.
Something like attachment 265014 [details] [diff] [review] from bug 378618 should probably be applied to nsLoginManagerPrompter.js.
Just for the record, Rlk went up from 4.88KB to 5.17KB due to that patch.
Assignee: nobody → dolske
Status: NEW → ASSIGNED
Attachment #265303 - Flags: review?(gavin.sharp)
Attachment #265303 - Flags: review?(gavin.sharp) → review+
Flags: blocking-firefox3? → blocking-firefox3+
Flags: blocking-firefox3+ → blocking-firefox3?
Flags: blocking-firefox3? → blocking-firefox3+
Whiteboard: [checkin-needed] → [checkin needed]
Whiteboard: [checkin needed]
This patch didn't seem to have any effect on Rlk.
mconnor noted that that this was a likely source of leaks.
Attachment #268308 - Flags: review?(mconnor)
Comment on attachment 268308 [details] [diff] [review] Use weak references with addObserver() you need to implement nsISupportsWeakReference for this to work, no?
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.
Yeah, it uses XPConnect magic: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp&rev=1.55#282
Would it be worth trying a patch for login manager that uses the same logic as the last one in bug 378618?
Comment on attachment 268308 [details] [diff] [review] Use weak references with addObserver() ok, r=me, yay for magic
Attachment #268308 - Flags: review?(mconnor) → review+
did this patch land? did it help enough to get back to even?
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.
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.
The new observer registration added in bug 385839 should be changed to weak too (or removed at shutdown).
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 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. :-)
Attachment #265303 - Attachment is obsolete: true
Attachment #265303 - Attachment description: Remove singleton from createInstance() → Remove singleton from createInstance() [checked in with no visible effect]
moving out, needs to make b1
Target Milestone: Firefox 3 alpha6 → Firefox 3 beta1
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.
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.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.