Last Comment Bug 616229 - crash [@ nsMsgXFGroupThread::SetMsgHdrAt(unsigned int, nsIMsgDBHdr*)]
: crash [@ nsMsgXFGroupThread::SetMsgHdrAt(unsigned int, nsIMsgDBHdr*)]
Status: VERIFIED FIXED
: crash, regression
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- critical (vote)
: Thunderbird 22.0
Assigned To: Magnus Melin
:
:
Mentors:
Depends on:
Blocks: 481824 842963
  Show dependency treegraph
 
Reported: 2010-12-02 12:03 PST by Wayne Mery (:wsmwk, NI for questions)
Modified: 2013-09-07 08:21 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
22+
fixed


Attachments
proposed fix (2.44 KB, patch)
2013-02-21 03:45 PST, Magnus Melin
irving: review+
standard8: approval‑comm‑esr17+
Details | Diff | Splinter Review

Description Wayne Mery (:wsmwk, NI for questions) 2010-12-02 12:03:13 PST
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, NI for questions) 2011-03-29 03:15:57 PDT
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 2011-03-31 02:22:11 PDT
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, NI for questions) 2012-12-28 05:52:47 PST
(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 2013-02-21 03:45:48 PST
Created attachment 716468 [details] [diff] [review]
proposed fix

Ensure GetChildHdrAt returns proper rv, and also check the rv after calling it.
Comment 5 :Irving Reid (No longer working on Firefox) 2013-02-26 11:19:45 PST
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 2013-02-26 11:51:17 PST
(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] 2013-03-01 06:31:10 PST
Can this crash fix land in nightlies, aurora and/or betas of thunderbird?
Comment 8 :Irving Reid (No longer working on Firefox) 2013-03-01 06:44:10 PST
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 2013-03-04 11:50:40 PST
http://hg.mozilla.org/comm-central/rev/82aa521103e2 -> FIXED
Comment 10 :aceman 2013-03-25 01:20:00 PDT
Mihai, can you test if this patch fixes your problem here and in bug 842963?
Comment 11 :aceman 2013-03-25 01:22:05 PDT
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, limited time in Dec) 2013-03-28 04:54:12 PDT
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] 2013-03-28 12:39:47 PDT
(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, limited time in Dec) 2013-06-19 02:43:59 PDT
https://hg.mozilla.org/releases/comm-esr17/rev/db4c8f00540a
Comment 15 Wayne Mery (:wsmwk, NI for questions) 2013-09-07 08:21:07 PDT
gone in version 17.0.7

Note You need to log in before you can comment on or make changes to this bug.