616229 - crash [@ nsMsgXFGroupThread::SetMsgHdrAt(unsigned int, nsIMsgDBHdr*)]

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect)

Description

[Wayne Mery (:wsmwk)]

crash [@ nsMsgXFGroupThread::SetMsgHdrAt(unsigned int, nsIMsgDBHdr*)]

~#200 crash

bp-4efc40cf-956b-4251-9f4c-6ff822101122 (web345)
EXCEPTION_ACCESS_VIOLATION_READ
0x0
0	Thunderbird.EXE	nsMsgXFGroupThread::SetMsgHdrAt	mailnews/base/src/nsMsgGroupThread.cpp:862
1	Thunderbird.EXE	nsMsgGroupThread::RemoveChildHdr	mailnews/base/src/nsMsgGroupThread.cpp:305
2	Thunderbird.EXE	nsMsgGroupView::OnHdrDeleted	mailnews/base/src/nsMsgGroupView.cpp:710
3	Thunderbird.EXE	nsMsgSearchDBView::OnHdrDeleted	mailnews/base/src/nsMsgSearchDBView.cpp:250
4	Thunderbird.EXE	nsMsgDatabase::NotifyHdrDeletedAll	mailnews/db/msgdb/src/nsMsgDatabase.cpp:724
5	Thunderbird.EXE	nsMsgDatabase::DeleteHeader	mailnews/db/msgdb/src/nsMsgDatabase.cpp:1810
6	Thunderbird.EXE	nsMsgLocalMailFolder::DeleteMessage	mailnews/local/src/nsLocalMailFolder.cpp:2276
7	Thunderbird.EXE	nsMsgLocalMailFolder::DeleteMessages	mailnews/local/src/nsLocalMailFolder.cpp:1515 

bp-c2fd020c-a5f6-4d14-9767-8632e2101122 (mchodrow)

Comment 1

[Wayne Mery (:wsmwk)]

a couple recent examples:
bp-aac127ad-d405-40cb-9eea-8f2812110328
EXCEPTION_ACCESS_VIOLATION_READ
0x0
0	thunderbird.exe	nsMsgXFGroupThread::SetMsgHdrAt	mailnews/base/src/nsMsgGroupThread.cpp:862
1	thunderbird.exe	nsMsgGroupThread::RemoveChildHdr	mailnews/base/src/nsMsgGroupThread.cpp:305
2	thunderbird.exe	nsMsgGroupView::OnHdrDeleted	mailnews/base/src/nsMsgGroupView.cpp:710
3	thunderbird.exe	nsMsgSearchDBView::OnHdrDeleted	mailnews/base/src/nsMsgSearchDBView.cpp:250
4	thunderbird.exe	nsMsgDatabase::NotifyHdrDeletedAll	mailnews/db/msgdb/src/nsMsgDatabase.cpp:724
5	thunderbird.exe	nsMsgDatabase::DeleteHeader	mailnews/db/msgdb/src/nsMsgDatabase.cpp:1810
6	thunderbird.exe	nsMsgDatabase::DeleteMessages	mailnews/db/msgdb/src/nsMsgDatabase.cpp:1753
7	thunderbird.exe	nsImapMailFolder::UpdateImapMailboxInfo	mailnews/imap/src/nsImapMailFolder.cpp:2860
8	xpcom_core.dll	NS_InvokeByIndex_P	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
9	xpcom_core.dll	nsProxyObjectCallInfo::Run	xpcom/proxy/src/nsProxyEvent.cpp:181 

bp-801fcc5d-3d40-4b3e-8134-3bf672110328
bp-02d2e3f2-d2bc-4881-8d87-f11192110328

(there was no response from the reporters cited in comment 0)

Comment 2

[timeless]

the old code wasn't worried about nulls, the new code

http://hg.mozilla.org/releases/comm-1.9.2/diff/986be3aa6df3/mailnews/base/src/nsMsgGroupThread.cpp#l1.135
passes null to the crashing function

Comment 3

[Wayne Mery (:wsmwk)]

(In reply to timeless from comment #2)
> the old code wasn't worried about nulls, the new code
> 
> http://hg.mozilla.org/releases/comm-1.9.2/diff/986be3aa6df3/mailnews/base/
> src/nsMsgGroupThread.cpp#l1.135
> passes null to the crashing function

bp-e3b4ebef-3b09-492b-9a8a-459f12121210

I imagine bug 646810 is a duplicate

Comment 4

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

Ensure GetChildHdrAt returns proper rv, and also check the rv after calling it.

Comment 5

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 716468 [details] [diff] [review]
proposed fix

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

This change looks good and compiles+runs cleanly, but I'd like to know whether there's a an underlying issue before giving r+.

Does anybody have steps to reproduce?

::: mailnews/base/src/nsMsgGroupThread.cpp
@@ +241,3 @@
>    uint32_t flags;
>    nsMsgKey key;
>    

Might as well kill this trailing white space since you're deleting the next lines...

@@ +790,5 @@
>  NS_IMETHODIMP nsMsgXFGroupThread::GetChildHdrAt(int32_t aIndex, nsIMsgDBHdr **aResult)
>  {
>    if (aIndex < 0 || aIndex >= m_folders.Count())
>      return NS_MSG_MESSAGE_NOT_FOUND;
> +  return m_folders.ObjectAt(aIndex)->GetMessageHeader(m_keys[aIndex], aResult);

Is there an underlying bug causing this call to fail, or is it normal for there to not always be a value?

Comment 6

[Magnus Melin [:mkmelin]]

(In reply to Irving Reid (:irving) from comment #5)
> Is there an underlying bug causing this call to fail, or is it normal for
> there to not always be a value?

I don't have any steps to reproduce, but GetMessageHeader can end up calling a lot of code that can potentially fail when doing GetDatabase(). 
It's also possible the crash is simply because of the NS_MSG_MESSAGE_NOT_FOUND case above.

Comment 7

[Mihai Sucan [:msucan]]

Can this crash fix land in nightlies, aurora and/or betas of thunderbird?

Comment 8

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 716468 [details] [diff] [review]
proposed fix

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

OK, let's land this to fix the crash.

Comment 9

[Magnus Melin [:mkmelin]]

http://hg.mozilla.org/comm-central/rev/82aa521103e2 -> FIXED

Comment 10

[:aceman]

Mihai, can you test if this patch fixes your problem here and in bug 842963?

Comment 11

[:aceman]

Comment on attachment 716468 [details] [diff] [review]
proposed fix

[Approval Request Comment]
Regression caused by (bug #): bug 481824 ?
User impact if declined: crash
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky):

Comment 12

[Mark Banner (:standard8)]

This is a low level crash, so I think we'll let it ride the trains and push it to esr in sync with gecko 22.

Comment 13

[Mihai Sucan [:msucan]]

(In reply to :aceman from comment #10)
> Mihai, can you test if this patch fixes your problem here and in bug 842963?

thunderbird-next in the mozilla team ubuntu PPA still points to thunderbird 19. I could build thunderbird myself but I do not yet have a proper workflow with thunderbird builds.

Thank you for the fix. I hope this works for me, once the ppa is updated. I will mark my bug report as fixed once I can test this fix.

Comment 14

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-esr17/rev/db4c8f00540a

Comment 15

[Wayne Mery (:wsmwk)]

gone in version 17.0.7