131360 - nsLDAPOperation makes it easy to leak nsILDAPMsgListener object

Status: NEW

(Directory :: LDAP XPCOM SDK, defect)

Description

[Rajiv Dayal]

Most implementations of nsILDAPMsgListener use an nsILDAPOperation object and at
several places, to do so the implementation defines a member variable of type
nsCOMPtr<nsILDAPOperation>, this results into cyclic reference since the
nsLDAPOperation implementation keeps an nsILDAPOperation as a member variable.
This would also mean changing the current implementations.

The solution for this is that the nsLDAPOperation should keep a weak reference
of the nsILDAPMsgListener object. This means the implementation of
nsILDAPMsgListener should implement the nsISupportsWeakReference which can be
done if the implementor derives from nsSupportsWeakReference class. To enforce
this on the implementors the nsILDAPMsgListener should be derived from
nsISupportsWeakReference.

Another solution to this is that the nsILDAPOperation instead of keeping a
member variable as nsCOMPtr<nsILDAPMsgListener> should keep the reference as
nsILDAPMsgListener *. When nsLDAPOperation::Init is called it should AddRef the
reference and then specifically call Release for the reference when the Search
Operation is complete, either on error or on Status completion.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Well, the operation itself doesn't leak the object, but it does make it easy for
the listener implementor to leak the object if they don't know how the operation
and connection code work together.  I still haven't decided what I think about
the proposed fix.  Another possible way to address this would be to just better
document what's going on with the ownership model under the hood.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Assigning bugs that I'm not actively working on back to nobody; use SearchForThis as a search term if you want to delete all related bugmail at once.

Comment 3

[Serge Gautherie (:sgautherie)]

Filter on "Nobody_NScomTLD_20080620"