644546 - crash [@ nsAddrDatabase::CreateCard(nsIMdbRow*, unsigned int, nsIAbCard**)] when viewing a replicated/downloaded LDAP directory in offline mode.

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Mark Banner (:standard8)]

This bug was filed from the Socorro interface and is 
report bp-26417e36-8480-4e98-a319-d49242110323 .
============================================================= 

Trunk issue only, regression from bug 

STR:
1) Download a LDAP directory for offline use.
2) Go offline
3) Select the LDAP directory in the address book to view it.

=> Crash.

The basic issue is due to the way LDAP works with MDB databases, we're releasing a pointer and not knowing about it.

Top of stack:

0 	xul.dll 	nsAddrDatabase::CreateCard 	mailnews/addrbook/src/nsAddrDatabase.cpp:2972
1 	xul.dll 	nsAddrDatabase::AddListCardColumnsToRow 	mailnews/addrbook/src/nsAddrDatabase.cpp:1451
2 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
3 	xul.dll 	nsProxyObjectCallInfo::Run 	xpcom/proxy/src/nsProxyEvent.cpp:181
4 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:633
5 	xul.dll 	NS_ProcessNextEvent_P 	objdir-tb/mozilla/xpcom/build/nsThreadUtils.cpp:250
6 	xul.dll 	nsXULWindow::ShowModal 	xpfe/appshell/src/nsXULWindow.cpp:419

Comment 1

[Mark Banner (:standard8)]

Attachment: Proposed fix

Here's what is happening:

1) nsAbView calls nsAbLDAPDirectory::GetChildCards.
2) We're in offline mode, and we have a replicated database. So at this stage, nsAbLDAPDirectory::GetChildCards goes and gets an nsIAbMDBDirectory with a URI that has the filename of the replicated database.
3) We then call nsAbMDBDirectory::GetChildCards, this sees that we're not doing a query, so goes to return all cards.
4) In getting the cards it calls nsAddrDatabase::EnumerateCards with itself as a parameter.
5) nsAddrDatabase::EnumerateCards saves the nsAbMDBDirectory in a *simple* pointer, and returns an nsAddrDBEnumerator.
6) We then go back up the stack, and as we do, the nsAbMDBDirectory gets freed because the only thing holding onto it was a local variable in nsAbLDAPDirectory::GetChildCards.
7) nsAbView then tries to iterate through the Enumerator, at which point we try and get the Uuid of the directory from the simple pointer, and we crash with a bad access because that's no longer available.

We were avoiding this before bug 444093 as we weren't tending to access the nsAbMDBDirectory from the enumerator, and as LDAP address books don't currently support mailing lists we really did get away with it.

The full fix would probably to pass in the nsIAbLDAPDirectory as that's what we should be using to get the Uuid. However, I'm not sure that's going to be very nice for our APIs and the right fix would be to have some appropraite separation that meant we weren't using nsAbMDBDirectory as workaround.

The fix I'm proposing here will cause no Uuid to be set on the cards. It does this by making the nsAddrDatabase hold a weak reference rather than a simple pointer. I didn't want to make it a reference pointer, as I suspect we would get into reference cycle issues with nsAddrDatabase and nsAbMDBDirectory holding onto each other.

Comment 2

[Mark Banner (:standard8)]

Attachment: Proposed fix v2

This time without the accidental mozmilltests.list change.

Comment 3

[David :Bienvenu]

Comment on attachment 521487 [details] [diff] [review]
Proposed fix v2

this does fix the crash, but we do get a lot of warnings. Is the plan that eventually these warnings will get fixed? Or maybe only warn if not offline, since we expect these while offline?

Comment 4

[Mark Banner (:standard8)]

(In reply to comment #3)
> Comment on attachment 521487 [details] [diff] [review]
> Proposed fix v2
> 
> this does fix the crash, but we do get a lot of warnings. Is the plan that
> eventually these warnings will get fixed? Or maybe only warn if not offline,
> since we expect these while offline?

I think I'd be happy just scrubbing that warning for now and we can have a bug on fixing it properly for LDAP.

Comment 5

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/3f6f17ce3567

Comment 6

[Philip Chee]

Callek/SM drivers. Mark and Neil say that this patch is probably safe to take on the comm-2.0 branch. Requesting approval. Thunderbird doesn't care about this branch any more.

Comment 7

[Justin Wood (:Callek)]

(In reply to comment #6)
> Callek/SM drivers. Mark and Neil say that this patch is probably safe to take
> on the comm-2.0 branch. Requesting approval. Thunderbird doesn't care about
> this branch any more.

So long as its not l10n, c-2.0 is OPEN not APPROVAL_REQUIRED :-P

Comment 8

[Philip Chee]

Pushed to comm-20
http://hg.mozilla.org/releases/comm-2.0/rev/0f0f0373dbf7

Comment 9

[[e-mail]]

Comment on attachment 521487 [details] [diff] [review]
Proposed fix v2

>diff --git a/mailnews/addrbook/src/nsAddrDatabase.h b/mailnews/addrbook/src/nsAddrDatabase.h
...
>+#include "nsWeakPtr.h"
...
>+  nsWeakPtr m_dbDirectory;

nsWeakPtr.h doesn't actually define nsWeakPtr ;-)
[This breaks experimental external API builds.]

Comment 10

[[e-mail]]

(In reply to comment #9)
> nsWeakPtr.h doesn't actually define nsWeakPtr ;-)
It's in nsIWeakReferenceUtils.h apparently.