1368267 - crash/segfault in nsMsgMaildirStore::MoveNewlyDownloadedMessage

Status: RESOLVED DUPLICATE of bug 1264673

(MailNews Core :: Backend, defect)

Description

[Robert Kiraly]

Attachment: thunderbird-oldcoder-newhdr-crash.patch

User Agent: Mozilla/5.0 (X11; Linux i686; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20100101

Steps to reproduce:

I built Thunderbird 52.1.1 and started it using POP3 email accounts. IMAP was not involved.

Note: mozconfig contents are probably not relevant. A specific problem has been identified in nsMsgMaildirStore.cpp and the purpose of this bug report is to submit a patch for review.



Actual results:

Thunderbird segfaulted repeatedly shortly after startup. I built multiple releases of the program and encountered the same issue.



Expected results:

Thunderbird should have downloaded email without segfaulting.

For the benefit of others, I've attached a patch that works for me. It's compatible with both 52.1.1 and 53.0b2. However, the patch may not be correct. Review of the patch is requested.

Comment 1

[Jorg K (CEST = GMT+2)]

Comment on attachment 8872076 [details] [diff] [review]
thunderbird-oldcoder-newhdr-crash.patch

I guess Kent knows best what's going on here.

To me, it doesn't look right, I think newHdr should be allowed to be null, but of course it shouldn't be dereferenced later.

Comment 2

[rsx11m]

Comment on attachment 8872076 [details] [diff] [review]
thunderbird-oldcoder-newhdr-crash.patch

>+  if (!newHdr) return NS_ERROR_FAILURE;
>+
>   if (NS_SUCCEEDED(rv) && !newHdr)
>     rv = NS_ERROR_UNEXPECTED;

Probably rv should be checked for NS_ERROR_UNEXPECTED then?
What happens if rv doesn't indicate success? ... etc.

Breaking with NS_ERROR_FAILURE indeed looks a bit crude.

Comment 3

[Kent James (:rkent)]

Comment on attachment 8872076 [details] [diff] [review]
thunderbird-oldcoder-newhdr-crash.patch

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

This method is used in a very narrow set of circumstances in messages moved by a filter. If newHdr is null, that move is going to fail. null newHdr is not expected (and in other contexts is an assert).

Although I'm not a fan of alerts to signal failure of a filter, that is the existing design so we should be consistent. That is, show the alert message if newHdr is null, then exit with a failure.

So I would prefer an alternate change a little later, like this:

  if (NS_FAILED(rv)) {
    aDestFolder->ThrowAlertMsg("filterFolderHdrAddFailed", nullptr);
    return NS_MSG_ERROR_WRITING_MAIL_FOLDER;
  }

Comment 4

[Wayne Mery (:wsmwk)]

Robert, can you submit a revised patch of the form https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Submitting_the_patch


example in the wild bp-b5ed30f3-b5ac-4abc-9fd1-d47510171230
 0 	xul.dll	nsMsgMaildirStore::MoveNewlyDownloadedMessage(nsIMsgDBHdr*, nsIMsgFolder*, bool*)	C:/builds/moz2_slave/tb-rel-c-esr52-w32_bld-0000000/build/mailnews/local/src/nsMsgMaildirStore.cpp:835
1 	xul.dll	nsParseNewMailState::ApplyFilterHit(nsIMsgFilter*, nsIMsgWindow*, bool*)	C:/builds/moz2_slave/tb-rel-c-esr52-w32_bld-0000000/build/mailnews/local/src/nsParseMailbox.cpp:2082
2 	xul.dll	nsMsgFilterList::ApplyFiltersToHdr(int, nsIMsgDBHdr*, nsIMsgFolder*, nsIMsgDatabase*, char const*, unsigned int, nsIMsgFilterHitNotify*, nsIMsgWindow*)	C:/builds/moz2_slave/tb-rel-c-esr52-w32_bld-0000000/build/mailnews/base/search/src/nsMsgFilterList.cpp:318
3 	xul.dll	nsParseNewMailState::ApplyFilters(bool*, nsIMsgWindow*, unsigned __int64)	C:/builds/moz2_slave/tb-rel-c-esr52-w32_bld-0000000/build/mailnews/local/src/nsParseMailbox.cpp:1962
4 	xul.dll	nsParseNewMailState::PublishMsgHeader(nsIMsgWindow*)	C:/builds/moz2_slave/tb-rel-c-esr52-w32_bld-0000000/build/mailnews/local/src/nsParseMailbox.cpp:1887
5 	xul.dll	nsPop3Sink::IncorporateComplete(nsIMsgWindow*, int)	C:/builds/moz2_slave/tb-rel-c-esr52-w32_bld-0000000/build/mailnews/local/src/nsPop3Sink.cpp:900
6 	xul.dll	nsPop3Protocol::HandleLine(char*, unsigned int)	C:/builds/moz2_slave/tb-rel-c-esr52-w32_bld-0000000/build/mailnews/local/src/nsPop3Protocol.cpp:3523
7 	xul.dll	nsPop3Protocol::RetrResponse(nsIInputStream*, unsigned int)	C:/builds/moz2_slave/tb-rel-c-esr52-w32_bld-0000000/build/mailnews/local/src/nsPop3Protocol.cpp:3308

Comment 5

[Jorg K (CEST = GMT+2)]

Should already be fixed by bug 1264673 here:
https://hg.mozilla.org/comm-central/rev/f9c68d3f6054c52681edc55e9b75b943ebd584b1#l1.12

Wayne can confirm whether the crash signature has gone away.

Comment 6

[Robert Kiraly]

I received email asking me to submit a patch in a revised format. However, it appears that this bug is now a duplicate that has been resolved. Is anything further needed?

Comment 7

[Jorg K (CEST = GMT+2)]

I believe it's fixed as per comment #5, so no further action required. Thank you.

Comment 8

[Wayne Mery (:wsmwk)]

(In reply to Jorg K (GMT+1) from comment #5)
> Should already be fixed by bug 1264673 here:
> https://hg.mozilla.org/comm-central/rev/
> f9c68d3f6054c52681edc55e9b75b943ebd584b1#l1.12
> 
> Wayne can confirm whether the crash signature has gone away.

That may take some days or weeks, because crash data prior to 12-24 has been purged. (ref bug 1427111)