84351 - Crash on exiting messenger in filter code - Trunk [@ nsMsgFilterList::~nsMsgFilterList]

Status: VERIFIED FIXED

(MailNews Core :: Filters, defect)

Description

[Scott MacGregor]

My release builds have been crashing recently on exit sometimes. 

Here's the stack trace from talkback. We are dereferencing a null ptr somewhere
in here. 

nsMsgFilterList::~nsMsgFilterList
[d:\builds\seamonkey\mozilla\mailnews\base\search\src\nsMsgFilterList.cpp, line 700]
nsMsgFilterList::`scalar deleting destructor'
nsMsgFilterList::Release
[d:\builds\seamonkey\mozilla\mailnews\base\search\src\nsMsgFilterList.cpp, line 64]
nsCOMPtr_base::~nsCOMPtr_base
[d:\builds\seamonkey\mozilla\xpcom\base\nsCOMPtr.cpp, line 50]
nsImapIncomingServer::~nsImapIncomingServer
[d:\builds\seamonkey\mozilla\mailnews\imap\src\nsImapIncomingServer.cpp, line 119]
nsImapIncomingServer::`scalar deleting destructor'
nsMsgIncomingServer::Release
[d:\builds\seamonkey\mozilla\mailnews\base\util\nsMsgIncomingServer.cpp, line 99]
nsImapIncomingServer::Release
[d:\builds\seamonkey\mozilla\mailnews\imap\src\nsImapIncomingServer.cpp, line 86]
nsMsgAccountManager::~nsMsgAccountManager
[d:\builds\seamonkey\mozilla\mailnews\base\src\nsMsgAccountManager.cpp, line 156]

Comment 1

[Scott MacGregor]

Here's an example incident ID: 31367793

Comment 2

[Navin Gupta]

I will look into it. 

Comment 3

[Navin Gupta]

moving to 0.9.2

Comment 4

[Navin Gupta]

Attachment: proposed fix

Comment 5

[Scott MacGregor]

hey naving, doesn't this mean that mfilterstream isn't going to get deleted anymore?

Comment 6

[Navin Gupta]

m_fileStream just points to nsIOFileStream objects allocated by callers. The 
callers have the obligation of closing and deleting stream, which they do 
in this case. cc bienvenu for review. 

Comment 7

[David :Bienvenu]

No, I think this introduces a leak of the file stream.
nsMsgFilterService::OpenFilterList() does not delete the file stream it creates,
although it does close it. Perhaps we should just use comptrs here - this code
was written before comptrs existed.

Comment 8

[laurel]

Remember to give some clues for verifying the fix.

Comment 9

[Navin Gupta]

I was just talking to mscott and will investigate more. However, I was thinking
if any massive changes would be allowed at this point. 

Comment 10

[David :Bienvenu]

I think it's about 5 lines of changes to use comptrs. Plus, people like seeing
comptrs :-)

Comment 11

[Scott MacGregor]

I've noticing a lot of talkback reports with this stack trace now. adding top
crash keyword. Hoping PDT won't push it out past the release.

Comment 12

[Jay Patel [:jay]]

Yes, this is definitely a topcrasher.  Here is the latest info from Talkback:

nsMsgFilterList::~nsMsgFilterList   37 
     First BBID :31250544
     Last BBID  :31380775
     Min Runtime :80
     Max Runtime :28009
     First Appearance Date : 2001-06-03
     Last Appearance Date : 2001-06-06
     First BuildID : 2001053023
     Last BuildID : 2001060509

Stack Trace: 

	 nsMsgFilterList::~nsMsgFilterList
[d:\builds\seamonkey\mozilla\mailnews\base\search\src\nsMsgFilterList.cpp  line 700]
	 nsMsgFilterList::`scalar deleting destructor'
	 nsMsgFilterList::Release
[d:\builds\seamonkey\mozilla\mailnews\base\search\src\nsMsgFilterList.cpp  line 64]
	 nsCOMPtr_base::~nsCOMPtr_base
[d:\builds\seamonkey\mozilla\xpcom\base\nsCOMPtr.cpp  line 50]
	 0x02409010
	 nsPop3IncomingServer::~nsPop3IncomingServer
[d:\builds\seamonkey\mozilla\mailnews\local\src\nsPop3IncomingServer.cpp  line 97]
	 nsPop3IncomingServer::`scalar deleting destructor'
	 nsMsgIncomingServer::Release
[d:\builds\seamonkey\mozilla\mailnews\base\util\nsMsgIncomingServer.cpp  line 99]
	 nsMsgAccountManager::hashElementRelease
[d:\builds\seamonkey\mozilla\mailnews\base\src\nsMsgAccountManager.cpp  line 1058]
	 PL_HashTableEnumerateEntries
[../../../lib/ds/plhash.c  line 414]
	 nsHashtable::Reset
[d:\builds\seamonkey\mozilla\xpcom\ds\nsHashtable.cpp  line 389]
 
 	Source File :
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/base/search/src/nsMsgFilterList.cpp
line : 700
     (31372283)	URL: google.netscape.com
     (31369860)	URL: google.netscape.com
     (31367376)	Comments: Closed Mozilla
     (31366994)	Comments: closing program
     (31366446)	Comments: I switch builds and now it's MSGBASE.DLL (I wonder if bugs really
do crawl around in code)
     (31365760)	URL: google.netscape.com
     (31364884)	URL: google.netscape.com
     (31364378)	Comments: closing program
     (31351202)	Comments: I selected "File->Exit".  That was enough to crash it  [:-(] 
(31350914)
Comments: crash on exit
     (31349891)	Comments: exiting N6 because it was failing to respond to email
     (31347456)	Comments: I'm nuking the install and starting over... I can't do anything
while I'm doing mail
     (31346345)	Comments: Crash upon closing
     (31346325)	URL: http://www.esd.gov.hk/chi/jumppage/auto_config.htm
(31343229)
Comments: shutting down mozilla
     (31333107)	Comments: Mozilla was in the process of closing down.  I had right clicked
on the install icon so that I could remove it from my desktop.
     (31309466)	Comments: quitting
     (31307616)	Comments: crash on exit
     (31286263)	Comments: Closing program

Comment 13

[Navin Gupta]

Looking at the first appearance date; this is a regression due to bug 49565

Comment 14

[Navin Gupta]

It is not crashing on 0.9.1 branch. I think hwaara just checked it on the trunk.
If it is urgent we can back out that fix.  

Comment 15

[Håkan Waara]

Whoa whoa, no one notified me on this.

Looking...

Comment 16

[Håkan Waara]

Bienvenu: Unfortunately, nsIOFileStream doesn't have any any .idl file, which
also means that it doesn't support such. Right?

Comment 17

[David :Bienvenu]

No, you're right, sorry, can't use comptrs - you'll have to fix it the old
fashioned way.

Comment 18

[David :Bienvenu]

Hwaara, the reason this is crashing is that the memory leak fix that caused this
regression deletes the tmp file stream, and the filter list itself also deletes
the file stream, in its destructor. You can't delete a pointer twice.

I think that backing out the memory leak fix is probably the right thing to do -
the root cause of the problem seems to me that the filter list itself is not
getting freed in all situations. Previously, when it didn't get freed, it also
caused a leak of the file stream. After the memory leak fix, when the filter
list does get freed, we get a double delete and crash. I think it would be
better to find the root cause of the filter list leak.

Alternatively, we could do what Navin suggested, but we'd have to add code to
nsMsgFilterService::OpenFilterList to delete that stream too. 

Comment 19

[Håkan Waara]

So in theory it all comes down to a huge leak where nsImapFolders on shutdown,
and therefore also leaking all members which include the nsMsgFilterList.

Will attach a patch which operates a delete on the filestream right after
close() in OpenFilterList() combined with naving's fix.

Comment 20

[Håkan Waara]

Attachment: proposed fix 2

Comment 21

[Håkan Waara]

r=, sr=?

Comment 22

[Navin Gupta]

I think it would be safer to pass the stream as parameter where ever needed
rather than making it a member variable (m_fileStream).

I was trying to use nsIFileSpec instead of nsIOFileStream but that didn't work 
for some reason. 
 

Comment 23

[Navin Gupta]

Attachment: proposed fix

Comment 24

[Navin Gupta]

Pass nsIOFileStream as parameter. need r & sr

Comment 25

[Scott MacGregor]

I like this solution a lot naving. r=mscott

Comment 26

[David :Bienvenu]

yes, I do too sr=bienvenu

Comment 27

[Navin Gupta]

chofmann, I need your approval

Comment 28

[scottputterman]

adding PDT+

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

a=dbaron for trunk checkin (on behalf of drivers)

Comment 30

[Navin Gupta]

fix checked in

Comment 31

[Scott MacGregor]

*** Bug 84248 has been marked as a duplicate of this bug. ***

Comment 32

[laurel]

Mscott: fixed for you?
Naving:  Any way to cause/verify this? Reproducibility hints?

Comment 33

[Navin Gupta]

It was crashing consistently on debug build. probably scott can verify it. 

Comment 34

[Scott MacGregor]

laurel, the best way to verify this is by looking at the talk back reports. I'm
no longer seeing this showing up on talkback reports for this stack trace since
the fix was checked in. It was a top crasher before

Another way to reproduce may have been to modify or create a new filter then
quit the app. But I'm not sure if that triggered the crash or not. 

Comment 35

[laurel]

Marking verified.
Can't reproduce with june21 commercial trunk builds: win98, linux rh6.2 and mac
OS 9.0