The LDAP thread nsIRunnable object holds a weak reference to an nsILDAPConnection object. That weak reference is created on the ui thread, but released on the ldap thread, when the mRunnable object goes away. Allocated here: http://lxr.mozilla.org/mozilla/source/directory/xpcom/base/src/nsLDAPConnection.cpp#1021 Defined here: http://lxr.mozilla.org/mozilla/source/directory/xpcom/base/src/nsLDAPConnection.h#160 released here on the ldap thread: http://lxr.mozilla.org/mozilla/source/directory/xpcom/base/src/nsLDAPConnection.cpp#829 And finally the runnable object is destroyed here, from an ldap thread: NSGlue_Assertion(const char * 0x00367650, const char * 0x0036761c, const char * 0x003675e8, int 0x0000006e) line 107 nsWeakReference::Release(nsWeakReference * const 0x044ba9f8) line 110 + 98 bytes nsCOMPtr<nsIWeakReference>::~nsCOMPtr<nsIWeakReference>() line 584 nsLDAPConnectionLoop::~nsLDAPConnectionLoop() line 575 + 11 bytes nsLDAPConnectionLoop::`scalar deleting destructor'(unsigned int 0x00000001) + 15 bytes nsLDAPConnectionLoop::Release(nsLDAPConnectionLoop * const 0x06950898) line 577 + 147 bytes nsCOMPtr<nsIRunnable>::assign_assuming_AddRef(nsIRunnable * 0x00000000) line 569 nsCOMPtr<nsIRunnable>::assign_with_AddRef(nsISupports * 0x00000000) line 1225 nsCOMPtr<nsIRunnable>::operator=(nsIRunnable * 0x00000000) line 714 nsThread::Main(void * 0x0676ef80) line 135 _PR_NativeRunThread(void * 0x06f68538) line 436 + 13 bytes pr_root(void * 0x06f68538) line 112 + 13 bytes _threadstartex(void * 0x06f686c0) line 227 + 13 bytes

I originally wrote most of this a while ago, but I've now got around to looking at it again. The basic plan of this patch is to: - drop nsLDAPConnectionLoop, this is the loop that currently continuously runs until told to shutdown. - replace the loop with nsLDAPConnectionRunnable. These are objects that we create, one for each operation. - When nsLDAPConnection gets a pending operation, it will now add it to its list, create an nsLDAPConnectionRunnable, and dispatch it to the connection thread. - Once that Run is called on the Runnable, it will get the result, and process it accordingly, either notifying the callers and/or re-dispatching itself. Once the operation finishes, it doesn't re-dispatch itself and just dies (the thread code keeps a reference to the runnable whilst operations are going on). It still doesn't fix the one case of SSL shutdown error that I can repeat (do a search, quit immediately), I'm starting to think we should be doing an xpcom-shutdown observer to fix that fully.

I've not got time to work on this at the moment. I think the patch basically works, its just the SSL shutdown error that is the problem.

(In reply to comment #2) This sounds like it wouldn't miss much to be completed. Would you have time to do it (now)? Otherwise, would you have an example/instructions to do so? (and a testcase or an SSL LDAP url to reproduce.)

Not blocking on this at the moment - afaict the crash is rarely hit as the code currently stands. The thread-safety assertion also isn't a major issue. (In reply to comment #3) > (In reply to comment #2) > Otherwise, would you have an example/instructions to do so? > (and a testcase or an SSL LDAP url to reproduce.) The end of comment 0 has what needs doing - fix the patch so that we don't crash when the user does a search and quit against an ssl server. The solution would potentially be to hook up an xpcom-shutdown observer.

Comment on attachment 355986 [details] [diff] [review] WIP v1 For patchlove query cleanup purposes, patch has obsoleted: $ patch -p1 --dry-run < ~/Desktop/p343332.diff patching file directory/xpcom/base/src/nsLDAPConnection.cpp Hunk #1 succeeded at 69 (offset 1 line). Hunk #2 FAILED at 80. Hunk #3 FAILED at 93. Hunk #4 succeeded at 137 (offset 5 lines). Hunk #5 succeeded at 201 (offset 5 lines). Hunk #6 succeeded at 221 (offset 5 lines). Hunk #7 FAILED at 312. Hunk #8 FAILED at 352. Hunk #9 succeeded at 569 (offset 25 lines). Hunk #10 succeeded at 584 (offset 25 lines). 4 out of 10 hunks FAILED -- saving rejects to file directory/xpcom/base/src/nsLDAPConnection.cpp.rej patching file directory/xpcom/base/src/nsLDAPConnection.h patching file directory/xpcom/base/src/nsLDAPMessage.h patching file directory/xpcom/base/src/nsLDAPOperation.cpp Hunk #1 succeeded at 323 (offset 98 lines). Hunk #2 succeeded at 336 (offset 98 lines). Hunk #3 succeeded at 520 (offset 98 lines). Hunk #4 succeeded at 585 (offset 98 lines). Hunk #5 succeeded at 732 (offset 98 lines). Hunk #6 succeeded at 784 (offset 98 lines). Hunk #7 succeeded at 899 (offset 98 lines). Hunk #8 succeeded at 964 (offset 98 lines).

for activating this bug. I've generated the patch before thinking. This means some new features implemented in the original source file after generating attachment 355986 [details] [diff] [review] may be lost. I'm only a patch fitter. I hope guru will complete this bug.

(In reply to comment #6) > Created attachment 468590 [details] [diff] [review] > Fit attachment 355986 [details] [diff] [review] to the latest comm-central > > for activating this bug. > > I've generated the patch before thinking. This means some new features > implemented in the original source file after generating attachment 355986 [details] [diff] [review] may > be lost. > > I'm only a patch fitter. I hope guru will complete this bug. Perhaps feedback should be requested anyway?

Any progress lately? Seems to be related to #590494.

(In reply to comment #8) > Any progress lately? Seems to be related to #590494. I've not had time recently, hence the helpwanted. Basically this patch needs updating, and then some sort of xpcom-shutdown hook observer to kill the LDAP connection before the app gets shut down.

Fixed patch that can be applied on latest comm-central. Looking into xpcom-shutdown hook observer.

I've added to patch xpcom-shutdown observer to abandon ldap operation. There's no crash during shutdown. However there are leaking urls: ldap://ldap.*****.com/dc=redhat,dc=com??sub?(objectclass=*) ldap://ldap.*****.com/dc=*****,dc=com?birthday,o,company,mail,mozillaCustom3,custom3,mozillaUseHtmlMail,xmozillausehtmlmail,mozillaCustom2,custom2,mozillaHomeCountryName,ou,department,departmentnumber,orgunit,mobile,cellphone,carphone,telephoneNumber,title,mozillaCustom1,custom1,sn,surname,mozillaNickname,xmozillanickname,mozillaWorkUrl,workurl,labeledURI,facsimiletelephonenumber,fax,mozillaSecondEmail,xmozillasecondemail,mozillaCustom4,custom4,nsAIMid,nscpaimscreenname,street,streetaddress,postOfficeBox,givenName,l,locality,homePhone,mozillaHomeUrl,homeurl,mozillaHomeStreet,st,region,mozillaHomePostalCode,mozillaHomeLocalityName,mozillaWorkStreet2,mozillaHomeStreet2,postalCode,zip,birthmonth,c,countryname,pager,pagerphone,mozillaHomeState,description,notes,birthyear,modifytimestamp,cn,commonname,objectClass?sub?(|(mail=*s*)(cn=*s*)(givenName=*s*)(sn=*s*)) I'm not sure if this is serious issue when leak occurs only when ldap operation is proceeding during shutdown. I've added PR_Sleep to ldap searching thread to be able to test shutting down app during search. I would like to know how far we are from accepting patch. TIA.

Ouch. I forgot to mention that I set LDAP *mConnectionHandle as a public member just for testing. This have to be solved differently. I'm aware of it.

I've hit an synchronization problem: In AbortPendingOperations, which is an enumeration function over opened connections called during shutdown, I would like to use nsILDAPOperation::AbandonExt() to cancel LDAP operation. The problem is that AbandonExt try to remove from hashtable which I'm currently enumerating by AbortPendingOperations. It is waiting until lock is freed, which is after finishing enumerating so this seems to be deadlock. One solution which comes to my mind is to change: AbandonExt(void) to: AbandonExt(PRBool remove_from_hashtable = PR_TRUE) when remove_from_hashtable is called by PR_FALSE it wouldn't try to remove entry from hash table and it will be removed by AbortPendingOperation by returning PL_DHASH_REMOVE. I try to add a patch with my proposed solution.

The patch in bug 609585 may help you here. If you stick the patch up next week and request feedback from me I'll try and remember to take a look at it.

So making copy of the hashtable is more preferred?

Um, mPendingOperations is nsInterfaceHashtableMT which does not have Clone() method (unlike nsHashtable). I'd rather keep it that way because nsInterfaceHashtable can be used in frozen linkage. We will have to create the copy manually or change AbandonExt function (which is not generic solution for this kind of problem). Could you please help me out?

Comment on attachment 490834 [details] [diff] [review] WIP patch including test case. see previous 2 comments

My patch is based on: https://bugzilla.mozilla.org/attachment.cgi?id=468590&action=diff and there is the change from nsSupportsHashtable to nsInterfaceHashtableMT. I still have problem with AbandonExt and solution proposed in https://bugzilla.mozilla.org/show_bug.cgi?id=609585 as long as nsInterfaceHashtableMT does not have clone method is unfortunately unusable. I would have to clone it manually, is this a proposed solution?

- sync to comm-central - pointers to pending operations copied to array to avoid lockup Please have a look and let me know.

Comment on attachment 501686 [details] [diff] [review] proposed patch 1 I've reviewed this using the review board which you can find here: http://reviews.visophyte.org/r/501686/ Here's the comments for bugzilla records: Sorry for the delay in reviewing this, I had issues getting a LDAP server connected to my Thunderbird. The patch looks really good, and I've been testing it and with a couple of tweaks it works really well and solves this bug and bug 544939 both of which will be really useful. As I wrote some of the original patch, I'd just like David to take a quick look as well to verify that the changes look reasonable. Thanks for your work on getting this completed. on file: ldap/xpcom/src/nsLDAPConnection.h line 108 > nsresult AddPendingOperation(PRInt32 aOperationID, nsILDAPOperation *aOperation); I think its would probably better to use PRUint32 here. Especially as the mPendingOperation expects a nsUint32HashKey. This also means you can remove the cast within in the function. on file: ldap/xpcom/src/nsLDAPConnection.h line 119 > nsresult RemovePendingOperation(PRInt32 aOperationID); Ditto with using PRUint32. on file: ldap/xpcom/src/nsLDAPConnection.cpp line 82 > obsServ->AddObserver(this, "xpcom-shutdown", PR_FALSE); Ok, contrary to what I originally though, "xpcom-shutdown" isn't quite early enough, this actually needs to be "profile-change-net-teardown". This combined with one other change will fix bug 544939 for us. on file: ldap/xpcom/src/nsLDAPConnection.cpp line 105 > NS_IMPL_CI_INTERFACE_GETTER3(nsLDAPConnection, nsILDAPConnection, You need to add nsIObserver in here (and increment the number), and add nsIObserver as an NS_INTERFACE_MAP_ENTRY. Its probably not strictly necessarily to do this, but just in case someone does try to QI to an nsIObserver, it'd probably be good to have. on file: ldap/xpcom/src/nsLDAPConnection.cpp line 259 > if (!nsCRT::strcmp(aTopic, "xpcom-shutdown")) { Please use "profile-change-net-teardown" as mentioned earlier. on file: ldap/xpcom/src/nsLDAPConnection.cpp line 272 > } After checking that we've abandoned the pending operations, please can you add a call to Close(). This will then mean we'll close the connection early enough at shutdown and properly fix bug 544939 in the process.

(In reply to comment #20) > in case someone does try to QI to an nsIObserver, it'd probably be good to have. nsCOMPtr<nsIObserver> automatically warns about this in debug builds.

Comment on attachment 501686 [details] [diff] [review] proposed patch 1 >+ nsIRunnable* runnable = new nsLDAPConnectionRunnable(aOperationID, aOperation, >+ this); >+ nsresult rv; >+ if (!mThread) >+ { >+ rv = NS_NewThread(getter_AddRefs(mThread), runnable); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } >+ else >+ { >+ rv = mThread->Dispatch(runnable, nsIEventTarget::DISPATCH_NORMAL); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } > >+ mPendingOperations.Put((PRUint32)aOperationID, aOperation); [Is this a potential race condition?] >+ if (listener) >+ listener->OnLDAPMessage(aMsg); Nit: I guess this should be 2 space indent now. >+ nsIThread* thread = NS_GetCurrentThread(); ... >+ nsIThread* thread = NS_GetCurrentThread(); ... >+ nsIThread* thread = NS_GetCurrentThread(); I don't think this compiles under the external API! Also, aren't these are all in the same method, so only the first is necessary? > */ >- nsresult AddPendingOperation(nsILDAPOperation *aOperation); >+ nsresult AddPendingOperation(PRInt32 aOperationID, nsILDAPOperation *aOperation); Nit: probably wrong to change the indentation here. > NS_IMETHODIMP > nsLDAPOperation::Init(nsILDAPConnection *aConnection, >@@ -141,7 +141,7 @@ > > // cache the connection handle > // >- mConnectionHandle = >+ mConnectionHandle = > static_cast<nsLDAPConnection *>(aConnection)->mConnectionHandle; On an unrelated note, I see that nsLDAPOperation casts its nsILDAPConnection to nsLDAPConnection all over (well, all except three places). Does anyone think it might be an idea to change mConnection to an nsRefPtr<nsLDAPConnection>? >- >+ [Unrelated whitespace changes make parts of this patch harder to read.]

(In reply to comment #22) > Comment on attachment 501686 [details] [diff] [review] > proposed patch 1 > > >+ nsIRunnable* runnable = new nsLDAPConnectionRunnable(aOperationID, aOperation, > >+ this); > >+ nsresult rv; > >+ if (!mThread) > >+ { > >+ rv = NS_NewThread(getter_AddRefs(mThread), runnable); > >+ NS_ENSURE_SUCCESS(rv, rv); > >+ } > >+ else > >+ { > >+ rv = mThread->Dispatch(runnable, nsIEventTarget::DISPATCH_NORMAL); > >+ NS_ENSURE_SUCCESS(rv, rv); > >+ } > > > >+ mPendingOperations.Put((PRUint32)aOperationID, aOperation); > [Is this a potential race condition?] If you mean that we should do the Put before the create thread/dispatch, that's probably a good idea. > >+ nsIThread* thread = NS_GetCurrentThread(); > ... > >+ nsIThread* thread = NS_GetCurrentThread(); > ... > >+ nsIThread* thread = NS_GetCurrentThread(); > I don't think this compiles under the external API! Also, aren't these are all > in the same method, so only the first is necessary? Just a note that I think I said to jhorak that we should probably not care about external API yet, but get this patch in without too much extra complexity (there's other bits which I believe won't compile yet), and then we can get the external API sorted later, hopefully in an easier way ;-) > >- mConnectionHandle = > >+ mConnectionHandle = > > static_cast<nsLDAPConnection *>(aConnection)->mConnectionHandle; > On an unrelated note, I see that nsLDAPOperation casts its nsILDAPConnection to > nsLDAPConnection all over (well, all except three places). Does anyone think it > might be an idea to change mConnection to an nsRefPtr<nsLDAPConnection>? That would be reasonable. > >- > >+ > [Unrelated whitespace changes make parts of this patch harder to read.] Using review board its much easier to filter these out of the reading ;-)

Comment on attachment 501686 [details] [diff] [review] proposed patch 1 And thinking about it, I'll switch the additional review to Neil as he's pretty much just looked through it anyway ;-)

(In reply to comment #23) > (In reply to comment #22) > > I don't think this compiles under the external API! > Just a note that I think I said to jhorak that we should probably not care > about external API yet Well, this was the only line that I spotted. But now you've requested my review, I might as well try this on my experimental external API build too :-P

(In reply to comment #25) > (In reply to comment #23) > > (In reply to comment #22) > > > I don't think this compiles under the external API! > > Just a note that I think I said to jhorak that we should probably not care > > about external API yet > Well, this was the only line that I spotted. But now you've requested my > review, I might as well try this on my experimental external API build too :-P Oh, LDAP isn't counted as part of the experimental external API build...

> On an unrelated note, I see that nsLDAPOperation casts its nsILDAPConnection to > nsLDAPConnection all over (well, all except three places). Does anyone think it > might be an idea to change mConnection to an nsRefPtr<nsLDAPConnection>? I'm not sure how to interpret this, could you show me some example of how to remove static_cast<> or solve this issue?

A nsRefPtr is a refcounted concrete implementation class, instead of having nsCOMPtr<nsILDAPConnection> mConnection and continuously casting mConnection.get() to its concrete type, you could just use nsRefPtr<nsLDAPConnection> mConnection.

(In reply to comment #27) > > On an unrelated note, I see that nsLDAPOperation casts its nsILDAPConnection to > > nsLDAPConnection all over (well, all except three places). Does anyone think it > > might be an idea to change mConnection to an nsRefPtr<nsLDAPConnection>? > I'm not sure how to interpret this, could you show me some example of how to > remove static_cast<> or solve this issue? It was an unrelated note, so I hope your change of nsLDAPConnectionRunnable's mConnection to an nsRefPtr<nsLDAPConnection> was unrelated too ;-)

Comment on attachment 509405 [details] [diff] [review] proposed patch 2 >@@ -136,17 +136,17 @@ nsLDAPOperation::Init(nsILDAPConnection *aConnection, > // set the member vars > // > mConnection = aConnection; > mMessageListener = aMessageListener; > mClosure = aClosure; > > // cache the connection handle > // >- mConnectionHandle = >+ mConnectionHandle = > static_cast<nsLDAPConnection *>(aConnection)->mConnectionHandle; Here's an example, by the way. Because aConnection is an nsILDAPConnection rather than an nsLDAPConnection the cast has been added to access the mConnectionHandle. (I don't know if we can change the way the nsLDAPOperation is constructed to avoid passing the interface type instead of the concrete type.) mConnection is currently an nsCOMPtr<nsILDAPConnection> to match aConnection, but a number of times throughout the file we cast it to nsLDAPConnection* (which is even uglier, because .get() used to return an nsDerivedSafe<nsILDAPConnection>). If we change mConnection to an nsRefPtr<nsLDAPConnection> then we eliminate all the casts on the way out. (We still have to cast aConnection in the initial assignment of course.)

Comment on attachment 509405 [details] [diff] [review] proposed patch 2 There's one thing that worries me after reading this patch. The old code used to work by polling mPendingOperations and running CheckLDAPOperationResult. This meant that simply removing the operation from mPendingOperations was enough to effectively cancel the operation (assuming the thread wasn't checking it at the time). This doesn't seem to happen in this code. Everything seems to work when I try an address book search, so am I overlooking something? > NS_INTERFACE_MAP_BEGIN(nsLDAPConnection) > NS_INTERFACE_MAP_ENTRY(nsILDAPConnection) > NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference) > NS_INTERFACE_MAP_ENTRY(nsIDNSListener) >+NS_INTERFACE_MAP_ENTRY(nsIObserver) > NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsILDAPConnection) > NS_IMPL_QUERY_CLASSINFO(nsLDAPConnection) > NS_INTERFACE_MAP_END_THREADSAFE Nit: wrong indentation on the new map entry. In fact I think this can be written NS_IMPL_QUERY_INTERFACE4_CI(nsLDAPConnection, nsILDAPConnection, nsISupportsWeakReference, nsIDNSListener, nsIObserver) Sadly there's no NS_IMPL_THREADSAFE_QUERY_INTERFACE4_CI macro ;-) >- // allocate a hashtable to keep track of pending operations. >- // 10 buckets seems like a reasonable size, and we do want it to >- // be threadsafe >- mPendingOperations = new nsSupportsHashtable(10, PR_TRUE); >- if (!mPendingOperations) { >- NS_ERROR("failure initializing mPendingOperations hashtable"); >- return NS_ERROR_OUT_OF_MEMORY; >- } >+ // Initialise the hashtable to keep track of pending operations. >+ // 10 buckets seems like a reasonable size. >+ mPendingOperations.Init(10); Does this use "infallible" malloc or do we need to keep the OOM check? >+ return NS_ERROR_UNEXPECTED; Nit: trailing whitespace (this is the last of 5 occurrences). >+ nsCOMPtr<nsILDAPMessage> msg; Could make this an nsRefPtr<nsLDAPMessage> to avoid having to have rawMsg. >+ nsIThread* thread = NS_GetCurrentThread(); I notice that existing ldap/xpcom/src code also uses nsCOMPtr<nsIThread> thread = do_GetCurrentThread();

Ah, I see. Without static_casts code looks much nicer. Please have a look.

Comment on attachment 509721 [details] [diff] [review] proposed patch 3 Very nice, except somehow the trailing whitespace count went up... >+ nsCOMPtr<nsIObserverService> obsServ = >+ mozilla::services::GetObserverService(); >+ obsServ->AddObserver(this, "profile-change-net-teardown", PR_FALSE); >+ (first and last of these lines) >+NS_IMPL_CI_INTERFACE_GETTER4(nsLDAPConnection, nsILDAPConnection, >+ nsTArray<nsILDAPOperation*>* pending_operations = static_cast<nsTArray<nsILDAPOperation*>* >(userArg); >+ /* We cannot use enumerate function to abort operations because >+ mPendingOperations.Put((PRUint32)aOperationID, aOperation); >+ (latter line)

Can I set checkin-needed or does this need superreview?

Comment on attachment 510285 [details] [diff] [review] Final patch 4 Carrying Neil's r+ over. There seems to be no API change, so no need for sr.

jhorak's attached quite enough patches as it is but whomever checks in please check as I think there are a few trailing whitespaces left still. Thanks.

Checked in: http://hg.mozilla.org/comm-central/rev/ee3e25817685 Note: I fixed Neil's whitespaces issues, plus I also moved the added Close() call slightly so that it took account of the pending operations = 0 case (we still need to close in that case because we keep the connection open), and I removed a redundant nsresult rv; that we hadn't spotted.

is this unlikely to be accepted to land on branch? (being such a large patch)

(In reply to comment #38) > is this unlikely to be accepted to land on branch? (being such a large patch) to be clear, I'm thinking of this in terms of the hang bug 524315 that this fixes

(In reply to comment #38) > is this unlikely to be accepted to land on branch? (being such a large patch) Yes, this is way too much for a branch release.