Closed Bug 669151 Opened 9 years ago Closed 9 years ago

LDAP connections are not cleaned up until shutdown

Categories

(MailNews Core :: LDAP Integration, defect)

defect
Not set

Tracking

(thunderbird6 fixed)

RESOLVED FIXED
Thunderbird 7.0
Tracking Status
thunderbird6 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Keywords: regression)

Attachments

(1 file)

Attached patch The fixSplinter Review
In reviewing another patch I've just spotted this.

If you do any sort of LDAP query via autocomplete or the address book quick search, then the connection stays alive until shutdown. This is per-compose window or per a quick search.

It is a regression from bug 343332 as that one registered nsLDAPConnection with the observer service as a strong reference, but that reference doesn't get removed until shutdown, hence connections are stuck open, even though we don't re-use them at the moment.

As nsLDAPConnection already implements weak references, then we can just change the observer service hold of the connection to a weak reference, and add a remove observal call to tidy up. This stops us keeping nsLDAPConnection in memory, so it gets freed at an appropriate time, e.g. a garbage collection.
Attachment #543750 - Flags: review?(dbienvenu)
Comment on attachment 543750 [details] [diff] [review]
The fix

I'm not sure that you're allowed to pass yourself to RemoveObserver in your own destructor...
Why not? How else am I meant to say, I'm going away now so there's no point in keeping a reference to me...

The other obvious place would be the Close() function, but that still gets called anyway.

Also see: http://mxr.mozilla.org/comm-central/source/mozilla/modules/libpref/src/nsPrefBranch.cpp#118
Comment on attachment 543750 [details] [diff] [review]
The fix

I can't run xpcshell tests at the moment, but it's hard to see how this could affect any of our tests. Testing by hand seems to work fine; the connections are destroyed at gc time.
Attachment #543750 - Flags: review?(dbienvenu) → review+
Well unfortunately we don't have xpcshell-tests covering LDAP connections at the moment anyway.
(In reply to comment #4)
> Well unfortunately we don't have xpcshell-tests covering LDAP connections at
> the moment anyway.
Do we have a fake LDAP server ?
Checked in: http://hg.mozilla.org/comm-central/rev/79ea291ad0c0


No we don't have a fake server, otherwise we would have something covering LDAP connections ;-)
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 7.0
Comment on attachment 543750 [details] [diff] [review]
The fix

Also want this for TB 6 as it is a regression in TB 5 that we should fix asap.
Attachment #543750 - Flags: approval-comm-aurora+
(In reply to comment #2)
> Why not? How else am I meant to say, I'm going away now so there's no point
> in keeping a reference to me...
But you've *already* gone by the time your destructor runs...

> Also see:
> http://mxr.mozilla.org/comm-central/source/mozilla/modules/libpref/src/
> nsPrefBranch.cpp#118
Interesting... I'll ask about that.
Blocks: 684492
You need to log in before you can comment on or make changes to this bug.