Closed Bug 380873 Opened 14 years ago Closed 14 years ago

Increase in leak stats since landing of new password manager

Categories

(Toolkit :: Password Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha7

People

(Reporter: RyanVM, Assigned: Dolske)

References

Details

(Keywords: memory-leak, regression)

Attachments

(1 file, 1 obsolete file)

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.
Flags: blocking-firefox3?
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?
Whiteboard: [checkin-needed]
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: --- → Firefox 3 alpha6
Whiteboard: [checkin-needed] → [checkin needed]
mozilla/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js 	1.2
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.
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?
Whiteboard: [checkin needed]
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.
Whiteboard: [checkin needed]
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]
Depends on: 385839
Looks like bug 385082 fixed certain leaks here.
Depends on: 385082
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
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.