1005336 - startup or shutdown crash in nsObserverService::RemoveObserver(nsIObserver*, char const*) via nsMsgIncomingServer. reference to nsImapIncomingServer released off the main thread in destructors, due to preferences no longer use threadsafe refcounting

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Wayne Mery (:wsmwk)]

Currently, this is #1 crashes for TB31. averaging 1 per day.

bp-3d9cd808-8e72-4a68-98ac-592c22140416 (below) is the oldest build that I have found so far, and is the most common stack of this crash signature.
=============================================================
0	xul.dll	nsObserverService::RemoveObserver(nsIObserver *,char const *)	xpcom/ds/nsObserverService.cpp
1	xul.dll	xul.dll@0x6dd0c	
2	xul.dll	nsPrefBranch::~nsPrefBranch()	modules/libpref/src/nsPrefBranch.cpp
3	mozglue.dll	arena_dalloc_small	memory/mozjemalloc/jemalloc.c
4	xul.dll	nsPrefBranch::`vector deleting destructor'(unsigned int)	
5	xul.dll	nsPrefBranch::Release()	modules/libpref/src/nsPrefBranch.cpp
6	xul.dll	nsMsgIncomingServer::~nsMsgIncomingServer()	mailnews/base/util/nsMsgIncomingServer.cpp
7	xul.dll	nsImapIncomingServer::`vector deleting destructor'(unsigned int)	
8	xul.dll	nsMsgIncomingServer::Release()	mailnews/base/util/nsMsgIncomingServer.cpp
9	xul.dll	nsOfflineCacheBinding::Release()	mailnews/imap/src/nsSyncRunnableHelpers.cpp
10	xul.dll	nsImapProtocol::ProcessCurrentURL()	mailnews/imap/src/nsImapProtocol.cpp 

A few others like this:
bp-bc9e4198-e2a0-465f-a5c3-b6fc82140424
bp-8f37559b-98f3-4e28-9fa8-0922a2140426
bp-47c473ab-42c3-4d11-8f48-73eb42140429
bp-92b7e793-2b24-4bee-8f2c-d2dec2140429
bp-8778b0e8-f0fa-4555-bb78-3b0b12140430

Comment 1

[Wayne Mery (:wsmwk)]

TB32 - #1 crash for nightly/daily 
TB31 - is now #2 (#1 is now intel_aes_gcmENC bug 1005988)

Comment 2

[Wayne Mery (:wsmwk)]

regression?  
or addon/user environment?

earliest buildid 20140418030203 // bp-3d9cd808-8e72-4a68-98ac-592c22140416

(Firefox has this signature, but only about 10% are startup)

Comment 3

[Wayne Mery (:wsmwk)]

#1 crash for TB31 beta.
correction to comment 2 ... I think build 20140416030203 crash bp-3d9cd808-8e72-4a68-98ac-592c22140416 is the earliest. tb31a1 crash frequency high enough (about 1 per day) that I suspect the checkin causing this occurred within 48 hours of the build

From reporter of bp-77e428e3-2d2a-4e95-8301-9cc3c2140613 "My probs starts with the migration of Thunderbird from Thunderbird v30.00 Beta 1 Build 2 to Thunderbird v31.00 Beta 1 Build 1".  So I don't have any steps from crash reporters, and I suspect we won't be getting any.

Comment 4

[Wayne Mery (:wsmwk)]

tatze, you've crashed a few times, mostly startup
bp-9c781958-fe09-4022-83a3-d4c962140613
(...a month of no crashes...)
bp-f80b6012-633e-4515-a894-d66092140512
bp-5b6dafed-cd3e-4694-ae68-9e71a2140512
bp-9100edae-bc85-4b3d-a8a1-b5ee32140504
bp-348b0036-8a33-4edb-b807-0304b2140503

Do you have any suspicions what is happening in your evironment?

Comment 5

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

These might not all be the same crash; the direct cause is that the last reference to an nsImapIncomingServer is being released off the main thread, which is causing a chain of object destructors to run. Some of the calls in those destructors need to run on the main thread, including the one in nsObserverService that is forcing the crash.

I've only looked at a couple of the crashes, but they don't all show the release coming from the same place, which means they might not all be the same bug. That said, it's also possible that there's a single underlying bug causing the main thread to *not* hold a reference that it's supposed to hold.

Comment 6

[Wayne Mery (:wsmwk)]

#2 crash now that TB31.0 shipped

Comment 7

[Makoto Kato [:m_kato]]

Irving, can we use nsMainThreadPtrHandle / nsMainThreadPtrHolder ?

Comment 8

[Makoto Kato [:m_kato]]

Attachment: Proposal fix

Comment 9

[Ludovic Hirlimann [:Usul]]

Comment on attachment 8461330 [details] [diff] [review]
Proposal fix

setting a review flag to get irving's attention.

Comment 10

[Wayne Mery (:wsmwk)]

Is this a regression of Bug 991626 - Thunderbird crash in NS_CycleCollectorSuspect3 ?

Comment 11

[[e-mail]]

(In reply to Wayne Mery from comment #10)
> Is this a regression of Bug 991626 - Thunderbird crash in
> NS_CycleCollectorSuspect3 ?

No, that was caused by the imap thread trying to release the msgWindow object instead of proxying it to the main thread. This is the imap thread releasing the last (!) ref to an incoming server causing it to release its non-threadsafe members (in particular, preferences no longer use threadsafe refcounting).

Comment 12

[Wayne Mery (:wsmwk)]

(In reply to [e-mail] from comment #11)
>  This is the imap thread
> releasing the last (!) ref to an incoming server causing it to release its
> non-threadsafe members (in particular, preferences no longer use threadsafe
> refcounting).

does this suggest which bug# we can block?

Comment 13

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

Comment on attachment 8461330 [details] [diff] [review]
Proposal fix

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

I can't think of any reason why it would be wise for an object with thread-safe ref counting to hold references to objects with non-thread-safe ref counts. I'm not sure if releasing the last ref to the incoming server on the imap thread is a bug or not, but it certainly seems odd.

If we're going to fix this by proxying a destructor back on to the main thread, I *think* the right place to switch back to the main thread is on the ImapServerSinkProxy, so that when nsImapProtocol releases the reference the whole stack gets destroyed in the right place.

I wonder how many things would break if we changed nsMsgIncomingServer (and perhaps all the other proxied objects) to use main-thread-only reference counting, to enforce the destruction happening on the right thread.

Which is all to say that I'm tentatively OK with this patch, but I'd want all kinds of super-review.

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8461330 [details] [diff] [review]
Proposal fix

Unfortunately I'm not really going to get to spend the time to look at this that this deserves over the next few days. Hence seeing if Neil can take a look.

Comment 15

[[e-mail]]

Comment on attachment 8461330 [details] [diff] [review]
Proposal fix

These main thread pointer handle holders are overkill. Can't you just proxy the release of the receiver to the main thread in the destructor?

Alternatively, make an nsMainThreadPtr template that's like nsMainThreadPtrHolder but a) doesn't do the refcounting and b) has operator->() and use that as the type of the receiver member.

Comment 16

[Wayne Mery (:wsmwk)]

I'm not sure to whom this is directed, so I'm picking both ofyou

(In reply to [e-mail] from comment #15)
> Comment on attachment 8461330 [details] [diff] [review]
> Proposal fix
> 
> These main thread pointer handle holders are overkill. Can't you just proxy
> the release of the receiver to the main thread in the destructor?
> 
> Alternatively, make an nsMainThreadPtr template that's like
> nsMainThreadPtrHolder but a) doesn't do the refcounting and b) has
> operator->() and use that as the type of the receiver member.

Linux signature is nsObserverService::RemoveObserver

Comment 17

[Wayne Mery (:wsmwk)]

shutdown, windows   nsObserverService::RemoveObserver(nsIObserver*, char const*)
bp-dde15dba-c2c7-4ab7-8812-c5f3b2140730
startup, linux, same user as above   nsObserverService::RemoveObserver 
bp-50168d86-2cb9-421e-8aa4-fa1682140827

Comment 18

[[e-mail]]

Attachment: Proposed patch

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 8484510 [details] [diff] [review]
Proposed patch

[Triage Comment]

[Triage Comment]

[Triage Comment]

I'm happy for this to land on trunk whenever.  Also giving a+ for branchws.

Comment 20

[Wayne Mery (:wsmwk)]

(In reply to Wayne Mery (:wsmwk) from comment #12)
> (In reply to [e-mail] from comment #11)
> >  This is the imap thread
> > releasing the last (!) ref to an incoming server causing it to release its
> > non-threadsafe members (in particular, preferences no longer use threadsafe
> > refcounting).
> 
> does this suggest which bug# we can block?

irving, I believe we can blame bug 993734 - The observer service should not pretend to be threadsafe - which checked in 4014-04-15, the day before thunderbird crashes started

Murali had one of the earliest crashes bp-5e3cf8fd-9a27-42cc-96ae-62a592140424 April 24

Comment 21

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

Wayne, yes, that's likely the change that turned our previous behaviour into crashes; what I'm not sure about is whether our previous behaviour was correct, or whether we were just getting away with it because the previous thread-unsafe condition very rarely caused errors.

In any case, we're better off with this patch than we were before.

Comment 22

[Mark Banner (:standard8)]

https://hg.mozilla.org/comm-central/rev/a8b23b49d30f

Comment 23

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-aurora/rev/c11434d84ca8
https://hg.mozilla.org/releases/comm-beta/rev/d9d2ce4eab0a

Comment 24

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-esr31/rev/b99752bc69b2