Startup crash at nsMsgMaildirStore::MoveNewlyDownloadedMessage due to null newHdr

VERIFIED FIXED in Thunderbird 59.0

Status

defect
--
critical
VERIFIED FIXED
3 years ago
Last year

People

(Reporter: rkent, Assigned: jorgk)

Tracking

(Blocks 1 bug, {crash})

Trunk
Thunderbird 59.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird_esr5258+ fixed, thunderbird58 fixed, thunderbird59 fixed)

Details

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

Attachments

(1 attachment)

Reporter

Description

3 years ago
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.
Reporter

Updated

3 years ago
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)
Assignee

Comment 6

2 years ago
(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+

Comment 7

2 years ago
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: 2 years ago
Resolution: --- → FIXED
Assignee

Updated

2 years ago
Target Milestone: --- → Thunderbird 59.0
Assignee

Updated

2 years ago
Duplicate of this bug: 1368267
Assignee

Comment 9

2 years ago
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+
Assignee

Updated

Last year
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.