We need to implement the LDAP Service manage objectclass. It will be used to "pool" LDAP connections, making sure they are properly connected, and when appropriate, disconnected. It depends heavily on nsLDAPServer objects for getting LDAP server specific information (like host, port etc.).

Taking this bug...

some comments: mTimestamp(0) - don't think this will compile on the Mac - you might need to use those LL_* macros. There doesn't seem to be much ref-counting going on, at least in your get routines. Should they be add-refing? You don't need to check for null before deleting: if (mServers) { delete mServers; } could just be delete mServers. I'm sure Brendan must have a few comments :-)

David says: >mTimestamp(0) - don't think this will compile on the Mac - you might need to >use >those LL_* macros. Ok, I'll talk to some local Mac guru about it. I've used similar initializers in other modules, which seemed to work on Mac. >There doesn't seem to be much ref-counting going on, at least in your get >routines. Should they be add-refing? Do I need to add-ref in the "internal" GetServer, GetMessage and GetConnection (part of the nsLDAPServiceEntry class) methods? The "public" methods nsLDAPService::GetServer, AddServer and GetConnection does add-ref properly. >You don't need to check for null before deleting: > if (mServers) { > delete mServers; > } Ok. Thanks! -- Leif

you should definitely try to build on the Mac re the PRTIme stuff. Better safe than sorry. I use something like LL_I2L(m_startTime, 0); I hadn't heard of LL_zero() but that looks good. Re the ref-count stuff, you can do what you want internally but I wanted to make sure you know what's going on with the ref-counting and ownership. It looks to me (though I could be wrong) that the ldap service does not own the servers and connections (i.e., doesn't hold a reference), so the client of the service has to make sure they remove the servers and connections from the service before deleting them or else the service will have dangling pointers. That's OK, as long as it's understood.

nsILDAPService.idl ------------------ * In 2 places, NS_ERROR_NOT_AVAILABLE should really be documented as "couldn't create connection thread", since it's coming from nsILDAPConnection.idl, and that's how it's documented there. nsLDAPService.cpp ----------------- * DeleteServer(): please file a bug on the renaming deleted entries thing, since this will probably be an issue once we start observing the preferences. * lines 340, 488, 551, 640, 759, 761: why are any casts at all necessary on these lines? * OnLDAPMessage(): according to the API, null messages are allowed, though their exact semantics are not yet defined. I suspect we may use this for timeouts or errors. At least mark this with an XXX so we come back and check for this once we define null message semantics. * re Bienvenu's comment on the service owning connections and servers: I believe the service does correctly own servers, the AddRef/Release code for this is in nsLDAPService::AddServer/DeleteServer, but it looks like it doesn't own the connection, which looks to me like it's probably unintentional. I suspect the code would be easier to read and understand if the AddRef and Release logic were pushed down into the nsLDAPServiceEntry XPCOM getters and setters themselves. Other those minor things, it looks good!

David says: > you should definitely try to build on the Mac re the PRTIme stuff. Better safe > than sorry. I use something like LL_I2L(m_startTime, 0); I hadn't heard of > LL_zero() but that looks good. Yep, I've fixed that (thanks!) and it builds and run on Mac now as well as Win32 and Linux. (I'm still waiting to get a Mac build systems, so I have to bug others to help me... :( I have a new patch, fixes a bug in nsLDAPConnection which caused havoc in the service, as well as this Mac stuff. I'll post it here asap. > Re the ref-count stuff, you can do what you want internally but I wanted to make > sure you know what's going on with the ref-counting and ownership. It looks to > me (though I could be wrong) that the ldap service does not own the servers and > connections (i.e., doesn't hold a reference), so the client of the service has The Service creates the connection objects, and owns message objects, and it holds a reference to the "server" object. The connection object and message object are stored in internal nsHashArrays, storing nsCOMPtr's. > to make sure they remove the servers and connections from the service before > deleting them or else the service will have dangling pointers. That's OK, as Yes, good point. However, I do want the service to keep a reference to the nsLDAPServer objects (and I add-ref that), even if the user deletes it. The nsLDAPService is a caching mechanism for servers, connections and the the bind message. Thanks again!

Ok, here's the status right now: 1. I've changed the stuff related to long-long's and Mac. It builds and run on Mac now. 2. I've fixed the other changes request by dmose. 3. I've fixed a bug in nsLDAPConnection, which although not related to the service directly, it caused some serious problems while testing my code. I hope it's ok to include it in this patch, since it's so minor (we need to initialize a couple of variables in the constructor). So, a new patch, and new nsLDAPService.cpp and nsILDAPService.idl are attached, as well as a new patch. nsLDAPService.h has not changed since the "4th" version.

Arbitrary/nonsensical control flow alert: if (!aServer) { return PR_FALSE; } else { mServer = aServer; } return PR_TRUE; The else-after-return is a non-sequitur, and the final return and its only predecessor statement, mServer = aServer;, should be indented the same (one 4-space unit). This glitch recurs earlier, with if (!mLeases) as the condition, and again earlier: now = PR_Now(); if (LL_IS_ZERO(now)) { return PR_FALSE; } else { mTimestamp = now; } But PR_Now will never return 0 unless your system's clock is misconfigured to the beginning of the epoch. Why should this method return false in only that case? Why not just set mTimestamp and let the chips fall where they may? There's code in the nsLDAPConnection.cpp patch that mixes four- and two-space indentation (nit, I know; also, beware diff -u adds a leading space on these lines): if (mPendingOperations) { delete mPendingOperations; } More else-after-return nonsense: if (mServers->Exists(&hashKey)) { return NS_ERROR_FAILURE; } else { and a leak on Init error right after that: // Create the new server entry, and add it into the hash table // entry = new nsLDAPServiceEntry; if (!entry || !entry->Init()) { NS_ERROR("nsLDAPService::AddServer: out of memory "); return NS_ERROR_OUT_OF_MEMORY; } else-after-return yet again: if (!entry->SetServer(aServer)) { return NS_ERROR_FAILURE; } else { NS_ADDREF(aServer); mServers->Put(&hashKey, entry); } from DeleteServer, more capricious control flow and indentation: entry = NS_STATIC_CAST(nsLDAPServiceEntry *, mServers->Get(&hashKey)); if (entry) { if (entry->GetLeases() > 0) { return NS_ERROR_FAILURE; } else { entry->DeleteEntry(); } } else { // There is no Server entry for this key // return NS_ERROR_FAILURE; } return NS_OK; There's no reason to put the failure early return in the outer else clause. Invert the sense of the if condition and return failure in the then part, unindenting the else part (which should not include else-after-return either, before entry->DeleteEntry() is called). The null _retval checks with NS_ERROR and error return backstop code seem like overkill -- who might call you with such violations of the rules of XPCOM? If developers, make 'em eat assertions. More tomorrow. /be

Brendan wrote: > Arbitrary/nonsensical control flow alert: > > if (!aServer) { > return PR_FALSE; > } else { > mServer = aServer; > } Ok, I've fixed all of those. The code looks better now, thanks. > But PR_Now will never return 0 unless your system's clock is misconfigured to >the beginning of the epoch. Why should this method return false in only that >case? Why not just set mTimestamp and let the chips fall where they may? Ok, changed. > There's code in the nsLDAPConnection.cpp patch that mixes four- and two-space > indentation (nit, I know; also, beware diff -u adds a leading space on these > lines): Dunno why that was, I didn't do it. But I've changed it. > and a leak on Init error right after that: Ack, good catch, fixed. > The null _retval checks with NS_ERROR and error return backstop code seem like > overkill -- who might call you with such violations of the rules of XPCOM? If > developers, make 'em eat assertions. Ok, I don't know what to do here... I don't really care either way, but some people want to use this error checks, several (including shaver) do not. I don't have a strong opinion on this matter, particularly since I'm a total XPCOM newbie. But, I'd like our code to be consistent at least, and that's what I've tried to do. I'm leaving this as-is for now, until someone tells me that not checking _retval for NULL is the way to go throughout the entire XPCOM code. I'm attaching a new version on nsLDAPService.cpp. I've changed the prototype for SetTimestamp() to be "void" as well, changing it in the .h file as well (of course). Thanks a lot for the reviews, much appreciated!

Do you really need to lock in nsLDAPService::Init()? Can it be called on multiple threads, or race with other manipulations? I would think not (a bad race will leak mLock or crash autolocking on a null mLock, by my reading. Probably better to just require that Init be called before any off-thread accesses are possible, which will almost always be the case anyway: calling code with Initialize immediately after construction, before passing it to anything that could race (especially since Init isn't part of the XPCOM interface). PR_Lock(mLock); entry = NS_STATIC_CAST(nsLDAPServiceEntry *, mServers->Get(&hashKey)); if (!entry) { PR_Unlock(mLock); The PR_Lock/PR_Unlock logic in RequestConnection is pretty complex: would an nsAutoLock, as used elsewhere, not make that simpler? Alternatively, it appears that you only need to hold mLock when you're accessing the hash tables (rename it to mTableLock for clarity?), so it would seem that you could unlock unconditionally after mServers->Get (manually or via a small scope for an nsAutoLock) and be done with it. In ReconnectConnection, you acquire mLock very early on, and don't release it until you return, but, again, it only seems to be necessary to protect the hash table access. Better to keep the critical section minimal: { nsAutoLock lock(mLock); entry = NS_STATIC_CAST(..., mServers->Get(&hashKey)); } if (!entry) ... (And especially to avoid calling other methods with the lock held. It's easy to end up deadlocking if you're not quite careful, which is why ``lock only in leaf functions'' is a good rule of thumb.) rv = EstablishConnection(entry); entry->SetRebinding(PR_TRUE); if (NS_FAILED(rv)) { entry->SetRebinding(PR_FALSE); return rv; } You SetRebinding twice in the error case. Better to move the PR_TRUE call to after the error check, or even after the PushListener stanza, if PushListener doesn't depend on the effects of SetRebinding(PR_TRUE). Hmmm. I'm actually starting to suspect that you're using mLock to serialize all accesses to nsLDAPServiceEntries, because of the lengths you go to OnLDAPMessage. If this is the case, _please_ add documentation to that effect in the headers and implementation of nsLDAPServiceEntry. Also, do you need to recheck any entry state after the listener's OnLDAPMessage returns, in case it mutates it on you? (If it's not the case, then the unlocking logic in OnLDAPMessage could be a lot simpler.) conn = do_CreateInstance("@mozilla.org/network/ldap-connection;1", &rv); Is the decoupling implied by this code in EstablishConnection really the case? That is, could you really interoperate with another implementation of ldap-operation;1? If not, you're probably best off using the CID or even just a direct construction of the class. Do you really need to list NS_ERROR_OUT_OF_MEMORY and NS_ERROR_NULL_POINTER in all those method comments in nsILDAPService.idl? Also, you should start all your sentences with capitals, and tidy the error-code detail indentation for requestConnection. For the time being, it is vital that you call this function * when you're done with a Connection, and that you do not keep any * copies of the Connection object Do you really mean ``copies of the Connection object'' (does it provide a |clone| method?) or something more like ``any dangling pointers to the provided Connection object''? (I'm a little nervous about this, because of the problems with people having pointers to other interfaces of the same object, etc., but dmose has explained the ownership requirements here, and I don't have a better solution to offer at the moment.) Is it worth putting a (DEBUG-only?) flag in the Connection objects to track when they're in the post-releaseConnection invalid state, and assert if they're manipulated (possibly including addref and release) in that state? Sorry for the delay in reviewing this: I got behind on Friday and didn't recover until now, because of some critical time management errors. I hope you can still get this stuff done in time.

Shaver wrote: >Do you really need to lock in nsLDAPService::Init()? Can it be called on >multiple threads, or race with other manipulations? I would think not (a bad No, removed. > PR_Lock(mLock); > entry = NS_STATIC_CAST(nsLDAPServiceEntry *, mServers->Get(&hashKey)); > if (!entry) { > PR_Unlock(mLock); I don't think this is enough. My concern is if someone calls in to RequestConnection() or ReconnectConnection() concurrently. We could then potentially ending up with two connections to the LDAP server. The idea with the service is that it's a caching mechanism, to reuse the same LDAP connection multiple times. I've added a comment to the IDL file regarding this. I might be overly careful in the locking, I'll look into this and see what I can relax. I'm just scared that the internal state of a cached connection gets messed up. >The PR_Lock/PR_Unlock logic in RequestConnection is pretty complex: would an >nsAutoLock, as used elsewhere, not make that simpler? Alternatively, it >appears >that you only need to hold mLock when you're accessing the hash tables (rename >it to mTableLock for clarity?), so it would seem that you could unlock >unconditionally after mServers->Get (manually or via a small scope for an >nsAutoLock) and be done with it. Again, I don't think that's enough, but I'll look into it. >You SetRebinding twice in the error case. Better to move the PR_TRUE call to >after the error check, or even after the PushListener stanza, if PushListener >doesn't depend on the effects of SetRebinding(PR_TRUE). Bug, fixed. :) >Hmmm. I'm actually starting to suspect that you're using mLock to serialize >all >accesses to nsLDAPServiceEntries, because of the lengths you go to >OnLDAPMessage. If this is the case, _please_ add documentation to that effect Ok, added this to the IDL file: * Much of the code here is serialized, to assure that we don't * end up with multiple instances of the connection to a server. * The main goal of this service is to cache connections to an LDAP * server, and it's important that we actually reuse connections. * This is not a serious bottleneck, since the calls into the service * are done fairly infrequently. Also added this to the .cpp file. >in the headers and implementation of nsLDAPServiceEntry. Also, do you need to >recheck any entry state after the listener's OnLDAPMessage returns, in case it >mutates it on you? (If it's not the case, then the unlocking logic in >OnLDAPMessage could be a lot simpler.) When calling an OnLDAPMessage() listener, it's quite likely that the listener will call back into the service. That's why the locking here got complicated. I've tested this a lot, and it was needed to avoid deadlock issues. I might be on drugs here, I can't say I'm an expert on concurrency issues, so if you have some specific suggestions how to make this simpler, please! :) > conn = do_CreateInstance("@mozilla.org/network/ldap-connection;1", > &rv); > >Is the decoupling implied by this code in EstablishConnection really the case? >That is, could you really interoperate with another implementation of >ldap-operation;1? If not, you're probably best off using the CID or even just a >direct construction of the class. Don't know the answer to this one. Dmose, any suggestions? >Do you really need to list NS_ERROR_OUT_OF_MEMORY and NS_ERROR_NULL_POINTER in >all those method comments in nsILDAPService.idl? Also, you should start all >your sentences with capitals, and tidy the error-code detail indentation for >requestConnection. This was done to be consistent with other LDAP XPCOM code. I don't have a strong opinion either way, as long as we are consistent. >Do you really mean ``copies of the Connection object'' (does it provide a >|clone| method?) or something more like ``any dangling pointers to the provided >Connection object''? (I'm a little nervous about this, because of the problems Dangling points. Fixed! >with people having pointers to other interfaces of the same object, etc., but >dmose has explained the ownership requirements here, and I don't have a better >solution to offer at the moment.) It's a known issue, and we will try to find a better solution, making ReleaseConnection() obsolete. >Is it worth putting a (DEBUG-only?) flag in the Connection objects to track >when >they're in the post-releaseConnection invalid state, and assert if they're >manipulated (possibly including addref and release) in that state? Dunno, maybe. I didn't want to spend an horrendous amount of time on this issue, since they goal is to get rid of the entire ReleaseConnection() jive. I'll look into it though. Thanks a lot for the review! I'll post an updated version of the files as soon as I can. If you can look at the new files when they are available, that'd be great. Btw, did you notice any missing add-reffing as indicated by David? It's one of my main concerns, particularly since I'm such a newbie to all this XPCOM stuff.

Regarding the use of contract-id here, I think it's ok, since, unlike most of the rest of the XPCOM SDK, the service doesn't actually know any thing about the guts of the other classes nor does it call into the LDAP C SDK directly. brendan/shaver: the NULL-checking at interface boundaries and documenting of the same is done throughout the XPCOM SDK code, not just here. If you really think this is important enough to get rid of, please open another bug against me and I'll write the full essay describing why I think it's actually a good thing.

Oh yeah, once shaver and brendan's concerns are satisfied, r=dmose@netscape.com.

Ok, posted three new files, addressing most of Shavers concerns. Please review again? Thanks, -- Leif

If I read your new comment regarding serialization, it still sounds like you only need to serialize hash table access, in order to make sure that two additions of the same server's entry don't race and end up with duplicates. What other operations need to be serialized to avoid duplicates? The refcounting looks fine to me: the connection cache keeps references to the cached objects, so you don't need to twiddle them for the internal methods in question. As far as the flag for use of released connections, I'm torn. On the one hand, I can see not wanting to burn the cycles on it, but on the other you're going to considerable lengths to defend against relatively easy-to-diagnose API errors like the passing of NULL, and yet you leave this case, which is harder for callers to diagnose and a much more subtle API requirement. I think the fact that the ReleaseConnection stuff is supposed to go away (is there a bug on that?) tips the balance in favour of just letting it slide, but I'm getting more and more tempted to ask for dmose's treatise on NULL checking. The only thing I'm still unsure about is the serialization; I really want to understand the requirements before I stamp here, and it still looks to me like just serializing the hash table accesses will be sufficient. (And I really prefer to keep locking to minimal sections: see 76661 for an example of where we've been burned by holding locks across too many calls.)

ok, let me look into this again then, see if I can relax the locks.

Shaver wrote: >If I read your new comment regarding serialization, it still sounds like you >only need to serialize hash table access, in order to make sure that two >additions of the same server's entry don't race and end up with duplicates. > >What other operations need to be serialized to avoid duplicates? I've made some changes to locking, but not what you are asking for. The problem is this: I don't want the internal state of a cached connection to get messed up. Or even worse, have two LDAP connections established concurrently, and then have to drop one when "collision" is detected. The reason there was serialization in RequestConnection()/ReconnectConnection() was to avoid any such possiblity. I've spent all morning trying to figure out how I can remove the locks here, without causing race conditions. The best I can accomplish (with a lot of if() statements etc.) is a situation where it's still possible to create extra connections to the LDAP server, which are then dropped on the floor (delete) when the collision is detected. That just seems wrong to me (but I might be on crack of course). Another similarly poor alternative was to allow multiple LDAP connections to happen, but only cache one of them (and let the other die when it's consumer releases it). Granted, this is fairly unlikely to happen, but the code was written to not let it happen at all. Hence the serialization on the creation/reconnection of LDAP connection objects. >As far as the flag for use of released connections, I'm torn. On the one hand, >I can see not wanting to burn the cycles on it, but on the other you're going to >considerable lengths to defend against relatively easy-to-diagnose API errors Well, I'm torn too... :) However, one of the main reasons to keep those checks you despise is to keep consistent with the rest of LDAP XPCOM. >callers to diagnose and a much more subtle API requirement. I think the fact >that the ReleaseConnection stuff is supposed to go away (is there a bug on >that?) tips the balance in favour of just letting it slide, but I'm getting Yep, there is a bug on that, bug 75954. It's also documented in the IDL, marked to become depracated in the future (I'd make it a noop). I need to talk to you about the serialization, I can't figure out how to do without it. :( And, I'm looking forward to see you and dmose fight out the battle of NULL pointer checks on the parameters in the API, it will be a most excellent battle. Thanks. -- leif

Are all operations on connections meant to be serialized, then? In other words, is it possible for two threads to be playing with the same connection at once, even after they've serially retrieved it from the LDAPService? If so, you need to lock on the |entry|, not just the service. I chatted with dmose last night -- bugger called me at _home_! -- and read the code some more, and I think my biggest concern can be met with this change: - Make the LDAPService more tightly coupled to the connection, either by use of a CID or just |new nsLDAPConnection|. This will assuage my fears about an alternate implementation trying to call back into the LDAP service during initialization and deadlocking[*]. With that change, sr=shaver. You don't have to push the locking down into the entry for this checkin but please, I beg of you, write something in directory/xpcom/doc/threading.html that talks about what operations are serialized against what others, etc. [*] Maybe we need nsReentrantLock? Maybe a reader-writer lock is a useful tool here? I'll ponder it. Now, back to resting.

>Are all operations on connections meant to be serialized, then? In other >words, >is it possible for two threads to be playing with the same connection at once, >even after they've serially retrieved it from the LDAPService? If so, you need >to lock on the |entry|, not just the service. This I don't understand... The LDAP Service caches and manages nsLDAPConnection and nsLDAPMessage objects. An nsLDAPConnection can be shared by multiple threads, which is one reason why I go through this serialization process (to make sure we don't step on each other). >I chatted with dmose last night -- bugger called me at _home_! -- and read the >code some more, and I think my biggest concern can be met with this change: Sorry, dmose is a buttmunch. :-p > - Make the LDAPService more tightly coupled to the connection, either by use > of a CID or just |new nsLDAPConnection|. This will assuage my fears about > an alternate implementation trying to call back into the LDAP service >during > initialization and deadlocking[*]. I did this after you first mentioned it - see the attachment from 04/30/01. From then on, I use static NS_DEFINE_CID(kLDAPConnectionCID, NS_LDAPCONNECTION_CID); and then conn = do_CreateInstance(kLDAPConnectionCID, &rv); Is this not what you had in mind? >With that change, sr=shaver. You don't have to push the locking down into the >entry for this checkin but please, I beg of you, write something in >directory/xpcom/doc/threading.html that talks about what operations are >serialized against what others, etc. Ok, I'll do that. The serialization is primarily around RequestConnection() and ReconnectConnection(), which is where the risk of ending up with extra connections to the LDAP server is worse. The design is made where the "helper" function EstablishConnection() is atomic, and it's used by those two methods mentioned. If I push down the locking into the entry, the code would still work, but I'm pretty sure there can be cases then where we establish a new LDAP connection, which has to be dropped or used only once (which defeats the purpose of the LDAP service). >[*] Maybe we need nsReentrantLock? Maybe a reader-writer lock is a useful tool >here? I'll ponder it. >Now, back to resting. Hope you feel better soon, and thanks again for looking into this.

I've created bug 78758, Reducing the serialization in nsLDAPService. I believe shaver is right (*sigh*) and we can reduce a lot of the serialization in RequestConnection() and ReconnectConnection(). Still hoping I can check this version in, it meets the criteria for Shavers SR=, and it does reduce a number of locking issues compared to earlier versions.

All those getters and setters for nsLDAPServiceEntry data members should be inline, I say. What invariants is nsLDAPService::mLock supposed to protect? The mServers hash table including all of its entries' state, at least. But why hold it all the way across EstablishConnection? It's bad to hold a lock (which might use SMP competitive spinning for short waits) across a bunch of do_CreateInstance calls -- and what about conn->Init(...) and operation->Init() do, never mind operation->SimpleBind(0)? It would be better to release the lock and protect the lifetime of the entry in use, if not its state. Or look it up again after releasing the lock and establishing the connection, and if not found or found but of a new generation (not the same entry we were using), cope somehow. More in a bit. /be

> class nsLDAPService : public nsILDAPService, nsILDAPMessageListener Don't you need a public before the second class from which nsLDAPService inherits? I worry you'll get out-of-line copies of GetMessage, etc. by not doing the inlining directly in the nsLDAPServiceEntry class declaration. Worth getting right now? Your call, if testing shows my worry is real. The extra block in nsLDAPService::GetServer to scope the nsAutoLock is not necessary any longer, if it ever was. Nit, easy to fix but entirely up to you. Ditto GetConnection. Should nsLDAPService::AddServer fail on collision, or just return NS_OK? There is no out param or return value by which the caller can tell that it added the server, rather than finding one there (due to a race, if the caller checked; else perhaps already there for a while!). Why fail? In RequestConnection, please fuse the big common tail of the final if-else: { nsAutoLock lock(mLock); entry = NS_STATIC_CAST(nsLDAPServiceEntry *, mServers->Get(&hashKey)); if (!entry) { return NS_ERROR_FAILURE; } if (!entry->PushListener(NS_STATIC_CAST(nsILDAPMessageListener *, aListener))) { return NS_ERROR_FAILURE; } } This should be done once, just before the final return NS_OK. From IRC: you caught the stuck lock in OnLDAPMessage, so that's cool. I'll look for a new patch, then sr=. Do it, almost there! /be /be

For posterity: the lack of ref-counting of nsLDAPServiceEntry is scary. Right now, no one ever removes an entry from mServers, and the unlocked calls to EstablishConnection that pass entry pointers depend on that fact. Short of ref-counting the table, you could maybe arrange to rename rather than remove, as you said on IRC. But eventually, you have to remove -- no? (This is a question to be answered later, not an obstacle to the forthcoming patch going in.) /be

I meant to inline the entire bodies of those Get and Set methods, moving them from the .cpp to the .h file -- but that can happen later, and maybe we can also avoid the local variables that work around NS_IF_ADDREF difficulties that we're consulting scc over. sr=brendan@mozilla.org. /be

Two leaks, both of the form: if (entry->GetMessage()) { // We already have a message, lets keep that one. // return NS_ERROR_FAILURE; } That was ok when you were cheating and not addref'ing, but now.... Either add a boolean HasMessage predicate, or use an nsCOMPtr and getter_AddRefs (sorry the already_AddRefed idea didn't pan out -- cc'ing scc for followup). Otherwise looks good enough to check in, so sr=brendan@mozilla.org again, once you fix the leaks. I did raise some questions in comments earlier today, such as when entries should get removed, and how the code should protect them. So leave this bug open or (better if you transcribe enough context) file a sequel bug and list its number here. /be /be

The other followup to-do: really inline these getters (and maybe make them XPCOM style, if we can't have already_AddRefed; it's incongruous to see an XPCOM-style out param passed using getter_AddRefs for operation->GetConnection, while the entry->GetConnection uses the return value). /be

Hey scc, can you read the last few patches and tell us why already_AddRefed for return-value-style getters didn't compile on all platforms? /be

Sorry, there's only one such GetMessage leak. My mistake driving the cheesy find dialog. /be

I've created a new bug 78993 to track the inline and the issues with calling styles in the nsLDAPServiceEntry class. I also addded BE's comment re: deletion of entries to bug 77669.

Checked in.

reassign to Olga as QA contact

nsILDAPService interface is implemented marking as verified