53827 - MTBF - Crash in nsXULWindow::Destroy - dereferencing a null mWindow [@ nsXULWindow::Destroy]

Status: VERIFIED FIXED

(Core :: XUL, defect, P1)

Description

[jefft]

Had a crash nsXULWindow::Destroy(). It looks like as one of the top crashers. 
Adding a check should be able to prevent it to crash.

Index: nsXULWindow.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpfe/appshell/src/nsXULWindow.cpp,v
retrieving revision 1.52
diff -c -r1.52 nsXULWindow.cpp
*** nsXULWindow.cpp     2000/09/19 22:17:32     1.52
--- nsXULWindow.cpp     2000/09/22 21:40:13
***************
*** 315,321 ****
     // destroyed window. This is especially necessary when the eldest window
     // in a stack of modal windows is destroyed first. It happens.
     ExitModalLoop(NS_OK);
!    mWindow->Show(PR_FALSE);

     mDOMWindow = nsnull;
     if(mDocShell) {
--- 315,323 ----
     // destroyed window. This is especially necessary when the eldest window
     // in a stack of modal windows is destroyed first. It happens.
     ExitModalLoop(NS_OK);
!    if (mWindow) {
!      mWindow->Show(PR_FALSE);
!    }

     mDOMWindow = nsnull;
     if(mDocShell) {


nsXULWindow::Destroy(nsXULWindow * const 0x071fc534) line 318 + 19 bytes
nsWebShellWindow::Destroy(nsWebShellWindow * const 0x071fc534) line 1779
nsChromeTreeOwner::Destroy(nsChromeTreeOwner * const 0x071fd464) line 217
GlobalWindowImpl::Close(GlobalWindowImpl * const 0x071fd1a4) line 2054
GlobalWindowImpl::CloseWindow(nsISupports * 0x071fd1a0) line 3540
nsJSContext::ScriptEvaluated(nsJSContext * const 0x071fda80, int 0x00000001) 
line 1294 + 18 bytes
nsJSContext::CallEventHandler(nsJSContext * const 0x071fda80, void * 0x068e6458, 
void * 0x00d186c8, unsigned int 0x00000001, void * 0x0012db50, int * 0x0012db4c, 
int 0x00000000) line 914
nsJSEventListener::HandleEvent(nsIDOMEvent * 0x0730b814) line 154 + 64 bytes
nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x0514d270, 
nsIDOMEvent * 0x0730b814, nsIDOMEventTarget * 0x05149b38, unsigned int 
0x00000004, unsigned int 0x00000007) line 788 + 19 bytes
nsEventListenerManager::HandleEvent(nsIPresContext * 0x0513a920, nsEvent * 
0x0012e430, nsIDOMEvent * * 0x0012e34c, nsIDOMEventTarget * 0x05149b38, unsigned 
int 0x00000007, nsEventStatus * 0x0012e750) line 935 + 39 bytes
nsXULElement::HandleDOMEvent(nsXULElement * const 0x05149b30, nsIPresContext * 
0x0513a920, nsEvent * 0x0012e430, nsIDOMEvent * * 0x0012e34c, unsigned int 
0x00000001, nsEventStatus * 0x0012e750) line 3321
PresShell::HandleEventInternal(nsEvent * 0x0012e430, nsIView * 0x00000000, 
unsigned int 0x00000001, nsEventStatus * 0x0012e750) line 4255 + 47 bytes
PresShell::HandleEventWithTarget(PresShell * const 0x05138400, nsEvent * 
0x0012e430, nsIFrame * 0x00e088d4, nsIContent * 0x05149b30, unsigned int 
0x00000001, nsEventStatus * 0x0012e750) line 4236 + 22 bytes
nsEventStateManager::CheckForAndDispatchClick(nsEventStateManager * const 
0x07329380, nsIPresContext * 0x0513a920, nsMouseEvent * 0x0012e860, 
nsEventStatus * 0x0012e750) line 1854 + 61 bytes
nsEventStateManager::PostHandleEvent(nsEventStateManager * const 0x07329388, 
nsIPresContext * 0x0513a920, nsEvent * 0x0012e860, nsIFrame * 0x00e088d4, 
nsEventStatus * 0x0012e750, nsIView * 0x05138a90) line 935 + 28 bytes
PresShell::HandleEventInternal(nsEvent * 0x0012e860, nsIView * 0x05138a90, 
unsigned int 0x00000001, nsEventStatus * 0x0012e750) line 4275 + 43 bytes
PresShell::HandleEvent(PresShell * const 0x05138404, nsIView * 0x05138a90, 
nsGUIEvent * 0x0012e860, nsEventStatus * 0x0012e750, int 0x00000001, int & 
0x00000001) line 4190 + 25 bytes
nsView::HandleEvent(nsView * const 0x05138a90, nsGUIEvent * 0x0012e860, unsigned 
int 0x0000001c, nsEventStatus * 0x0012e750, int 0x00000001, int & 0x00000001) 
line 379
nsViewManager2::DispatchEvent(nsViewManager2 * const 0x05138c20, nsGUIEvent * 
0x0012e860, nsEventStatus * 0x0012e750) line 1429
HandleEvent(nsGUIEvent * 0x0012e860) line 68
nsWindow::DispatchEvent(nsWindow * const 0x05138954, nsGUIEvent * 0x0012e860, 
nsEventStatus & nsEventStatus_eIgnore) line 681 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012e860) line 702
nsWindow::DispatchMouseEvent(unsigned int 0x0000012d, nsPoint * 0x00000000) line 
3890 + 21 bytes
ChildWindow::DispatchMouseEvent(unsigned int 0x0000012d, nsPoint * 0x00000000) 
line 4100
nsWindow::ProcessMessage(unsigned int 0x00000202, unsigned int 0x00000000, long 
0x005500b7, long * 0x0012ebdc) line 2960 + 24 bytes
nsWindow::WindowProc(HWND__ * 0x001702f8, unsigned int 0x00000202, unsigned int 
0x00000000, long 0x005500b7) line 950 + 27 bytes
USER32! 77e71820()
005500b7()

Comment 1

[jefft]

adding nsbeta3, crash keyword.

Comment 2

[Jay Patel [:jay]]

adding topcrash keyword since this crash has been on the topcrash list for most 
of this week.  it has gone off the top40 radar as of today, but just wanted to 
track it. [@ nsXULWindow::Destroy]

Comment 3

[Shiva Thirumazhusai]

Since we are analyzing only the last 9 days of crash data. Most of crash report
might have happened earlier than 9 days. If need be we can generate the data 
earlier than 10 days or specific to this stack signature.

Comment 4

[jefft]

Reassing to hyatt, not sure the patch is the right fix ...

Comment 5

[David Hyatt]

--> WINDOW MAN

Comment 6

[Peter Trudelle]

nsbeta3-, rtm+, p1/critical M19

Comment 7

[Phil Peterson]

PDT agrees [rtm need info] until code reviews are available.

Comment 8

[Dan M]

  Urk! I'd like to be able to reproduce this crash. Something's wrong; mWindow 
shouldn't be null here. Still, lacking that, jefft's patch seems good, especially 
since a similar check is made later in the same function. r=me.

Comment 9

[Mike Shaver (:shaver emeritus)]

a=shaver

Comment 10

[jefft]

I was able to reproduce the problem by disabling the cookies and check for warn 
me before accepting a cookie. I manually close down all windows and leave the 
warning dialog box the last one to close.

Comment 11

[Dan M]

  Urk! some more! The cookie alert is supposed to be modal to the browser window, 
so it should be impossible to close all browser windows before closing the alert. 
I just tried it, and it worked the way it was supposed to. I know there are 
exceptional circumstances where the cookie alert would be modal to the wrong 
window, but we really need to ferret all those out and kill them.
  This has always a been very dangerous thing to do -- closing the "parent" 
browser before closing its "modal" windows. When they weren't really modal and 
this was possible. But if this one null check fixes those crashes, I'll be happy 
to check it in.

Comment 12

[Phil Peterson]

PDT marking [rtm++]

Comment 13

[Dan M]

  I still can't reproduce the bug, but the null check is checked in to the trunk 
and Netscape rtm branch. Is it fixed, then?

Comment 14

[John Morrison]

Well, Dan. Looks like good news, bad news (or semi-bad news).

On linux it is possible to close the parent browser window, and leave this
cookie alert dialog as the sole remaining mozilla window open. However, when
this last window is closed, there is no crash, so that's good.

(I will leave this open for a couple more days, then do a query on cyclone to
see if this is showing up as a topcrash anymore).

Note that the cookie alert dialog is indeed modal -- the browser window will
not accept any user interaction [*], and the dialog remain z-order above the
browser window at all times. I will file a bug.

[*] er, with two exceptions: you can type in the url bar (carriage return goes
to the new site [eeek!]), and the window's 'X' close widget will close the
parent browser window.

[Note: on the mac, this cookie alert is application modal, and on win32 it is
window modal].

Comment 15

[Dan M]

Oh, yes. Some or all window managers on Linux leave the min/max/close widgets 
active on the parent window. The one I use does, anyway. Sigh. I remember working 
on that in ages past. I'd swear we had some old bug covering that; I mean besides 
the one you just filed (bug 55472). Can't find it though.

Comment 16

[John Morrison]

Well, it's no great surprise that _not_ dereferencing a null pointer avoids
a crash :-]  I looked through talkback reports, and post-checkin this crash
has stopped occurring. (There were a lot crashing on this condition for the
PR3 release). Marking verified fixed.