669151 - LDAP connections are not cleaned up until shutdown

Status: RESOLVED FIXED

(MailNews Core :: LDAP Integration, defect)

Description

[Mark Banner (:standard8)]

Attachment: The fix

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.

Comment 1

[[e-mail]]

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...

Comment 2

[Mark Banner (:standard8)]

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 3

[David :Bienvenu]

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.

Comment 4

[Mark Banner (:standard8)]

Well unfortunately we don't have xpcshell-tests covering LDAP connections at the moment anyway.

Comment 5

[Ludovic Hirlimann [:Usul]]

(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 ?

Comment 6

[Mark Banner (:standard8)]

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 ;-)

Comment 7

[Mark Banner (:standard8)]

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.

Comment 8

[Mark Banner (:standard8)]

Checked into aurora: http://hg.mozilla.org/releases/comm-aurora/rev/0d7f09a3495b

Comment 9

[[e-mail]]

(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.