523114 - reindex crash [@ nsImapMailFolder::ParseAdoptedMsgLine(char const*, unsigned int, int, nsIImapUrl*)]

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Wayne Mery (:wsmwk)]

from crash-stats (not my crash)
reindex crash [@ nsImapMailFolder::ParseAdoptedMsgLine(char const*, unsigned int, int, nsIImapUrl*)]

top of stack is a topcrash for 3.0bpre - however, there may be at least two different crashes here based on cursory look at stacks, so we need to sort this out. marking blocking? just for the QA bit. I've dropped this into backend, rather than imap because reindex is mentioned in comments

nsImapMailFolder::ParseAdoptedMsgLine(char const*, unsigned int, int, nsIImapUrl*) is not new - goes back to at least 3.0b1

bp-eca62347-1ff4-4b92-bc03-bbb712091014
Rebuild index in imap inbox. I've had a message index problem today - one particular entry in the inbox list shows a completely different message when I click on the list item. I did two index rebuilds to try and correct it. The second one caused this crash.
Frame	Module	Signature [Expand]	Source
0	thunderbird-bin	nsImapMailFolder::ParseAdoptedMsgLine	mailnews/imap/src/nsImapMailFolder.cpp:4410
1	libxpcom_core.dylib	NS_InvokeByIndex_P	xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:179
2	libxpcom_core.dylib	nsProxyObjectCallInfo::Run	xpcom/proxy/src/nsProxyEvent.cpp:181
3	libxpcom_core.dylib	nsThread::ProcessNextEvent	xpcom/threads/nsThread.cpp:521
4	libxpcom_core.dylib	NS_ProcessNextEvent_P	nsThreadUtils.cpp:227
5	thunderbird-bin	nsXULWindow::ShowModal	
6	thunderbird-bin	nsWindowWatcher::OpenWindowJSInternal	embedding/components/windowwatcher/src/nsWindowWatcher.cpp:992
7	thunderbird-bin	nsWindowWatcher::OpenWindowJS	embedding/components/windowwatcher/src/nsWindowWatcher.cpp:487
8	thunderbird-bin	nsGlobalWindow::OpenInternal	dom/src/base/nsGlobalWindow.cpp:7364
9	thunderbird-bin	nsGlobalWindow::OpenDialog	dom/src/base/nsGlobalWindow.cpp:5157
10	libxpcom_core.dylib	NS_InvokeByIndex_P	xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:179
11	thunderbird-bin	XPCWrappedNative::CallMethod	js/src/xpconnect/src/xpcwrappednative.cpp:2456
12	thunderbird-bin	XPC_WN_CallMethod	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1590
13	libmozjs.dylib	js_Invoke	js/src/jsinterp.cpp:1386
14	libmozjs.dylib	js_Interpret	js/src/jsinterp.cpp:5179
15	libmozjs.dylib	js_Invoke	js/src/jsinterp.cpp:1394
16	libmozjs.dylib	js_InternalInvoke	js/src/jsinterp.cpp:1447
17	libmozjs.dylib	JS_CallFunctionValue	js/src/jsapi.cpp:5187
18	thunderbird-bin	nsJSContext::CallEventHandler	dom/src/base/nsJSEnvironment.cpp:2085
19	thunderbird-bin	nsJSEventListener::HandleEvent	dom/src/events/nsJSEventListener.cpp:247
20	thunderbird-bin	nsEventListenerManager::HandleEventSubType	content/events/src/nsEventListenerManager.cpp:1098
21	thunderbird-bin	nsEventListenerManager::HandleEvent	content/events/src/nsEventListenerManager.cpp:1206
22	thunderbird-bin	nsEventTargetChainItem::HandleEvent	content/events/src/nsEventDispatcher.cpp:236 


a different crash
bp-726aadf5-1201-43f2-a977-0d5b72090302
0	thunderbird.exe	nsMIMEHeaderParamImpl::GetParameterInternal	 netwerk/mime/src/nsMIMEHeaderParamImpl.cpp:210
1	thunderbird.exe	MimeHeaders_get_parameter	mailnews/mime/src/mimehdrs.cpp:492
2	thunderbird.exe	MimeMultipart_parse_line	mailnews/mime/src/mimemult.cpp:376
3	thunderbird.exe	convert_and_send_buffer	mailnews/mime/src/mimebuf.cpp:184
4	thunderbird.exe	mime_LineBuffer	mailnews/mime/src/mimebuf.cpp:272
5	thunderbird.exe	MimeObject_parse_buffer	mailnews/mime/src/mimeobj.cpp:275
6	thunderbird.exe	MimeMessage_parse_line	mailnews/mime/src/mimemsg.cpp:232
7	thunderbird.exe	convert_and_send_buffer	mailnews/mime/src/mimebuf.cpp:184
8	thunderbird.exe	mime_LineBuffer	mailnews/mime/src/mimebuf.cpp:272
9	thunderbird.exe	MimeObject_parse_buffer	mailnews/mime/src/mimeobj.cpp:275
10	thunderbird.exe	mime_display_stream_write	mailnews/mime/src/mimemoz2.cpp:946
11	thunderbird.exe	nsStreamConverter::OnDataAvailable	mailnews/mime/src/nsStreamConverter.cpp:938
12	thunderbird.exe	nsImapCacheStreamListener::OnDataAvailable	mailnews/imap/src/nsImapProtocol.cpp:8130
13	thunderbird.exe	nsInputStreamPump::OnStateTransfer	netwerk/base/src/nsInputStreamPump.cpp:508
14	thunderbird.exe	nsInputStreamPump::OnInputStreamReady	netwerk/base/src/nsInputStreamPump.cpp:398
15	xpcom_core.dll	nsOutputStreamReadyEvent::Run	xpcom/io/nsStreamUtils.cpp:111

Comment 1

[Kent James (:rkent)]

If this upstream code at http://hg.mozilla.org/comm-central/annotate/cce9484a2a61/mailnews/db/msgdb/src/nsMsgDatabase.cpp#l1672 :

  if (err == NS_OK && m_mdbStore /* && hasOid */)

had err == NS_OK but null m_mdbStore, then in the code involved in the crash:

4407   if (!m_offlineHeader)
4408   {
4409     rv = GetMessageHeader(uidOfMessage, getter_AddRefs(m_offlineHeader));
4410     NS_ENSURE_SUCCESS(rv, rv);
4411     rv = StartNewOfflineMessage();
4412     m_offlineHeader->SetMessageSize(aMsgSize);
4413   }

(crash occurs at line 4412) the NS_ENSURE_SUCCESS would succeed, but with null m_offlineHeader. The defensive fix would be to add a null check for m_offlineHeader as well.

I'll do a patch and let bienvenu think about it.

Comment 2

[David :Bienvenu]

What about making GetMessgeHeader return an error if m_mdbStore is null?

Comment 3

[Kent James (:rkent)]

(In reply to comment #2)
> What about making GetMessgeHeader return an error if m_mdbStore is null?

I could go either way. Your proposal is a better fix, but a riskier fix, in that it a) assumes my analysis of the upstream source is correct, and b) assumes that no existing users of the code rely on the existing behaviour.

Comment 4

[Kent James (:rkent)]

Attachment: Rev a: add check in both folder and database code

This is a conservative patch, that adds the check both in the place upstream where we think the error starts, and also near the crash. I could be persuaded to eliminate one of these checks, though I don't see the harm in doing both.

Comment 5

[David :Bienvenu]

Comment on attachment 407116 [details] [diff] [review]
Rev a: add check in both folder and database code

I'd move the m_mdbStore check up here:

  if (!pmsgHdr || !m_mdbAllMsgHeadersTable)
    return NS_ERROR_NULL_POINTER;

any header we return from the use cache w/ a null mdbstore is going to be dangerous.

I had actually suggested changing GetMessageHeader, which is at a bit higher level, and thus potentially less risky. But I'm OK with changing GetMsgHdrForKey if you move the m_mdbStore check higher up.

Comment 6

[Kent James (:rkent)]

Attachment: Rev b: move check earlier in GetMsgHdrForKey

Comment 7

[David :Bienvenu]

Comment on attachment 407300 [details] [diff] [review]
Rev b: move check earlier in GetMsgHdrForKey

thx, Kent.

Comment 8

[David :Bienvenu]

fix checked in.