1264673 - Startup crash at nsMsgMaildirStore::MoveNewlyDownloadedMessage due to null newHdr

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect)

Description

[Kent James (:rkent)]

This crash is appearing in Thunderbird 45.0b4 but I doubt it is unique to that.

In http://mxr.mozilla.org/comm-esr45/source/mailnews/local/src/nsMsgMaildirStore.cpp a newHdr is created. If null, an error rv value is set. But that rv value never results in an error return, so later a null newHdr can be used resulting in the crash.

Comment 1

[Wayne Mery (:wsmwk)]

~84% are within one minute of startup
https://crash-stats.mozilla.com/signature/?date=%3E2015-10-01&product=Thunderbird&signature=nsMsgMaildirStore%3A%3AMoveNewlyDownloadedMessage&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&page=1#summary

bp-72f146cf-0247-470b-be2d-2c49f2151019
0 	xul.dll	nsMsgMaildirStore::MoveNewlyDownloadedMessage(nsIMsgDBHdr*, nsIMsgFolder*, bool*)	c:/builds/moz2_slave/tb-rel-c-esr38-w32_bld-0000000/build/mailnews/local/src/nsMsgMaildirStore.cpp:831
1 	xul.dll	nsParseNewMailState::ApplyFilterHit(nsIMsgFilter*, nsIMsgWindow*, bool*)	c:/builds/moz2_slave/tb-rel-c-esr38-w32_bld-0000000/build/mailnews/local/src/nsParseMailbox.cpp:2064
2 	xul.dll	nsMsgFilterList::ApplyFiltersToHdr(int, nsIMsgDBHdr*, nsIMsgFolder*, nsIMsgDatabase*, char const*, unsigned int, nsIMsgFilterHitNotify*, nsIMsgWindow*)	c:/builds/moz2_slave/tb-rel-c-esr38-w32_bld-0000000/build/mailnews/base/search/src/nsMsgFilterList.cpp:317
3 	xul.dll	nsParseNewMailState::ApplyFilters(bool*, nsIMsgWindow*, unsigned int)	c:/builds/moz2_slave/tb-rel-c-esr38-w32_bld-0000000/build/mailnews/local/src/nsParseMailbox.cpp:1951
4 	xul.dll	nsParseNewMailState::PublishMsgHeader(nsIMsgWindow*)	c:/builds/moz2_slave/tb-rel-c-esr38-w32_bld-0000000/build/mailnews/local/src/nsParseMailbox.cpp:1867 

another major signature is https://crash-stats.mozilla.com/signature/?date=%3E2015-10-01&product=Thunderbird&signature=nsMsgMaildirStore%3A%3AGetNewMsgOutputStream

Comment 2

[Wayne Mery (:wsmwk)]

#1 maildir crash.  A current example bp-af5eef63-30d5-4f9a-bce1-bd5f00170810

per comment 0, easy to fix?

Comment 3

[Makoto Kato [:m_kato]]

I believe that newHdr of newHdr->GetFlags is nullptr.

destMailDB->CopyHdrFromExistingHdr returns error, but we have no right error path for nullptr check.  (although ThrowAlertMsg is called...)

I don't know why CopyHdrFromExistingHdr returns error.

Comment 4

[Makoto Kato [:m_kato]]

So I think we should add error path for nullptr after calling ThrowAlertMsg.

Comment 5

[Wayne Mery (:wsmwk)]

(In reply to Makoto Kato [:m_kato] (slow due to PTO, back at Jan) from comment #4)
> So I think we should add error path for nullptr after calling ThrowAlertMsg.

jorg, do you agree?

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1264673-fix-crash.patch (v1)

(In reply to Wayne Mery (:wsmwk) from comment #5)
> jorg, do you agree?
Yes, although I don't understand this clumsy code. It sets rv and never acts on it and later overwrites it. Anyway, not worth a crash, so let's exit here.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f9c68d3f6054
don't crash when no header was returned. r=m_kato

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 8938761 [details] [diff] [review]
1264673-fix-crash.patch (v1)

Let's uplift this.

Comment 10

[Jorg K (CEST = GMT+2)]

Beta (TB 58):
https://hg.mozilla.org/releases/comm-beta/rev/c0c02dfd78b4

Comment 11

[Jorg K (CEST = GMT+2)]

TB 52.6 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/6448f59565a2174f0096bba7d0e88e96b68b5d70

Comment 12

[Wayne Mery (:wsmwk)]

Confirmed gone in 52.6.0. And may have fixed some other crash sigs, for example 
- Bug 791149 - startup crash in [@ nsParseMailMessageState::FinalizeHeaders
- Bug 529253 - startup crash [@ nsJARChannel::OnStartRequest(nsIRequest*, nsISupports*) ] because nntp is violating AsyncOpe