Closed Bug 1264673 Opened 5 years ago Closed 4 years ago

Startup crash at nsMsgMaildirStore::MoveNewlyDownloadedMessage due to null newHdr

Categories

(MailNews Core :: Backend, defect)

defect
Not set
critical

Tracking

(thunderbird_esr5258+ fixed, thunderbird58 fixed, thunderbird59 fixed)

VERIFIED FIXED
Thunderbird 59.0
Tracking Status
thunderbird_esr52 58+ fixed
thunderbird58 --- fixed
thunderbird59 --- fixed

People

(Reporter: rkent, Assigned: jorgk-bmo)

References

(Blocks 1 open bug)

Details

(Keywords: crash, Whiteboard: [startupcrash][TB 52.6 ESR])

Crash Data

Attachments

(1 file)

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.
Severity: normal → critical
Crash Signature: nsMsgMaildirStore::MoveNewlyDownloadedMessage
~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
Crash Signature: nsMsgMaildirStore::MoveNewlyDownloadedMessage → [@ nsMsgMaildirStore::MoveNewlyDownloadedMessage]
Keywords: crash
Summary: crash at nsMsgMaildirStore::MoveNewlyDownloadedMessage due to null newHdr → Startup crash at nsMsgMaildirStore::MoveNewlyDownloadedMessage due to null newHdr
Whiteboard: [startupcrash]
#1 maildir crash.  A current example bp-af5eef63-30d5-4f9a-bce1-bd5f00170810

per comment 0, easy to fix?
Flags: needinfo?(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.
Flags: needinfo?(m_kato)
So I think we should add error path for nullptr after calling ThrowAlertMsg.
(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?
Flags: needinfo?(jorgk)
(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.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Flags: needinfo?(jorgk)
Attachment #8938761 - Flags: review?(m_kato)
Attachment #8938761 - Flags: review?(m_kato) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f9c68d3f6054
don't crash when no header was returned. r=m_kato
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 59.0
Duplicate of this bug: 1368267
Comment on attachment 8938761 [details] [diff] [review]
1264673-fix-crash.patch (v1)

Let's uplift this.
Attachment #8938761 - Flags: approval-comm-esr52?
Attachment #8938761 - Flags: approval-comm-beta+
Attachment #8938761 - Flags: approval-comm-esr52? → approval-comm-esr52+
See Also: → 529253
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
Status: RESOLVED → VERIFIED
See Also: → 1457409
You need to log in before you can comment on or make changes to this bug.