141028 - M1RC1 topcrash [@ nsMsgDatabase::NotifyAnnouncerGoingAway]

Status: VERIFIED FIXED

(MailNews Core :: Database, defect, P1)

Description

[Jan Carpenter]

This stack signature is a topcrasher for M1RC1

Product ID  Mozilla1.0
Build ID 2002041717
Platform Win32
Operating System Windows NT 5.0 build 2195
Module msgdb.dll
URL visited http://www.ibm.com
Trigger Reason Access violation
Source File Name d:\builds\seamonkey\mozilla\mailnews\db\msgdb\src\nsMsgDatabase.cpp
Trigger Line No. 568
 	Source File :
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/db/msgdb/src/nsMsgDatabase.cpp
line : 568

     (5692095)	URL: http://www.ibm.com
     (5670105)	Comments: Exited program
     (5657775)	URL: www.apple.com/trailers
     (5653440)	URL: www.apple.com/trailers
     (5646885)	Comments: Sent email  closed down client  selected Shut Down from Start menu
     (5617112)	URL: www.itc.org.uk
     (5617112)	Comments: Closed Mozilla after reading email.
     (5566056)	Comments: Closing mailer
     (5560656)	Comments: closing Mozilla
     (5522762)	Comments: shuting down
     (5516294)	URL: www.cinemaxx.de
     (5509868)	Comments: I closed all the mozilla browser/mail windows leaving the prefs
window open and then  when closing the prefs window (with cancel button) mozilla
crashed.
     (5432040)	Comments: Closing Messenger window (exiting application)
     (5411238)	URL: www.cinemaxx.de
     (5396472)	Comments: Set up new Yahoo! POP3 account (add to 5 IMAP and one other POP3
account)  set up Yahoo! SMTP server  whole program hangs after sending email
(though sent successfully)  then quit mail with Task Manager  started it again
from browser  hang when trying to
     (5396472)	Comments:  download from Yahoo! POP3 server.


Stack Trace
nsMsgDatabase::NotifyAnnouncerGoingAway
[d:\builds\seamonkey\mozilla\mailnews\db\msgdb\src\nsMsgDatabase.cpp, line 568]
nsMsgDatabase::ForceClosed
[d:\builds\seamonkey\mozilla\mailnews\db\msgdb\src\nsMsgDatabase.cpp, line 1057]
nsMailDatabase::ForceClosed
[d:\builds\seamonkey\mozilla\mailnews\db\msgdb\src\nsMailDatabase.cpp, line 195]
nsMsgDatabase::CleanupCache
[d:\builds\seamonkey\mozilla\mailnews\db\msgdb\src\nsMsgDatabase.cpp, line 608]
nsGenericModule::Shutdown
[d:\builds\seamonkey\mozilla\xpcom\glue\nsGenericFactory.cpp, line 325]
nsGenericModule::Release
[d:\builds\seamonkey\mozilla\xpcom\glue\nsGenericFactory.cpp, line 236]
nsDll::Shutdown [d:\builds\seamonkey\mozilla\xpcom\components\xcDll.cpp, line 489]
nsFreeLibrary
[d:\builds\seamonkey\mozilla\xpcom\components\nsNativeComponentLoader.cpp, line 386]
nsFreeLibraryEnum
[d:\builds\seamonkey\mozilla\xpcom\components\nsNativeComponentLoader.cpp, line 435]
_hashEnumerate [d:\builds\seamonkey\mozilla\xpcom\ds\nsHashtable.cpp, line 199]
PL_HashTableEnumerateEntries [../../../lib/ds/plhash.c, line 430]
nsHashtable::Enumerate [d:\builds\seamonkey\mozilla\xpcom\ds\nsHashtable.cpp,
line 362]
nsNativeComponentLoader::UnloadAll
[d:\builds\seamonkey\mozilla\xpcom\components\nsNativeComponentLoader.cpp, line
1018]
nsComponentManagerImpl::UnloadLibraries
[d:\builds\seamonkey\mozilla\xpcom\components\nsComponentManager.cpp, line 2978]
nsComponentManagerImpl::Shutdown
[d:\builds\seamonkey\mozilla\xpcom\components\nsComponentManager.cpp, line 795]
NS_ShutdownXPCOM [d:\builds\seamonkey\mozilla\xpcom\build\nsXPComInit.cpp, line 584]
main [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1774]
WinMain [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1784]
WinMainCRTStartup()
KERNEL32.DLL + 0xd326 (0x77e8d326)

Comment 1

[David :Bienvenu]

This has been occuring from 2002-04-19 to 2002-04-28 (presumably, all 10 days
the data is available for) so it doesn't look like a recent regression. I can't
find any reports on the trunk, at least through quick search...I'm not sure how
to reproduce this, but I'll try.

Comment 2

[David :Bienvenu]

I take it back about not being able to find trunk reports of this crash - I
tried an exact match search thinking it would be faster but it turned up no
hits. I then tried a "begins with" search for
nsMsgDatabase::NotifyAnnouncerGoingAway and turned up some trunk hits.

Comment 3

[David :Bienvenu]

What's likely going on here is that a listener to the db has been deleted but
not unregistered from the db, so when we shutdown, we attempt to notify the
listener, but the pointer is invalid. So I need to figure out what potential db
listeners do not unregister themselves.

Comment 4

[David :Bienvenu]

Although I can't reproduce this, I suspect this happens when we call ForceClosed
on a DB (e.g, compacting a folder or emptying the trash), which does not remove
the listeners, but forces the db closed. I'll try to come up with a fix.

Comment 5

[Jay Patel [:jay]]

Adding qawanted keyword to see if we can get this reproduced in-house.

Comment 6

[David :Bienvenu]

I've found reproducible steps for this:

1. start up mail
2. bring up new compose window and start composing message.
3. Save as draft (I have an imap folder as my drafts folder - not sure if it
matters if drafts imap or local)
4. File | Exit from the 3 pane mail window (without closing the compose window
first)

Comment 7

[greer]

Since David is able to reproduce this one, replacing qawanted with testcase. 
That also qualifies it as topcrash+. And I'll nominate nsbeta1 for good measure.

Comment 8

[David :Bienvenu]

Attachment: proposed fix

the basic problem was that in some cases listeners were adding themselves as
listeners twice with the db, but, of course, only removing themselves once. So
the fix is to check if a listener is already registered before adding it to the
change listeners array. I could have made it an error to register yourself
twice, but it's harmless now, and some code might not be able to tell if it's
already added itself as a listener.
The code path that was causing this particular crash was calling
nsImapMailFolder::UpdateImapMailboxInfo without mDatabase being set (e.g.,
saving a draft w/o having opened the draft folder). It would call
GetMsgDatabase(), which adds the folder as a listener. Then,
UpdateImapMailboxInfo would add the folder as a listener again. I'll attach a
patch to remove this unneeded second listener add, but the first fix is more
general and will catch any other code adding itself as a listener twice.

Comment 9

[David :Bienvenu]

can I get an r=, Navin? thx.

Comment 10

[David :Bienvenu]

Attachment: remove unneeded call to AddListener

this patch just removes the unneeded call to AddListener, as a general cleanup
thing.

Comment 11

[(not reading, please use [e-mail] instead)]

Comment on attachment 81897 [details] [diff] [review]
remove unneeded call to AddListener

sr=sspitzer

Comment 12

[Navin Gupta]

Comment on attachment 81895 [details] [diff] [review]
proposed fix

>Index: nsMsgDatabase.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/db/msgdb/src/nsMsgDatabase.cpp,v
>retrieving revision 1.290
>diff -u -w -r1.290 nsMsgDatabase.cpp
>--- nsMsgDatabase.cpp	12 Apr 2002 14:36:11 -0000	1.290
>+++ nsMsgDatabase.cpp	1 May 2002 18:49:15 -0000
>@@ -446,6 +446,10 @@
>         if (!m_ChangeListeners) 
> 			return NS_ERROR_OUT_OF_MEMORY;
>     }
>+  // check if this listener is already registered
>+  // if already registered, do nothing (not an error)
>+  else if (m_ChangeListeners->IndexOf(listener) != -1)
>+    return NS_OK;
> 	return m_ChangeListeners->AppendElement(listener);
> }
This could be cleaner 
  else if (m_ChangeListeners->IndexOf(listener) == -1)
      return m_ChangeListeners->AppendElement(listener);

Comment 13

[Navin Gupta]

Comment on attachment 81897 [details] [diff] [review]
remove unneeded call to AddListener

r=naving

Comment 14

[David :Bienvenu]

No, that's wrong, Navin. We can't use an else if there, because we still want to
append the listener when there were no previous listeners! My initial impulse
was to code it the way you suggested, but then I noticed it would be wrong.

Comment 15

[(not reading, please use [e-mail] instead)]

Comment on attachment 81895 [details] [diff] [review]
proposed fix

sr=sspitzer

Comment 16

[Navin Gupta]

ok, you are right we can't use "else if" but we can use "if". 

Comment 17

[Navin Gupta]

I mean you will have to include in an else

if (m_ChangeListeners == nsnull)
{
   m_ChangeListeners = new nsVoidArray();
   if (!m_ChangeListeners) 
     return NS_ERROR_OUT_OF_MEMORY;
}
+else
+{
 + if (m_ChangeListeners->IndexOf(listener) == -1)
      return m_ChangeListeners->AppendElement(listener);
+}

Comment 18

[David :Bienvenu]

fix checked into trunk. Will await ADT for more input about checking into
branch. These are fairly safe fixes.

Comment 19

[Navin Gupta]

I think it is ok as it is (in your patch). 

Comment 20

[David :Bienvenu]

fixed

Comment 21

[Asa Dotzler [:asa]]

Adding to the RC2 tracking bug. We really should knock this one off the list.
It's hitting a lot of RC1 users and all the previous milestones back to the fall. 

Comment 22

[Asa Dotzler [:asa]]

Comment on attachment 81897 [details] [diff] [review]
remove unneeded call to AddListener

a=asa (on behalf of drivers) for checkin to the 1.0 branch

Comment 23

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) approval for checkin to the 1.0 brnach. Pls check
this in tonight, then add the fixed1.0.0 keyword.

Comment 24

[David :Bienvenu]

I can check it in today - I was in the debugger staring at Mork last night and
didn't see these comments until this morning.

Comment 25

[David :Bienvenu]

I checked in the first patch to the branch, which should be sufficient.

Comment 26

[esther]

Unfortunately, I can't reproduce this with David's scenario and the build dated
the day he found the scenario 5-1-2002.  I'm looking for a reproducible case so
this can be verified as fixed, if I don't find one we'll have to rely on the
lack of Talkback incidents after 5-7 builds to confirm the fix.

Comment 27

[esther]

Try as I may, I could not reproduce the crash with builds before 5-1.  So I took
builds with the fix 5-7 and ran through the scenario to verify the scenario does
indeed work OK.  We'll have to rely on no more talkback reports with this type
of crash using builds after 5-7 to verify it for sure.  

Comment 28

[esther]

I tested 5-7 trunk builds too so this will be marked verified and fixed1.0.0
will be replaced with verified 1.0.0