991626 - Thunderbird crash in NS_CycleCollectorSuspect3

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Wayne Mery (:wsmwk)]

(crash predates TB27 so not a regression of bug 867755) 

Crash dates to early TB25. Earliest I find is bp-8200fe16-afee-4497-80a5-bf6392130823 25.0a2 buildid 20130806004000. Definitely occurs in TB28 beta.  eg bp-005877fa-c690-483d-9e84-96f3a2140318. Unfortunately crash-stats is currently in turmoil, so I can't say for sure if today it is a topcrash nor whether it has gotten worse or better with more recent builds. But iirc when I looked a few weeks ago when socorro was working it was a topcrash for Thunderbird development builds

This bug was filed from the Socorro interface and is 
report bp-29d58fa1-9793-4bdf-82a5-e0f842130813 TB25

0	xul.dll	NS_CycleCollectorSuspect3	xpcom/base/nsCycleCollector.cpp
1	xul.dll	nsTransactionManager::Release()	editor/txmgr/src/nsTransactionManager.cpp
2	xul.dll	nsCOMPtr_base::assign_with_AddRef(nsISupports *)	objdir-tb/mozilla/xpcom/build/nsCOMPtr.cpp
3	xul.dll	nsMsgWindow::CloseWindow()	mailnews/base/src/nsMsgWindow.cpp
4	xul.dll	`anonymous namespace'::SyncRunnable2<nsIImapServerSink,nsIImapUrl *,nsIImapMockChannel *>::`vector deleting destructor'(unsigned int)	
5	xul.dll	nsMsgWindow::~nsMsgWindow()	mailnews/base/src/nsMsgWindow.cpp
6	xul.dll	nsMsgWindow::`vector deleting destructor'(unsigned int)	
7	xul.dll	nsImapProtocol::ProcessAfterAuthenticated()	mailnews/imap/src/nsImapProtocol.cpp
8	xul.dll	nsMsgWindow::Release()	mailnews/base/src/nsMsgWindow.cpp
9	xul.dll	nsImapProtocol::TryToLogon()	mailnews/imap/src/nsImapProtocol.cpp
10	xul.dll	nsImapServerResponseParser::ParseIMAPServerResponse(char const *,bool,char *)	mailnews/imap/src/nsImapServerResponseParser.cpp
11	mozglue.dll	arena_dalloc_small	memory/mozjemalloc/jemalloc.c
12	xul.dll	nsImapProtocol::Capability()	mailnews/imap/src/nsImapProtocol.cpp 

khuey@135526 3239 // We should have started the cycle collector by now.
khuey@135526 3240 MOZ_ASSERT(data);
khuey@135526 3241
khuey@135526 3242 if (!data->mCollector) {

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Null pointer crash.

But the argh thing here is that threadsafe nsMsgWindow has a strong pointer to
main thread only transactionmanager.
Do we end up deleting nsMsgWindow in non-main thread? That would lead us to try to release
mTransactionManager in that thread too, and since there is no cycle collector there
(because it is not main thread nor DOM worker thread) sCollectorData.get(); returns null and
we crash.

Comment 2

[Tracy Walker [:tracy]]

This has risen to #7 top crasher on Fx30.  It is affecting desktop and mobile.  -> core

Comment 3

[Andrew McCreight [:mccr8]]

The transaction manager needs to proxy its release to the main thread.  Here are some examples:
  http://mxr.mozilla.org/mozilla-central/ident?i=NS_ProxyRelease

Comment 4

[Andrew McCreight [:mccr8]]

Please file new bugs for non-Thunderbird crashes with this signature, as they are likely to be completely unrelated.  In any event, the Firefox crash increase mentioned in comment 2 should be fixed now.

Comment 5

[Mark Banner (:standard8)]

(In reply to Olli Pettay [:smaug] from comment #1)
> Null pointer crash.
> 
> But the argh thing here is that threadsafe nsMsgWindow has a strong pointer
> to
> main thread only transactionmanager.
> Do we end up deleting nsMsgWindow in non-main thread? That would lead us to
> try to release

We certainly grab a reference to it on a non-main thread:

http://hg.mozilla.org/comm-central/annotate/bc77857518f2/mailnews/imap/src/nsImapProtocol.cpp#l8327

Comment 6

[Mark Banner (:standard8)]

(In reply to Mark Banner (:standard8) from comment #5)
> (In reply to Olli Pettay [:smaug] from comment #1)
> > Null pointer crash.
> > 
> > But the argh thing here is that threadsafe nsMsgWindow has a strong pointer
> > to
> > main thread only transactionmanager.
> > Do we end up deleting nsMsgWindow in non-main thread? That would lead us to
> > try to release
> 
> We certainly grab a reference to it on a non-main thread:
> 
> http://hg.mozilla.org/comm-central/annotate/bc77857518f2/mailnews/imap/src/
> nsImapProtocol.cpp#l8327

I wonder if we could make that a weak reference or something.

Comment 7

[Ludovic Hirlimann [:Usul]]

Adding the proper neil Standard8 wanted to add :-)

Comment 8

[[e-mail]]

Attachment: Ensure nsMsgWindow is released on the main thread

TryToLogon and two other methods retrieve the msgWindow from the IMAP thread. In one case this is just to see whether the msgWindow exists, and in the other two it gets passed to a proxy back to the main thread. This means that no methods of msgWindow actually get invoked on the IMAP thread except for Release. This patch makes the Release happen on the main thread too. (This might make it possible to remove the threadsafe annotation from nsMsgWindow but I don't know how to tell.)

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8408072 [details] [diff] [review]
Ensure nsMsgWindow is released on the main thread

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

Looks good. r=Standard8

Comment 10

[[e-mail]]

Pushed comm-central changeset 707b47552a07.

Unfortunately I quoted the attachment number instead of the bug number...