313357 - SeaMonkey crashes on quit (MailNews-related) [@ nsMsgSearchBoolExpression::~nsMsgSearchBoolExpression()]

Status: VERIFIED FIXED

(MailNews Core :: Search, defect)

Description

[Frank Wein [:mcsmurf]]

Lately SeaMonkey sometimes crashes on quit, probably this is related to MailNews.

Stacktrace:
nsMsgSearchBoolExpression::~nsMsgSearchBoolExpression() line 129 + 3 bytes
nsMsgSearchBoolExpression::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsMsgSearchBoolExpression::~nsMsgSearchBoolExpression() line 130 + 30 bytes
nsMsgSearchBoolExpression::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsMsgSearchSession::DestroyTermList() line 651 + 31 bytes
nsMsgSearchSession::~nsMsgSearchSession() line 76
nsMsgSearchSession::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsMsgSearchSession::Release(nsMsgSearchSession * const 0x05bddd38) line 56 + 212
bytes
nsCOMPtr<nsIMsgSearchSession>::~nsCOMPtr<nsIMsgSearchSession>() line 584
VirtualFolderChangeListener::~VirtualFolderChangeListener() line 2596 + 27 bytes
VirtualFolderChangeListener::`scalar deleting destructor'(unsigned int 1) + 15 bytes
VirtualFolderChangeListener::Release(VirtualFolderChangeListener * const
0x05bd63f8) line 2610 + 212 bytes
nsCOMArray_base::~nsCOMArray_base() line 61 + 18 bytes
nsCOMArray<nsIDBChangeListener>::~nsCOMArray<nsIDBChangeListener>() line 149 +
16 bytes
nsMsgAccountManager::~nsMsgAccountManager() line 188 + 140 bytes
nsMsgAccountManager::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsMsgAccountManager::Release(nsMsgAccountManager * const 0x04844550) line 156 +
150 bytes
nsCOMPtr_base::assign_assuming_AddRef(nsISupports * 0x00000000) line 532
nsCOMPtr_base::assign_with_AddRef(nsISupports * 0x00000000) line 90
nsCOMPtr<nsISupports>::operator=(nsISupports * 0x00000000) line 1034
FreeServiceContractIDEntryEnumerate(PLDHashTable * 0x0026b604, PLDHashEntryHdr *
0x00eef300, unsigned int 643, void * 0x00000000) line 2029
PL_DHashTableEnumerate(PLDHashTable * 0x0026b604, int (PLDHashTable *,
PLDHashEntryHdr *, unsigned int, void *)* 0x10067170
FreeServiceContractIDEntryEnumerate(PLDHashTable *, PLDHashEntryHdr *, unsigned
int, void *), void * 0x00000000) line 621 + 34 bytes
nsComponentManagerImpl::FreeServices() line 2041 + 19 bytes
NS_ShutdownXPCOM_P(nsIServiceManager * 0x00000000) line 825
NS_ShutdownXPCOM(nsIServiceManager * 0x00000000) line 171 + 10 bytes
GRE_Shutdown() line 515 + 7 bytes
main(int 1, char * * 0x00262638) line 1745 + 5 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e98989()

This /could/ be a fall-out from Bug 295088, not sure so.

Comment 1

[Frank Wein [:mcsmurf]]

Another stacktrace when crashing on quit is just a stacktrace full with
"nsMsgSearchBoolExpression::~nsMsgSearchBoolExpression" (about 1000 frames,
didn't count ;-) and it finally crashes in "delete m_leftChild;".

Comment 2

[David :Bienvenu]

is this your own debug build? I saw this right after the first checkin for bug 295088, but after the second checkin, I haven't seen it.

Comment 3

[Howard Chu]

(In reply to comment #1)
> Another stacktrace when crashing on quit is just a stacktrace full with
> "nsMsgSearchBoolExpression::~nsMsgSearchBoolExpression" (about 1000 frames,
> didn't count ;-) and it finally crashes in "delete m_leftChild;".

I haven't got any virtual folders setup. Does this happen in any other circumstances, like opening fresh, perform one search, exit? Some actual steps to reproduce would be good.

Comment 4

[Frank Wein [:mcsmurf]]

@David: First checkin, second checkin? I only saw one checkin for this or do you mean the checkin which turned the tree red and was fixed shortly after that with a second checkin? So yes, i use my own build for this, i see this with trunk opt and debug and i compiled after the bustage fix was in (before it would not even compile :).

@Howard: Yes, i have one virtual folder set up, but i did not use it in that session where it crashes. Getting some steps to reproduce is quite hard, but i'll see if i can get something together.

Comment 5

[David :Bienvenu]

By the second checkin, I meant the build bustage fix. On Windows, it built OK without the second checkin, but crashed on exit. With the second checkin, the crash stopped. Have you tried doing a make clean, make in mailnews/base?

Comment 6

[Frank Wein [:mcsmurf]]

Yes it still occours: I removed my obj-dir, checked out from CVS again just to make sure and then built again. Then I again crashed on exit. As said, i cannot reproduce this every time, it just occours when using MailNews for a somewhat longer time and then quitting SeaMonkey.

Comment 7

[David :Bienvenu]

It looks to me like we delete the expression tree in a bunch of places w/o setting it to null. So, for example, if we call AddSearchTerm but then never call MatchHdr, we will crash from a double free when we destory the search session. Seems like the easy fix is to null out m_expressionTree whereever we delete it.

Comment 8

[David :Bienvenu]

Attachment: proposed fix

null out expression tree after deleting it. There may be cases where it's not required, butbetter safe than sorry :-) Also, don't need to check for null before deleting since delete will check.

Comment 9

[David :Bienvenu]

Attachment: better fix, I think

DestroyTermList is only called from the nsMsgSearchSession destructor, so it doesn't need to delete m_expressionTree.

Comment 10

[Howard Chu]

(In reply to comment #9)
> Created an attachment (id=200622) [edit]
> better fix, I think
> 
> DestroyTermList is only called from the nsMsgSearchSession destructor, so it
> doesn't need to delete m_expressionTree.
> 
It looks like there are two places in nsMsgFilter.cpp that need a patch too, nsMsgFilter::AppendTerm and ::SetSearchTerms. You agree?

The current patch looks fine and compiles and runs ok for me, but I never saw the original problem.

Comment 11

[Scott MacGregor]

Comment on attachment 200622 [details] [diff] [review]
better fix, I think

I don't understand this:

         delete m_expressionTree;
+        m_expressionTree = nsnull;

deleting an object clears the pointer for us, doesn't it?

Comment 12

[Karsten Düsterloh]

>          delete m_expressionTree;
> +        m_expressionTree = nsnull;
> 
> deleting an object clears the pointer for us, doesn't it?

No, why should it? (Unless it's overloaded - but <http://lxr.mozilla.org/mozilla/ident?i=delete> doesn't suggest at first galnce that this would be the case here.)
But usually PR_DELETE does that for us in NSPR.

Comment 13

[Scott MacGregor]

Comment on attachment 200622 [details] [diff] [review]
better fix, I think

i was confused.

Comment 14

[Stephen Donner [:stephend] Not actively reading bugmail]

According to http://talkback-public.mozilla.org, MozillaOrgThunderbirdTrunkWin322005102606 was the last build to crash with this, which makes sense given that the fix would be in the 2005-10-27 nightlies.

Verified (tested on SeaMonkey 1.1a

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051101 Mozilla/1.0), too.

Comment 15

[David :Bienvenu]

Comment on attachment 200622 [details] [diff] [review]
better fix, I think

clearing obsolete request

Comment 16

[David :Bienvenu]

bug 295088 fix was checked into 1.8.1 branch, so I checked this one in too.

Comment 17

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.3 using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.3) Gecko/20070326 Thunderbird/2.0.0.0 Mnenhy/0.7.5.0 ID:2007032620 - no crash on quit -> adding verified keyword