Closed Bug 991626 Opened 8 years ago Closed 8 years ago

Thunderbird crash in NS_CycleCollectorSuspect3

Categories

(MailNews Core :: Networking: IMAP, defect)

All
Windows NT
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 31.0

People

(Reporter: wsmwk, Assigned: neil)

References

Details

(Keywords: crash, topcrash, topcrash-thunderbird, Whiteboard: [regression:TB25.0a2?])

Crash Data

Attachments

(1 file)

(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) {
Depends on: 990676
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.
This has risen to #7 top crasher on Fx30.  It is affecting desktop and mobile.  -> core
Component: General → JavaScript: GC
Keywords: topcrash
Product: Thunderbird → Core
See Also: → 993918
Keywords: topcrash-win
Whiteboard: [regression:??] → [regression:TB25.0a2?]
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
Component: JavaScript: GC → XPCOM
Component: XPCOM → General
Product: Core → Thunderbird
Summary: crash in NS_CycleCollectorSuspect3 → Thunderbird crash in NS_CycleCollectorSuspect3
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.
(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
(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.
Component: General → Networking: IMAP
Product: Thunderbird → MailNews Core
Adding the proper neil Standard8 wanted to add :-)
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.)
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #8408072 - Flags: review?(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
Attachment #8408072 - Flags: review?(standard8) → review+
Pushed comm-central changeset 707b47552a07.

Unfortunately I quoted the attachment number instead of the bug number...
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 31.0
See Also: → 1132339
Blocks: 1133892
You need to log in before you can comment on or make changes to this bug.