31891 - [pp]A crash occurs after a onMousedown dialog is closed

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Description

[Chris Petersen]

Build: Apprunner
Version: 2000031318
Platforms: Win 98
Other Platforms: Not occuring on Mac. Need to try linux .
Expected Results: Closing the onMousedown event generated dialog should not crash 
the application.
What I got: Click the close button crashes the browser.

Steps to reproduce:

1) Open the following test case:

http://mozilla.org/quality/browser/standards/html/a_onmousedown.html


2) Click on link to generate a event.

3) After the dilaog appears, click the OK button.

4) The crash occurs.

Comment 1

[leger]

PDT needs more info.  Does this happen on a top to site?  Happens every time?  
Need results for Win, Mac and Linux today's commercial branch builds please!

Comment 2

[Chris Petersen]

With the Win32 March 17th build (2000031716) and Linux March 17th (2000031716), I 
get a crash after closing this alert dialog created by JS. The Mac March 17th  
(2000031712) doesn't crash when closing this dialog. This test case wasn't 
distilled from a site but alert dialogs are used frequently to prompt users is 
common at web sites.

Comment 3

[Chris Petersen]

This crash happens every time when using the Linux or Win 32 build.Tested under 
Windows 98 and Linux Redhat 6.0.

Comment 4

[Jim Roskind]

Is there a stack trace for this issue??

Comment 5

[joki (gone)]

Okay, have a fix for this too.  It seems that the DocumentViewerImpl is adding a 
SelectionListener called nsDocViwerSelectionListener which its not removing.  As 
the selection listeners are refcnted by the nsRangeList this means the 
nsDocViwerSelectionListener doesn't go away and when activated later it dies.  
This code only went in on 2/15 so this likely was an oversight which can been in 
since then.  Fix is below, including comments for AddSelectionListener and 
RemoveSelectionListener noting their refcnt'd nature.

Index: src/nsDocumentViewer.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/base/src/nsDocumentViewer.cpp,v
retrieving revision 1.43
diff -r1.43 nsDocumentViewer.cpp
353a354,364
>   //If we added a selection listener then remove it here
>   if (mSelectionListener) {
>     nsresult rv;
>
>     nsCOMPtr<nsIDOMSelection> selection;
>     rv = GetDocumentSelection(getter_AddRefs(selection));
>     if (NS_SUCCEEDED(rv)) {
>       selection->RemoveSelectionListener(mSelectionListener);
>     }
>   }
>
Index: src/nsRangeList.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/base/src/nsRangeList.cpp,v
retrieving revision 1.183
diff -r1.183 nsRangeList.cpp
4415c4415,4417
<
---
> //The rangelist will hold a ref to the listener added.  Therefore
> //the listener must be removed when it is no longer needed, not just
> //released by its owner.
4423c4425,4427
<
---
> //The rangelist will hold refs to all listeners added.  Therefore
> //the listener must be removed when it is no longer needed, not just
> //released by its owner.

Comment 6

[mjudge]

ok this looks good. What pixley has said is accurate. nice to see this caught 
now. r = mjudge

Comment 7

[Chris Petersen]

Fixed in the Windows and Linux builds (2000032006).

Comment 8

[Chris Petersen]

This was fixed in the latest beta builds (2000032006).

Comment 9

[Chris Petersen]

Marking verified in the  Windows and Linux builds (2000032006).