Last Comment Bug 837643 - crash in nsMsgDatabase::ClearHdrCache
: crash in nsMsgDatabase::ClearHdrCache
Status: RESOLVED FIXED
[regression:TB17?]
: crash, regression
Product: MailNews Core
Classification: Components
Component: Database (show other bugs)
: Trunk
: All All
: -- critical (vote)
: Thunderbird 23.0
Assigned To: Magnus Melin
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-04 05:50 PST by Wayne Mery (:wsmwk, NI for questions)
Modified: 2013-04-03 12:18 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed fix (1.10 KB, patch)
2013-03-31 02:43 PDT, Magnus Melin
irving: review+
Details | Diff | Splinter Review

Description Wayne Mery (:wsmwk, NI for questions) 2013-02-04 05:50:54 PST
This bug was filed from the Socorro interface and is 
report bp-888d8ea1-aff2-4afb-a247-f44b22130202 .
============================================================= 

0		@0xba6adc7	
1	xul.dll	nsMsgDatabase::ClearHdrCache	mailnews/db/msgdb/src/nsMsgDatabase.cpp:608
2	xul.dll	nsMsgDatabase::AddHdrToCache	mailnews/db/msgdb/src/nsMsgDatabase.cpp:465
3	xul.dll	nsMsgDatabase::CreateMsgHdr	mailnews/db/msgdb/src/nsMsgDatabase.cpp:788
4	xul.dll	nsMsgDBEnumerator::PrefetchNext	mailnews/db/msgdb/src/nsMsgDatabase.cpp:2834
5	xul.dll	nsMsgDBEnumerator::HasMoreElements	mailnews/db/msgdb/src/nsMsgDatabase.cpp:2860
6	xul.dll	nsMsgDatabase::InitRefHash	mailnews/db/msgdb/src/nsMsgDatabase.cpp:4245
7	xul.dll	nsMsgDatabase::GetRefFromHash	mailnews/db/msgdb/src/nsMsgDatabase.cpp:4113
8	xul.dll	nsMsgDatabase::GetThreadForMessageId	mailnews/db/msgdb/src/nsMsgDatabase.cpp:4431
9	xul.dll	nsMsgDatabase::ThreadNewHdr	mailnews/db/msgdb/src/nsMsgDatabase.cpp:4521
10	xul.dll	nsMsgDatabase::AddNewHdrToDB	mailnews/db/msgdb/src/nsMsgDatabase.cpp:3400
11	xul.dll	nsImapMailDatabase::AddNewHdrToDB	mailnews/db/msgdb/src/nsImapMailDatabase.cpp:104 

crashes last line of 
mwu@9538
601nsresult nsMsgDatabase::ClearHdrCache(bool reInit)
hg@0
602{
hg@0
603 if (m_cachedHeaders)
hg@0
604 {
hg@0
605 // save this away in case we renter this code.
hg@0
606 PLDHashTable *saveCachedHeaders = m_cachedHeaders;
mconley@13475
607 m_cachedHeaders = nullptr;
mconley@13475
608 PL_DHashTableEnumerate(saveCachedHeaders, HeaderEnumerator, nullptr); 

where those lines changed with mconley's checking with Bug 776630 - Switch comm-central from using nsnull to nullptr.  Prior to TB17 this crash sig was rare.

OTOH, slightly different stack with signature 0x0 | nsMsgDatabase::ClearHdrCache(bool) is not so rare in TB16
example - bp-63c6d147-d3ea-4aef-8b3f-d9ea52121203

ditto PL_DHashTableEnumerate | nsMsgDatabase::ClearHdrCache(bool) 
bp-3a10ed8b-b377-44df-b1be-ae9612130131
Comment 1 Wayne Mery (:wsmwk, NI for questions) 2013-02-04 05:58:00 PST
one more story similar to DHashTableEnumerate | nsMsgDatabase::ClearHdrCache(bool) -- well established in TB16

nsMsgDatabase::HeaderEnumerator(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*) 
bp-854efc23-9a80-47e5-88c0-0b5f82130204
0	xul.dll	nsMsgDatabase::HeaderEnumerator	mailnews/db/msgdb/src/nsMsgDatabase.cpp:486
1	xul.dll	PL_DHashTableEnumerate	objdir-tb/mozilla/xpcom/build/pldhash.cpp:715
2	xul.dll	nsMsgDatabase::ClearHdrCache	mailnews/db/msgdb/src/nsMsgDatabase.cpp:608
3	xul.dll	nsMsgDatabase::AddHdrToCache	mailnews/db/msgdb/src/nsMsgDatabase.cpp:465
4	xul.dll	nsMsgDatabase::CreateMsgHdr	mailnews/db/msgdb/src/nsMsgDatabase.cpp:788
5	xul.dll	nsMsgDatabase::GetMsgHdrForGMMsgID	mailnews/db/msgdb/src/nsMsgDatabase.cpp:4629
6	xul.dll	nsImapMailFolder::GetOfflineMsgFolder	mailnews/imap/src/nsImapMailFolder.cpp:9672
7	xul.dll	nsImapMailFolder::HasMsgOffline	mailnews/imap/src/nsImapMailFolder.cpp:9577
8	xul.dll	nsImapProtocol::TryToRunUrlLocally	mailnews/imap/src/nsImapProtocol.cpp:2026 

perhaps there is more than one bug.
and perhaps this is related to an even older crash?  bug 625850?
Comment 2 Magnus Melin 2013-03-31 02:43:04 PDT
Created attachment 731621 [details] [diff] [review]
proposed fix

ClearHeaderEnumerator a few lines down also protects against null.
Comment 3 :Irving Reid (No longer working on Firefox) 2013-04-02 10:31:35 PDT
Comment on attachment 731621 [details] [diff] [review]
proposed fix

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

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +484,5 @@
>  {
>  
>    MsgHdrHashElement* element = reinterpret_cast<MsgHdrHashElement*>(hdr);
> +  if (element)
> +    NS_IF_RELEASE(element->mHdr);

This only gets called from http://mxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/nsMsgDatabase.cpp#603; any idea why m_cachedHeaders would contain a null entry?

That said, the fix looks safe to me.
Comment 4 Magnus Melin 2013-04-02 11:53:25 PDT
(In reply to :Irving Reid from comment #3)
> nsMsgDatabase.cpp#603; any idea why m_cachedHeaders would contain a null
> entry?

No idea, sorry.
Comment 5 Magnus Melin 2013-04-03 12:18:46 PDT
http://hg.mozilla.org/comm-central/rev/125f9c2a7a67 -> FIXED

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