Closed Bug 617945 Opened 9 years ago Closed 4 years ago

crash [@ nsImapMailFolder::DeleteMessages(nsIArray*, nsIMsgWindow*, int, int, nsIMsgCopyServiceListener*, int)]

Categories

(MailNews Core :: Networking: IMAP, defect, critical)

x86
All
defect
Not set
critical

Tracking

(thunderbird46 fixed, thunderbird47 fixed, thunderbird48 fixed, thunderbird_esr45 fixed)

RESOLVED FIXED
Thunderbird 48.0
Tracking Status
thunderbird46 --- fixed
thunderbird47 --- fixed
thunderbird48 --- fixed
thunderbird_esr45 --- fixed

People

(Reporter: wsmwk, Assigned: rkent)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

crash [@ nsImapMailFolder::DeleteMessages(nsIArray*, nsIMsgWindow*, int, int, nsIMsgCopyServiceListener*, int)]

bp-3774dee8-e6f0-4343-a314-67e322101129
EXCEPTION_ACCESS_VIOLATION_READ
0x0
0	thunderbird.exe	nsImapMailFolder::DeleteMessages	mailnews/imap/src/nsImapMailFolder.cpp:2380
1	thunderbird.exe	nsMsgDBView::DeleteMessages	mailnews/base/src/nsMsgDBView.cpp:3002
2	thunderbird.exe	nsMsgDBView::ApplyCommandToIndices	mailnews/base/src/nsMsgDBView.cpp:2726
3	thunderbird.exe	nsMsgDBView::DoCommand	mailnews/base/src/nsMsgDBView.cpp:2393
4	xpcom_core.dll	NS_InvokeByIndex_P	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
5	thunderbird.exe	XPCWrappedNative::CallMethod	js/src/xpconnect/src/xpcwrappednative.cpp:2722 


slightly didferent stack - bp-7cf2173d-e299-4675-a8c7-128bd2101201
OS: Windows Vista → All
Crash Signature: [@ nsImapMailFolder::DeleteMessages(nsIArray*, nsIMsgWindow*, int, int, nsIMsgCopyServiceListener*, int)]
bp-74ed0632-02d6-4797-9acb-efedd2111122 v8

this may phave morhed to nsImapMailFolder::DeleteMessages(nsIArray*, nsIMsgWindow*, bool, bool, nsIMsgCopyServiceListener*, bool) 
eg bp-a75f5c71-2db0-4d8e-bc8a-8e9182120329 v11
Crash Signature: [@ nsImapMailFolder::DeleteMessages(nsIArray*, nsIMsgWindow*, int, int, nsIMsgCopyServiceListener*, int)] → [@ nsImapMailFolder::DeleteMessages(nsIArray*, nsIMsgWindow*, int, int, nsIMsgCopyServiceListener*, int)] [@ nsImapMailFolder::DeleteMessages(nsIArray*, nsIMsgWindow*, bool, bool, nsIMsgCopyServiceListener*, bool) ]
two different stacks


bp-e9488400-7709-4746-a61e-dd9e72121225
0	xul.dll	nsImapMailFolder::DeleteMessages	mailnews/imap/src/nsImapMailFolder.cpp:2324
1	xul.dll	nsMsgDBView::DeleteMessages	mailnews/base/src/nsMsgDBView.cpp:3186
2	xul.dll	nsMsgDBView::ApplyCommandToIndices	mailnews/base/src/nsMsgDBView.cpp:2919
3	xul.dll	nsMsgDBView::DoCommand	mailnews/base/src/nsMsgDBView.cpp:2590
4	xul.dll	NS_InvokeByIndex_P	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:70
5	xul.dll	XPCWrappedNative::CallMethod	js/xpconnect/src/XPCWrappedNative.cpp:2406
6	xul.dll	XPC_WN_CallMethod	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1478
7	mozjs.dll	js::InvokeKernel	js/src/jsinterp.cpp:352 

bp-f764ceb6-a861-48e1-a858-17d412121204 
0	xul.dll	nsImapMailFolder::DeleteMessages	mailnews/imap/src/nsImapMailFolder.cpp:2324
1	xul.dll	nsMsgDatabase::ApplyRetentionSettings	mailnews/db/msgdb/src/nsMsgDatabase.cpp:5224
2	xul.dll	nsMsgDBFolder::ApplyRetentionSettings	mailnews/base/util/nsMsgDBFolder.cpp:4786
3	xul.dll	nsMsgDBFolder::ApplyRetentionSettings	mailnews/base/util/nsMsgDBFolder.cpp:4762
4	xul.dll	nsImapMailFolder::ApplyRetentionSettings	mailnews/imap/src/nsImapMailFolder.cpp:1281
5	xul.dll	nsImapMailFolder::Compact	mailnews/imap/src/nsImapMailFolder.cpp:1296
6	xul.dll	hashCleanupOnExit	mailnews/base/src/nsMsgAccountManager.cpp:1005
7	xul.dll	nsBaseHashtable<nsCStringHashKey,__int64,__int64>::s_EnumStub	objdir-tb/mozilla/dist/include/nsBaseHashtable.h:419
8	xul.dll	PL_DHashTableEnumerate	objdir-tb/mozilla/xpcom/build/pldhash.cpp:715
9	xul.dll	nsBaseHashtable<nsCStringHashKey,nsAutoPtr<mozilla::scache::CacheEntry>,mozilla:	objdir-tb/mozilla/dist/include/nsBaseHashtable.h:223
10	xul.dll	nsMsgAccountManager::CleanupOnExit	mailnews/base/src/nsMsgAccountManager.cpp:1624
mDatabase is nullptr, so mDatabase is gone after checking it.
I can only offer wild guess.

The routines invoked after |if (mDatabase)| may not change this directly, but
are there other threads that may be touching this |mDatabase| variable?
nsMsgDB*.cpp files seem to refer to this variable, and I wonder if there are any asynchronous access
to this variable.
In the second stack in comment 2, I see |nsImapMailFolder::Compact|, maybe |Compact| for IMAP may act
slightly differently from |Compact| for POP3, but |Compact| for POP3 does seem to act asynchronously
with the rest of POP3-style message operation. I have to wait for the lock (held by Compact) to be freed before I can touch a folder, etc. 

So maybe a protection on |mDatabase| variable is missing somewhere against such thread-race?

For example,
shouldn't the code starting from 
http://mxr.mozilla.org/comm-esr24/source/mailnews/imap/src/nsImapMailFolder.cpp#2264
and end with the usage of |mDatabase->...| several lines below
be within some type of mutex/semaphore/lock?

(And places that mucks with mDatabase shall properly honour such mutex/semaphore/lock?)

TIA
Duplicate of this bug: 953129
(In reply to Magnus Melin from comment #4)
> bp-87165c55-e94d-47ea-bae5-71e7b2131220 -
> http://hg.mozilla.org/releases/comm-esr24/annotate/a5e50e44b4c4/mailnews/
> imap/src/nsImapMailFolder.cpp#l2278 but the question is why mDatabase
> becomes null after it's checked

Yeah, compact could surely be one possible cause.
What do you think we can do out of comment 5?
Flags: needinfo?(rkent)
Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.
Crash Signature: [@ nsImapMailFolder::DeleteMessages(nsIArray*, nsIMsgWindow*, int, int, nsIMsgCopyServiceListener*, int)] [@ nsImapMailFolder::DeleteMessages(nsIArray*, nsIMsgWindow*, bool, bool, nsIMsgCopyServiceListener*, bool) ] → [@ nsImapMailFolder::DeleteMessages(nsIArray*, nsIMsgWindow*, int, int, nsIMsgCopyServiceListener*, int)] [@ nsImapMailFolder::DeleteMessages(nsIArray*, nsIMsgWindow*, bool, bool, nsIMsgCopyServiceListener*, bool) ] [@ nsImapMailFolder::DeleteMessages]
We've had many cases now of where mDatabase mysteriously disappears. A local reference prevents the crash until we discover the root cause.
Assignee: nobody → rkent
Status: NEW → ASSIGNED
Flags: needinfo?(rkent)
Attachment #8729707 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8729707 [details] [diff] [review]
Use a local reference for the db

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

LGTM! r=mkmelin
Attachment #8729707 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 8729707 [details] [diff] [review]
Use a local reference for the db

[Triage Comment]

[Triage Comment]

http://hg.mozilla.org/releases/comm-aurora/rev/b56c25ae4748
http://hg.mozilla.org/releases/comm-beta/rev/c45514a56b28
Attachment #8729707 - Flags: approval-comm-beta+
Attachment #8729707 - Flags: approval-comm-aurora+
Whiteboard: [checkin-needed comm-central]
Comment on attachment 8729707 [details] [diff] [review]
Use a local reference for the db

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky):
Attachment #8729707 - Flags: approval-comm-esr45?
Comment on attachment 8729707 [details] [diff] [review]
Use a local reference for the db

http://hg.mozilla.org/releases/comm-esr45/rev/47b22519b5a2
Attachment #8729707 - Flags: approval-comm-esr45? → approval-comm-esr45+
Keywords: checkin-needed
Whiteboard: [checkin-needed comm-central]
(In reply to Magnus Melin from comment #4)
> bp-87165c55-e94d-47ea-bae5-71e7b2131220 -
> http://hg.mozilla.org/releases/comm-esr24/annotate/a5e50e44b4c4/mailnews/
> imap/src/nsImapMailFolder.cpp#l2278 but the question is why mDatabase
> becomes null after it's checked

I found an interesting comment!

http://mxr.mozilla.org/comm-esr24/source/mailnews/imap/src/nsImapMailFolder.cpp#608

608     // UpdateNewMessages/UpdateSummaryTotals can null mDatabase, so we save a local copy
609     nsCOMPtr<nsIMsgDatabase> database(mDatabase);
610     UpdateNewMessages();
611     if(mAddListener)
612       database->AddListener(this);
613     UpdateSummaryTotals(true);
614     mDatabase = database;

This means that we may want to save mDatabase and reuse it
when the NULL reference happened!?
http://hg.mozilla.org/releases/comm-esr24/diff/e0aafab0f5da/mailnews/imap/src/nsImapMailFolder.cpp#l649

This is more clear about the issue.
We need to change the code ala the patch above, at least!
https://hg.mozilla.org/comm-central/rev/eb9570adfe51
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 48.0
You need to log in before you can comment on or make changes to this bug.