617946 - crash [@ nsImapMailFolder::GetDatabase()]

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Wayne Mery (:wsmwk)]

crash [@ nsImapMailFolder::GetDatabase()]

bp-9d7c3ac3-0f6b-4e43-ad8d-891122101203
EXCEPTION_ACCESS_VIOLATION_READ
0x0
0	thunderbird.exe	nsImapMailFolder::GetDatabase	mailnews/imap/src/nsImapMailFolder.cpp:666
1	thunderbird.exe	nsMsgDBFolder::GetMsgDatabase	mailnews/base/util/nsMsgDBFolder.cpp:932
2	xpcom_core.dll	NS_InvokeByIndex_P	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
3	thunderbird.exe	XPCWrappedNative::CallMethod	js/src/xpconnect/src/xpcwrappednative.cpp:2722
4	thunderbird.exe	XPC_WN_GetterSetter	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1784
5	js3250.dll	js_Invoke	js/src/jsinterp.cpp:1360
6	js3250.dll	js_InternalInvoke	js/src/jsinterp.cpp:1423
7	js3250.dll	js_InternalGetOrSet	js/src/jsinterp.cpp:1486
8	js3250.dll	js_GetSprop	js/src/jsscope.h:602
9	js3250.dll	js_NativeGet	js/src/jsobj.cpp:4117
10	js3250.dll	js_GetPropertyHelper	js/src/jsobj.cpp:4275
11	js3250.dll	js_Interpret	js/src/jsops.cpp:1520
12	js3250.dll	js_Invoke	js/src/jsinterp.cpp:1368
13	js3250.dll	js_InternalInvoke	js/src/jsinterp.cpp:1423
14	js3250.dll	JS_CallFunctionValue	js/src/jsapi.cpp:5114
15	thunderbird.exe	nsJSContext::CallEventHandler	dom/base/nsJSEnvironment.cpp:2197
16	thunderbird.exe	nsJSEventListener::HandleEvent	dom/src/events/nsJSEventListener.cpp:269
17	thunderbird.exe	nsEventListenerManager::HandleEventSubType	content/events/src/nsEventListenerManager.cpp:1041
18	thunderbird.exe	nsEventListenerManager::HandleEvent	content/events/src/nsEventListenerManager.cpp:1147
19	thunderbird.exe	nsEventTargetChainItem::HandleEvent	content/events/src/nsEventDispatcher.cpp:246
20	thunderbird.exe	nsEventTargetChainItem::HandleEventTargetChain	content/events/src/nsEventDispatcher.cpp:310
21	thunderbird.exe	nsEventDispatcher::Dispatch	content/events/src/nsEventDispatcher.cpp:573
22	thunderbird.exe	DocumentViewerImpl::LoadComplete	layout/base/nsDocumentViewer.cpp:1036
23	thunderbird.exe	nsDocShell::EndPageLoad	docshell/base/nsDocShell.cpp:5714

Comment 1

[Vincent (caméléon)]

Also reported on Geckozone, see http://www.geckozone.org/forum/viewtopic.php?f=4&t=105350

Comment 2

[Ludovic Hirlimann [:Usul]]

This is a top changer for 16. Irving any idea what might be going here ?

Comment 3

[Wayne Mery (:wsmwk)]

bp-07b4d56e-c1fc-46ff-bace-7c8922121017 states "thunderbird needs very much memory (150.000MB) and often is in a state of NOT REACTING. right now it crashed when trying to compress"

0	xul.dll	nsImapMailFolder::GetDatabase	mailnews/imap/src/nsImapMailFolder.cpp:613
1	xul.dll	nsMsgDBFolder::GetMsgDatabase	mailnews/base/util/nsMsgDBFolder.cpp:920
2	xul.dll	NS_InvokeByIndex_P	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:70
3	xul.dll	XPCWrappedNative::CallMethod	js/xpconnect/src/XPCWrappedNative.cpp:2418

Comment 4

[Wayne Mery (:wsmwk)]

(In reply to Ludovic Hirlimann [:Usul] from comment #2)
> This is a top changer for 16. Irving any idea what might be going here ?

examined several TB17 crashes - here are examples of 4 *different* stacks, but all end at mailnews/imap/src/nsImapMailFolder.cpp:613


bp-805d27a9-e5b3-4c5d-b4ba-c92ed2121224
0	xul.dll	nsImapMailFolder::GetDatabase	mailnews/imap/src/nsImapMailFolder.cpp:613
1	xul.dll	nsImapMailFolder::UpdateFolderWithListener	mailnews/imap/src/nsImapMailFolder.cpp:773
2	xul.dll	nsImapMailFolder::UpdateFolder	mailnews/imap/src/nsImapMailFolder.cpp:622
3	xul.dll	nsImapMailFolder::OnNewIdleMessages	mailnews/imap/src/nsImapMailFolder.cpp:2654
4	xul.dll	`anonymous namespace'::SyncRunnable0<nsIImapMessageSink>::Run	mailnews/imap/src/nsSyncRunnableHelpers.cpp:92 


bp-01d6b11c-9c4f-4bea-9567-cbb612121221
0	xul.dll	nsImapMailFolder::GetDatabase	mailnews/imap/src/nsImapMailFolder.cpp:613
1	xul.dll	nsImapMailFolder::UpdateImapMailboxInfo	mailnews/imap/src/nsImapMailFolder.cpp:2666
2	xul.dll	`anonymous namespace'::SyncRunnable2<nsIImapMailFolderSink,nsIMsgMailNewsUrl*,ch	mailnews/imap/src/nsSyncRunnableHelpers.cpp:146
3	xul.dll	nsThread::ProcessNextEvent	xpcom/threads/nsThread.cpp:624
4	xul.dll	NS_ProcessNextEvent_P	objdir-tb/mozilla/xpcom/build/nsThreadUtils.cpp:220
5	xul.dll	mozilla::ipc::MessagePump::Run	ipc/glue/MessagePump.cpp:82
6	xul.dll	MessageLoop::RunHandler	ipc/chromium/src/base/message_loop.cc:201 


bp-cfffb6ec-3d2d-4768-b73e-8f86c2121221
0	xul.dll	nsImapMailFolder::GetDatabase	mailnews/imap/src/nsImapMailFolder.cpp:613
1	xul.dll	nsImapMailFolder::ApplyRetentionSettings	mailnews/imap/src/nsImapMailFolder.cpp:1246
2	xul.dll	nsMsgPurgeService::PerformPurge	mailnews/base/src/nsMsgPurgeService.cpp:215 


bp-6657e233-ba90-47e8-bb23-80edc2121224
xul.dll	nsImapMailFolder::GetDatabase	mailnews/imap/src/nsImapMailFolder.cpp:613
1	xul.dll	nsImapMailFolder::GetDBFolderInfoAndDB	mailnews/imap/src/nsImapMailFolder.cpp:2076
2	xul.dll	nsImapProtocol::SetupWithUrl	mailnews/imap/src/nsImapProtocol.cpp:657
3	xul.dll	nsImapProtocol::LoadImapUrl	mailnews/imap/src/nsImapProtocol.cpp:2090
4	xul.dll	nsImapIncomingServer::GetImapConnectionAndLoadUrl	mailnews/imap/src/nsImapIncomingServer.cpp:424
5	xul.dll	nsImapService::GetImapConnectionAndLoadUrl	mailnews/imap/src/nsImapService.cpp:2223
6	xul.dll	nsImapService::FolderCommand	mailnews/imap/src/nsImapService.cpp:1505
7	xul.dll	nsImapService::UpdateFolderStatus	mailnews/imap/src/nsImapService.cpp:1558
8	xul.dll	nsImapMailFolder::UpdateStatus	mailnews/imap/src/nsImapMailFolder.cpp:1408
9	xul.dll	nsImapMailFolder::InitiateAutoSync	mailnews/imap/src/nsImapMailFolder.cpp:9527

Comment 5

[:Irving Reid (No longer working on Firefox)]

I suspect this behaviour came in through change http://hg.mozilla.org/comm-central/rev/e0aafab0f5da, which was committed by Bienvenu with no bug reference, only the comment " timeless <[e-mail]> ".

That change introduces a local reference holder for the database, in case some of the sub-functions called release the reference held in a member variable, but *removes* a surrounding null check on mDatabase.

By my reading of the code, mDatabase should only be null at this point if either msgDBService->OpenFolderDB() or msgDBService->CreateNewDB() returns NS_OK and a null DB reference; I haven't read down into those methods yet to see how that happens.

Timeless, do you remember the change in question?

Comment 6

[Wayne Mery (:wsmwk)]

(In reply to Irving Reid (:irving) from comment #5)
> I suspect this behaviour came in through change
> http://hg.mozilla.org/comm-central/rev/e0aafab0f5da, which was committed by
> Bienvenu with no bug reference, only the comment " timeless
> <[e-mail]> ".

bienvenu, do you remember the change in question?

Comment 7

[Wayne Mery (:wsmwk)]

(In reply to Irving Reid (:irving) from comment #5)
> I suspect this behaviour came in through change
> http://hg.mozilla.org/comm-central/rev/e0aafab0f5da, which was committed by
> Bienvenu with no bug reference, only the comment " timeless
> <[e-mail]> ".

irving,
that commit for TB10 is from bug 642759.
whether it induced a regression in TB10, I can't tell via crash-stats

bp-fea40db8-86ae-4bf6-9e9e-d90b52130129 TB10 
bp-37ea2a92-d6b9-44f2-92d2-1f6f12130110 TB17 (account5)
bp-62ed486e-1361-4e3a-b728-06e822130125 (nick)

Comment 8

[Ludovic Hirlimann [:Usul]]

Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.

Comment 9

[Wayne Mery (:wsmwk)]

Many users crashing with nsImapMailFolder::GetDatabase (#100 crash for 38.6.0) also crash with nsMsgDBFolder::UpdateNewMessages (#117 crash for TB38.6.0) - which combined would make this a top 50 crash. 

Examples with the two signatures:
* claudia bp-bc228ce8-8396-4d8b-96cf-160032160317 bp-cca701ad-b32c-422b-8ea3-168af2160315
* bp-623a8621-22f6-4078-90fa-ab3132160329 bp-8a6b6acd-3cbb-4f7f-82c3-28af02160401
* salino bp-abf1f4ac-ed53-4054-ba51-ef2be2160222 bp-444f29ea-4e2c-438b-b9b5-ed1322160223

Comment 10

[Kent James (:rkent)]

The crash is here with null database:

    if (NS_FAILED(rv))
      rv = msgDBService->CreateNewDB(this, getter_AddRefs(mDatabase));

    NS_ENSURE_SUCCESS(rv, rv);

    // UpdateNewMessages/UpdateSummaryTotals can null mDatabase, so we save a local copy
    nsCOMPtr<nsIMsgDatabase> database(mDatabase);
    UpdateNewMessages();
    if(mAddListener)
      database->AddListener(this);

In CreateNewDB there is this check:

  rv = msgDatabase->Open(this, summaryFilePath, true, true);
  NS_ENSURE_TRUE(rv == NS_MSG_ERROR_FOLDER_SUMMARY_MISSING, rv);

if rv == NS_OK here, then it would return success without update that database, resulting in a null database.

This is an easy fix.

Comment 11

[:aceman]

It seems the NS_ENSURE_TRUE is used with inverted logic in this case (we want rv to be an error).
If this is the right fix, please add a comment about why we check those specific error codes there.

Comment 12

[Kent James (:rkent)]

Attachment: Always return an error if NS_ENSURE_TRUE fails

This is a fairly simple attempt to fix this.

Comment 13

[:aceman]

(In reply to :aceman from comment #11)
> It seems the NS_ENSURE_TRUE is used with inverted logic in this case (we
> want rv to be an error).
> If this is the right fix, please add a comment about why we check those
> specific error codes there.

Sorry, I was already commenting on your patch you pushed to try, before you even attached it here. Now you attached the patch unchanged. Can you add what I asked for?

Comment 14

[Wayne Mery (:wsmwk)]

(In reply to :aceman from comment #13)
> (In reply to :aceman from comment #11)
> > It seems the NS_ENSURE_TRUE is used with inverted logic in this case (we
> > want rv to be an error).
> > If this is the right fix, please add a comment about why we check those
> > specific error codes there.
> 
> Sorry, I was already commenting on your patch you pushed to try, before you
> even attached it here. Now you attached the patch unchanged. Can you add
> what I asked for?

I haven't checked into why, but this is now a topcrash - #11 for 45.0. So it would be good to get this for 45.1 or 45.2

Comment 15

[Wayne Mery (:wsmwk)]

also nsMsgDBFolder::UpdateNewMessages signature has risen from #126 in 38.x to #21 in 45.0

Several users who crash with nsImapMailFolder::GetDatabase also crash with nsMsgDBFolder::UpdateNewMessages. For example ...

User A. 
 nsImapMailFolder::GetDatabase bp-e849e344-4775-4899-b6f7-1d7672160329
 nsMsgDBFolder::UpdateNewMessages bp-623a8621-22f6-4078-90fa-ab3132160329
0 	xul.dll	nsMsgDBFolder::UpdateNewMessages()	c:/builds/moz2_slave/tb-rel-c-esr38-w32_bld-0000000/build/mailnews/base/util/nsMsgDBFolder.cpp:584
1 	xul.dll	nsImapMailFolder::GetDatabase()	c:/builds/moz2_slave/tb-rel-c-esr38-w32_bld-0000000/build/mailnews/imap/src/nsImapMailFolder.cpp:610
2 	xul.dll	nsMsgDBFolder::GetMsgDatabase(nsIMsgDatabase**)	c:/builds/moz2_slave/tb-rel-c-esr38-w32_bld-0000000/build/mailnews/base/util/nsMsgDBFolder.cpp:932
3 	xul.dll	nsMsgDBFolder::GetMessageHeader(unsigned int, nsIMsgDBHdr**)	c:/builds/moz2_slave/tb-rel-c-esr38-w32_bld-0000000/build/mailnews/base/util/nsMsgDBFolder.cpp:5138
4 	xul.dll	`anonymous namespace'::SyncRunnable3<nsIImapMessageSink, char const*, unsigned int, nsIImapUrl*>::Run()	c:/builds/moz2_slave/tb-rel-c-esr38-w32_bld-0000000/build/mailnews/imap/src/nsSyncRunnableHelpers.cpp:177 

User B claudia
 nsMsgDBFolder::UpdateNewMessages bp-ec32e110-44f2-49dc-ab83-a946d2160415
 nsImapMailFolder::GetDatabase bp-cca701ad-b32c-422b-8ea3-168af2160315

Should we wait to see the impact of this patch before investigating the nsMsgDBFolder::UpdateNewMessages signature? (looks like maybe we should)

Comment 16

[Kent James (:rkent)]

Attachment: Now with comments

Comment 17

[Kent James (:rkent)]

Attachment: Now with comments and fewer typos

Comment 18

[:aceman]

Comment on attachment 8743486 [details] [diff] [review]
Now with comments and fewer typos

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

Thanks, let's try it.

Comment 19

[Kent James (:rkent)]

http://hg.mozilla.org/comm-central/rev/bd1e2d7c70c5

Comment 20

[Kent James (:rkent)]

http://hg.mozilla.org/releases/comm-esr45/rev/5b07803d7e29

Comment 21

[Jorg K (CEST = GMT+2)]

Comment on attachment 8743486 [details] [diff] [review]
Now with comments and fewer typos

[Triage Comment]
This should be landed on Aurora TB 47 for the next 47 beta.

Comment 22

[Jorg K (CEST = GMT+2)]

Aurora (TB 47):
https://hg.mozilla.org/releases/comm-aurora/rev/820389b55d0d