557928 - crash [@ nsLDAPOperation::SimpleBind(nsACString_internal const&)] and [@ @0x0 | nsLDAPOperation::SimpleBind(nsACString_internal const&)]

Status: VERIFIED FIXED

(MailNews Core :: LDAP Integration, defect)

Description

[Wayne Mery (:wsmwk)]

crash [@ nsLDAPOperation::SimpleBind(nsACString_internal const&)] and [@ @0x0 | nsLDAPOperation::SimpleBind(nsACString_internal const&)]

low rate crash

bp-89737fbd-f0c5-439e-9a55-99b342100401
creating an address list

bp-51d15fb8-e124-412d-9291-10f9c2100407 (larry) (no crash comment)
0		@0x75ff50b8	
1	thunderbird.exe	nsLDAPOperation::SimpleBind	directory/xpcom/base/src/nsLDAPOperation.cpp:337
2	thunderbird.exe	nsAbLDAPListenerBase::OnLDAPInit	mailnews/addrbook/src/nsAbLDAPListenerBase.cpp:323
3	thunderbird.exe	nsLDAPAutoCompleteSession::OnLDAPInit	mailnews/addrbook/src/nsLDAPAutoCompleteSession.cpp:444
4	xpcom_core.dll	NS_InvokeByIndex_P	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
5	xpcom_core.dll	nsProxyObjectCallInfo::Run	xpcom/proxy/src/nsProxyEvent.cpp:181
6	xpcom_core.dll	nsThread::ProcessNextEvent	xpcom/threads/nsThread.cpp:521
7	xpcom_core.dll	NS_ProcessNextEvent_P	objdir-tb/mozilla/xpcom/build/nsThreadUtils.cpp:247
8	thunderbird.exe	nsXULWindow::ShowModal	xpfe/appshell/src/nsXULWindow.cpp:415
9	thunderbird.exe	nsContentTreeOwner::ShowAsModal	xpfe/appshell/src/nsContentTreeOwner.cpp:528
10	thunderbird.exe	nsWindowWatcher::OpenWindowJSInternal	embedding/components/windowwatcher/src/nsWindowWatcher.cpp:1004
11	thunderbird.exe	nsWindowWatcher::OpenWindowJS	embedding/components/windowwatcher/src/nsWindowWatcher.cpp:488
12	thunderbird.exe	nsGlobalWindow::OpenInternal	dom/src/base/nsGlobalWindow.cpp:7435
13	thunderbird.exe	nsGlobalWindow::OpenDialog	dom/src/base/nsGlobalWindow.cpp:5200

Comment 1

[timeless]

296 nsLDAPOperation::SimpleBind(const nsACString& passwd)
313     rv = mConnection->GetBindName(bindName);

323     if (originalMsgID)
324       static_cast<nsLDAPConnection *>(static_cast<nsILDAPConnection *>(mConnection.get()))->RemovePendingOperation(this);

326     mMsgID = ldap_simple_bind(mConnectionHandle, bindName.get(),
327                               PromiseFlatCString(mSavePassword).get());

So, this should be a null pointer dereference of mConnection.
337     rv = static_cast<nsLDAPConnection *>(static_cast<nsILDAPConnection *>(mConnection.get()))->AddPendingOperation(this);

The easiest way for this to be a problem is if ldap_simple_bind or the RemovePendingOperation clears mConnection. Neither seems likely.

555 nsLDAPOperation::AbandonExt()
584     if (mConnection)
585     {
586       rv = static_cast<nsLDAPConnection *>(static_cast<nsILDAPConnection *>(mConnection.get()))->RemovePendingOperation(this);

---
465 nsLDAPConnection::InvokeMessageCallback(LDAPMessage *aMsgHandle, 
535     if (aRemoveOpFromConnQ) {
536         nsCOMPtr <nsLDAPOperation> operation = 
537           getter_AddRefs(static_cast<nsLDAPOperation *>
538                                     (mPendingOperations->Get(key)));
539         // try to break cycles
540         if (operation)
541           operation->Clear();

One possibility is that another thread has reached here.

96 NS_IMPL_THREADSAFE_ADDREF(nsLDAPConnection)

the class claims to be threadsafe, but i see no locking.

I'd tentatively suggest grabbing a local reference to mConnection at the beginning of nsLDAPOperation::SimpleBind and using it instead of the member variable (or using whichever lock this nsLDAPConnection class supposedly has).

Comment 2

[Wayne Mery (:wsmwk)]

see comment 1 please

Comment 3

[Wayne Mery (:wsmwk)]

https://crash-stats.mozilla.com/report/index/ac333ae0-57f1-422a-bff1-22fc72120313 has reporter's address - perhaps he can get us an ldap log

Comment 4

[Leon Sha]

Attachment: patch

Patch according to comments 1.

Comment 5

[David :Bienvenu]

Comment on attachment 620176 [details] [diff] [review]
patch

seems reasonable to try, thx for the patch.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Leaving it open for now since it's not clear that this will for sure fix the crash. Feel free to close it if you disagree.
http://hg.mozilla.org/comm-central/rev/65e0569b34cc

Comment 7

[Wayne Mery (:wsmwk)]

cant tell via crash stats until after v15 is released

Comment 8

[Phoenix]

Resovled per whiteboard

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 620176 [details] [diff] [review]
patch

[Triage Comment]
I can't find any reports of this in 15.0.1 and it seems a safe fix, so I think we can take this into ESR 10 and fix it there as well.

Comment 10

[Mark Banner (:standard8)]

Checked in: https://hg.mozilla.org/releases/comm-esr10/rev/c0a7bf8041be

Comment 11

[Wayne Mery (:wsmwk)]

yup verified - No crashes in TB15.
only crashes were TB10.0.7esr which will now be fixed :)