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

VERIFIED FIXED in Thunderbird 22.0

Status

MailNews Core
Backend
--
critical
VERIFIED FIXED
7 years ago
4 years ago

People

(Reporter: wsmwk, Assigned: Magnus Melin)

Tracking

({crash, regression})

Trunk
Thunderbird 22.0
crash, regression
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird-esr1722+ fixed)

Details

(crash signature)

Attachments

(1 attachment)

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)
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)
OS: Windows Vista → All
Version: 1.9.2 Branch → Trunk

Comment 2

6 years ago
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
Assignee: nobody → bugmail
Blocks: 481824
Keywords: regression
Crash Signature: [@ nsMsgXFGroupThread::SetMsgHdrAt(unsigned int, nsIMsgDBHdr*)]
(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
Flags: needinfo?(m_kato)
Keywords: qawanted
(Assignee)

Comment 4

4 years ago
Created attachment 716468 [details] [diff] [review]
proposed fix

Ensure GetChildHdrAt returns proper rv, and also check the rv after calling it.
Assignee: bugmail → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #716468 - Flags: review?(irving)
Flags: needinfo?(m_kato)
(Reporter)

Updated

4 years ago
Blocks: 842963
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?
(Assignee)

Comment 6

4 years ago
(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.
Can this crash fix land in nightlies, aurora and/or betas of thunderbird?
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.
Attachment #716468 - Flags: review?(irving) → review+
(Assignee)

Comment 9

4 years ago
http://hg.mozilla.org/comm-central/rev/82aa521103e2 -> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: qawanted
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 22.0

Comment 10

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

Comment 11

4 years ago
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):
Attachment #716468 - Flags: approval-comm-esr17?
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.
tracking-thunderbird-esr17: --- → 22+
(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.
Attachment #716468 - Flags: approval-comm-esr17? → approval-comm-esr17+
https://hg.mozilla.org/releases/comm-esr17/rev/db4c8f00540a
status-thunderbird-esr17: --- → fixed
gone in version 17.0.7
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.