54530 - Edit | List. Clicking OK crashes.

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Address Book & Contacts, defect, P2)

Description

[Suresh]

steps:
1. Create a new Mailing list. 
2. Close AB window. Relaunch AB again.
3. Select the list. Click Edit. Click OK. Booom....

Build and platform:
Today's BRANCH commercial build on Win NT.

Comment 1

[Suresh]

stack trace:
  nsAddrDatabase::NotifyListEntryChange 
                                                
[d:\builds\seamonkey\mozilla\mailnews\addrbook\src\nsAddrDatabase.cpp, line 360]
     
  nsAddrDatabase::EditMailList 
                                                
[d:\builds\seamonkey\mozilla\mailnews\addrbook\src\nsAddrDatabase.cpp, line 
2509]
     
  nsAbDirProperty::EditMailListToDatabase 
                                                
[d:\builds\seamonkey\mozilla\mailnews\addrbook\src\nsAbDirProperty.cpp, line 
377]
     
  XPTC_InvokeByIndex 
                                                
[d:\builds\seamonkey\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcinvoke.cpp, 
line 139]
     
  nsXPCWrappedNativeClass::CallWrappedMethod 
                                                
[d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappednativeclass.cpp, 
line 915]
     
  WrappedNative_CallMethod 
                                                
[d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappednativejsops.cpp, 
line 223]
     
  js_Invoke 
                                                
[d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 822]
     
  js_Interpret 
                                                
[d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 2622]
     
  js_Invoke 
                                                
[d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 838]
     
  js_InternalInvoke 
                                                
[d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 910]
     
  JS_CallFunctionValue 
                                                
[d:\builds\seamonkey\mozilla\js\src\jsapi.c, line 3195]
     
  nsJSContext::CallEventHandler 
                                                
[d:\builds\seamonkey\mozilla\dom\src\base\nsJSEnvironment.cpp, line 910]
     
  nsJSEventListener::HandleEvent 
                                                
[d:\builds\seamonkey\mozilla\dom\src\events\nsJSEventListener.cpp, line 155]
     
  nsEventListenerManager::HandleEventSubType 
                                                
[d:\builds\seamonkey\mozilla\layout\events\src\nsEventListenerManager.cpp, line 
789]
     
  nsEventListenerManager::HandleEvent 
                                                
[d:\builds\seamonkey\mozilla\layout\events\src\nsEventListenerManager.cpp, line 
942]
     
  nsXULElement::HandleDOMEvent 
                                                
[d:\builds\seamonkey\mozilla\rdf\content\src\nsXULElement.cpp, line 3321]
     
  PresShell::HandleEventInternal 
                                                
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 4257]
     
  PresShell::HandleEventWithTarget 
                                                
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 4237]
     
  nsEventStateManager::CheckForAndDispatchClick 
                                                
[d:\builds\seamonkey\mozilla\layout\events\src\nsEventStateManager.cpp, line 
1855]
     
  nsEventStateManager::PostHandleEvent 
                                                
[d:\builds\seamonkey\mozilla\layout\events\src\nsEventStateManager.cpp, line 
937]
     
  PresShell::HandleEventInternal 
                                                
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 4278]
     
  PresShell::HandleEvent 
                                                
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 4192]
     
  nsView::HandleEvent 
                                                
[d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 379]
     
  nsViewManager2::DispatchEvent 
                                                
[d:\builds\seamonkey\mozilla\view\src\nsViewManager2.cpp, line 1429]
     
  HandleEvent 
                                                
[d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 68]
     
  nsWindow::DispatchEvent 
                                                
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 685]
     
  nsWindow::DispatchWindowEvent 
                                                
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 702]
     
  nsWindow::DispatchMouseEvent 
                                                
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 3892]
     
  ChildWindow::DispatchMouseEvent 
                                                
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 4100]
     
  nsWindow::ProcessMessage 
                                                
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 2985]
     
  nsWindow::WindowProc 
                                                
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 951]
     
  USER32.dll + 0x1820 (0x77e71820) 
                                                 

Comment 2

[laurel]

Happens with today's linux branch commercial build, too.  Changing platform
field to All.

Comment 3

[scottputterman]

reassigning to chuang. changeListener has been deleted.

Comment 4

[selmer (gone)]

rtm+, looks very easy to hit.

Comment 5

[scottputterman]

Changing to rtm need info. We want to rtm+ it but need a patch and code reviewers.

Comment 6

[scottputterman]

changing to P2

Comment 7

[chuang]

I found the problem but not the fix yet. The changeListener didn't get clean up 
when address book window is closed.

Comment 8

[chuang]

ref count problem, still looking for the fix.

Comment 9

[chuang]

I am getting close to have a fix, I need time to do the testing before I say I 
have a fix.

Comment 10

[chuang]

Attachment: Patch for the fix

Comment 11

[scottputterman]

Adding topcrash. This has been showing up on the PR3 branch top crash list for
the last couple of weeks so I think we want to take this once we get reviews and
the opportunity to check in presents itself.

Comment 12

[chuang]

This bug will happen in address book card too.  Follow the steps, just replace 
Mialing list with card, you get the same result.  

Comment 13

[selmer (gone)]

Does the attached patch fix the AB card case too?  Who is doing the r= and sr=?
 When those reviews are done, please move to [rtm+] in the whiteboard.

Comment 14

[chuang]

The patch will fix the card too.  I've found another crash may or may not 
related to this patch.  Scott P. is reviewing the code (we will go over the 
patch tomorrow).  We want to make sure the fix won't make the code more fragile.

Comment 15

[selmer (gone)]

This is a rather large patch.  Please land it on the trunk and get both
regression and affirmative testing done on it.  Please describe the testing done
in the bug (and get the reviews) before moving it to rtm+.

Comment 16

[chuang]

Attachment: A real minimum fix for this bug

Comment 17

[chuang]

This new fix remove the listener that we didn't remove to cause the invalid 
listener hanging in the listener array.  You may see ui problem after the fix 
(like duplicate address entry in the mailing list), it is not introduced by this 
fix.

Comment 18

[Scott MacGregor]

Candice, just so I'm clear, an ab directory is a listener on just one ab
database? Why do we only execute this code path if the URI is the data source
root? Are there other code paths where we are registered as a listener on an ab
database that we need to remove ourselves too?

Comment 19

[chuang]

We execute the code if the URI is NOT the data source root(strcmp returns 0 if 
the strings are identical).  nsAbDirectory is either an address book or a 
mailing list.  nsAbCard is another listener we have to remove and it is done in 
the code already.

Comment 20

[scottputterman]

I think it's when it's not the datasource root that this is happening. This
looks good to me since it sounds like you are now removing the listener that got
deleted so the next time opening the ab that listener will be gone.

Minor nitpick. Can we make "addresbook" be "addressbook"?

Comment 21

[scottputterman]

adding to summary.

Comment 22

[selmer (gone)]

Candice, thanks for the extra effort to reduce this to a minimum fix!

Comment 23

[Scott MacGregor]

looks good sr=mscott

Comment 24

[chuang]

Attachment: Final patch with spelling correction

Comment 25

[chuang]

The last patch changed "addresBook" to "addressBook" and add "addressBook" into 
checking "if (NS_SUCCEEDED(rv) && addressBook)".

Did I get r=putterman?

Comment 26

[scottputterman]

yes, r=putterman

Comment 27

[selmer (gone)]

rtm++

Comment 28

[chuang]

Fix checked in on branch and trunk.

Comment 29

[Suresh]

Verified in Today's BRANCH builds on all platforms.

Comment 30

[varada]

Marking Verified on Branch
WinNT 2000102008
Mac 2000102008
Linux 2000102009

Comment 31

[Asa Dotzler [:asa]]

Verified Fixed on Mozilla trunk builds.  Creating an list or card, closing AB,
reopening AB and editicn list or card works fine in classic and modern themes on
linux 102308 RedHat 6.2
win32 102304 NT 4
mac 102312 Mac OS9
Setting bug to Verified and removing vtrunk keyword

Comment 32

[chuang]

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