531716 - crash [@ nsAbMDBCard::Equals(nsIAbCard*, int*)]

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Wayne Mery (:wsmwk)]

crash [@ nsAbMDBCard::Equals(nsIAbCard*, int*)]
exactly one crash in 1 year

bp-b5857ac0-9702-4184-8c75-088642091124 v3.0b4
0  	thunderbird.exe  	nsAbMDBCard::Equals  	 mailnews/addrbook/src/nsAbMDBCard.cpp:72
1 	thunderbird.exe 	nsAbView::FindIndexForCard 	mailnews/addrbook/src/nsAbView.cpp:969
2 	thunderbird.exe 	nsAbView::OnItemPropertyChanged 	mailnews/addrbook/src/nsAbView.cpp:985
3 	thunderbird.exe 	nsAbManager::NotifyItemPropertyChanged 	mailnews/addrbook/src/nsAbManager.cpp:372
4 	thunderbird.exe 	nsAbMDBDirectory::NotifyItemChanged 	mailnews/addrbook/src/nsAbMDBDirectory.cpp:250
5 	thunderbird.exe 	nsAbMDBDirectory::OnCardEntryChange 	mailnews/addrbook/src/nsAbMDBDirectory.cpp:872
6 	thunderbird.exe 	nsAddrDatabase::NotifyCardEntryChange 	mailnews/addrbook/src/nsAddrDatabase.cpp:244
7 	thunderbird.exe 	nsAddrDatabase::EditCard 	mailnews/addrbook/src/nsAddrDatabase.cpp:1945
8 	thunderbird.exe 	nsAbMDBDirectory::ModifyCard 	mailnews/addrbook/src/nsAbMDBDirectory.cpp:727
9 	thunderbird.exe 	nsMsgCompose::CheckAndPopulateRecipients 	mailnews/compose/src/nsMsgCompose.cpp:4727

Comment 1

[Wayne Mery (:wsmwk)]

no idea why I filed this, but it still happens.
bp-433fb681-a96c-47ea-a191-dc7172110825 version 6

Comment 2

[:aceman]

A null check on 'card' could help here :)

bp-b8173c1a-2a00-4790-935e-48f502120817

Comment 3

[:aceman]

Attachment: patch

Comment 4

[Kent James (:rkent)]

 NS_IMETHODIMP nsAbMDBCard::Equals(nsIAbCard *card, bool *result)
 {
+  NS_ENSURE_ARG_POINTER(card);
+  NS_ENSURE_ARG_POINTER(result);
+

You could return false if card is null - a null card does not equal a real card.

Comment 5

[:aceman]

Yes, I thought about that, but maybe it is better to tell the developer he sends garbage, with an error?

That is a semantic decision so let's wait for mconley.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

(In reply to :aceman from comment #5)
> Yes, I thought about that, but maybe it is better to tell the developer he
> sends garbage, with an error?
> 
> That is a semantic decision so let's wait for mconley.

Hm. So the point of this function is to compare two nsIAbCard's for equality. A nullptr (nee nsnull) can be assigned to an nsIAbCard pointer, but is definitely not an nsIAbCard.

So I think I'd rather the caller ensure that the card they're passing is not nsnull before checking equality. So I'm going to side with aceman on this.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

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

Review of attachment 656908 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Thanks aceman!

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/cba58fffc994