Closed Bug 54233 Opened 24 years ago Closed 24 years ago

Quitting via keyboard crashes on exit

Categories

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

Product:
Core
Component:
DOM: UI Events & Focus Handling
Platform:
x86
Windows NT
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: alecf, Assigned: hjtoi-bugzilla)

Details

(Keywords: crash, Whiteboard: [nsbeta3-][rtm++][fixinhand])

Attachments

(1 file)

Fix: make sure presshell is released before viewmanager
1.58 KB, patch
Details | Diff | Splinter Review 
if you type Alt-F x (or Alt-F q on linux) to quit mozilla, it crashes.

here is the linux stack trace:

 #0  0xdbdbdbdb in ?? ()
#1  0x4143afa0 in PresShell::Release (this=0x875b160) at nsPresShell.cpp:1188
#2  0x416b7e0f in nsMenuFrame::Execute (this=0x8d34584)
    at ../../../../dist/include/nsCOMPtr.h:489
#3  0x416b62f7 in nsMenuFrame::Enter (this=0x8d34584) at nsMenuFrame.cpp:1152
#4  0x416b079e in nsMenuPopupFrame::ShortcutNavigation (this=0x8cb18a0, 
    aLetter=113, aHandledFlag=@0xbfffcf20) at nsMenuPopupFrame.cpp:1014
#5  0x416b623b in nsMenuFrame::ShortcutNavigation (this=0x89ea934, 
    aLetter=113, aHandledFlag=@0xbfffcf20) at nsMenuFrame.cpp:1104
#6  0x416bb1e9 in nsMenuBarFrame::ShortcutNavigation (this=0x89d4c4c, 
    aLetter=113, aHandledFlag=@0xbfffcf20) at nsMenuBarFrame.cpp:269
#7  0x416be3c5 in nsMenuListener::KeyPress (this=0x8fd0130, 
    aKeyEvent=0x8a91874) at nsMenuListener.cpp:194
#8  0x413d1ea8 in nsEventListenerManager::HandleEvent (this=0x85871f0, 
    aPresContext=0x8fbaa50, aEvent=0xbffff3fc, aDOMEvent=0xbfffeffc, 
    aCurrentTarget=0x8586ae8, aFlags=4, aEventStatus=0xbffff328)
    at nsEventListenerManager.cpp:1118
#9  0x40708b61 in nsXULDocument::HandleDOMEvent (this=0x8586ac8, 
    aPresContext=0x8fbaa50, aEvent=0xbffff3fc, aDOMEvent=0xbfffeffc, aFlags=4, 
    aEventStatus=0xbffff328) at nsXULDocument.cpp:2111
#10 0x406eb91a in nsXULElement::HandleDOMEvent (this=0x8861dd0, 
    aPresContext=0x8fbaa50, aEvent=0xbffff3fc, aDOMEvent=0xbfffeffc, aFlags=4, 
    aEventStatus=0xbffff328) at nsXULElement.cpp:3304
#11 0x406eb8ef in nsXULElement::HandleDOMEvent (this=0x88a04b8, 
    aPresContext=0x8fbaa50, aEvent=0xbffff3fc, aDOMEvent=0xbfffeffc, aFlags=4, 
    aEventStatus=0xbffff328) at nsXULElement.cpp:3300
#12 0x406eb8ef in nsXULElement::HandleDOMEvent (this=0x88a2b40, 
    aPresContext=0x8fbaa50, aEvent=0xbffff3fc, aDOMEvent=0xbfffeffc, aFlags=4, 
    aEventStatus=0xbffff328) at nsXULElement.cpp:3300
#13 0x406eb8ef in nsXULElement::HandleDOMEvent (this=0x88a2ca0, 
    aPresContext=0x8fbaa50, aEvent=0xbffff3fc, aDOMEvent=0xbfffeffc, aFlags=4, 
    aEventStatus=0xbffff328) at nsXULElement.cpp:3300
#14 0x406eb8ef in nsXULElement::HandleDOMEvent (this=0x884dbb8, 
    aPresContext=0x8fbaa50, aEvent=0xbffff3fc, aDOMEvent=0xbfffeffc, aFlags=4, 
    aEventStatus=0xbffff328) at nsXULElement.cpp:3300
#15 0x406f18cd in nsXULElement::HandleChromeEvent (this=0x884dbb8, 
    aPresContext=0x8fbaa50, aEvent=0xbffff3fc, aDOMEvent=0xbfffeffc, aFlags=4, 
    aEventStatus=0xbffff328) at nsXULElement.cpp:4296
#16 0x40482b42 in ?? ()
   from /home1/alecf/seamonkey/mozilla/dist/bin/libjsdom.so
#17 0x41713216 in nsDocument::HandleDOMEvent (this=0x9294ba0, 
    aPresContext=0x8fbaa50, aEvent=0xbffff3fc, aDOMEvent=0xbfffeffc, aFlags=4, 
    aEventStatus=0xbffff328) at nsDocument.cpp:3038
#18 0x41746ca9 in nsGenericElement::HandleDOMEvent (this=0x922f81c, 
    aPresContext=0x8fbaa50, aEvent=0xbffff3fc, aDOMEvent=0xbfffeffc, aFlags=1, 
    aEventStatus=0xbffff328) at nsGenericElement.cpp:1387
#19 0x414b2d93 in nsHTMLHtmlElement::HandleDOMEvent (this=0x922f808, 
    aPresContext=0x8fbaa50, aEvent=0xbffff3fc, aDOMEvent=0x0, aFlags=1, 
    aEventStatus=0xbffff328) at nsHTMLHtmlElement.cpp:185
#20 0x41447b1b in PresShell::HandleEventInternal (this=0x90d2f08, 
    aEvent=0xbffff3fc, aView=0x8f837b8, aFlags=1, aStatus=0xbffff328)
    at nsPresShell.cpp:4255
#21 0x4144783c in PresShell::HandleEvent (this=0x90d2f08, aView=0x8f837b8, 
    aEvent=0xbffff3fc, aEventStatus=0xbffff328, aForceHandle=0, 
    aHandled=@0xbffff2bc) at nsPresShell.cpp:4190
#22 0x41a08d89 in ?? ()
   from /home1/alecf/seamonkey/mozilla/dist/bin/components/libgkview.so
#23 0x41a08d2e in ?? ()
   from /home1/alecf/seamonkey/mozilla/dist/bin/components/libgkview.so
#24 0x41a08d2e in ?? ()
   from /home1/alecf/seamonkey/mozilla/dist/bin/components/libgkview.so
#25 0x41a134be in ?? ()
   from /home1/alecf/seamonkey/mozilla/dist/bin/components/libgkview.so
#26 0x41a084bd in ?? ()
   from /home1/alecf/seamonkey/mozilla/dist/bin/components/libgkview.so
#27 0x40a7a891 in ?? ()
   from /home1/alecf/seamonkey/mozilla/dist/bin/components/libwidget_gtk.so
#28 0x40a7a609 in ?? ()
   from /home1/alecf/seamonkey/mozilla/dist/bin/components/libwidget_gtk.so
#29 0x40a77bc1 in ?? ()
   from /home1/alecf/seamonkey/mozilla/dist/bin/components/libwidget_gtk.so
#30 0x40a73d50 in ?? ()
   from /home1/alecf/seamonkey/mozilla/dist/bin/components/libwidget_gtk.so
#31 0x40a7417e in ?? ()
   from /home1/alecf/seamonkey/mozilla/dist/bin/components/libwidget_gtk.so
#32 0x40a7403a in ?? ()
   from /home1/alecf/seamonkey/mozilla/dist/bin/components/libwidget_gtk.so
#33 0x40bf956b in ?? () from /usr/lib/libgdk-1.2.so.0
#34 0x40c261b6 in ?? () from /usr/lib/libglib-1.2.so.0
#35 0x40c26781 in ?? () from /usr/lib/libglib-1.2.so.0
#36 0x40c26921 in ?? () from /usr/lib/libglib-1.2.so.0
#37 0x40b4e919 in ?? () from /usr/lib/libgtk-1.2.so.0
#38 0x40a683ec in ?? ()
   from /home1/alecf/seamonkey/mozilla/dist/bin/components/libwidget_gtk.so
#39 0x405acc79 in ?? ()
   from /home1/alecf/seamonkey/mozilla/dist/bin/components/libnsappshell.so
#40 0x80528bb in main1 (argc=2, argv=0xbffff724, nativeApp=0x0)
    at nsAppRunner.cpp:1004
#41 0x8052e22 in main (argc=2, argv=0xbffff724) at nsAppRunner.cpp:1185
#42 0x402ed1eb in __libc_start_main (main=0x8052ccc <main>, argc=2, 
    argv=0xbffff724, init=0x804c3f0 <_init>, fini=0x805ee94 <_fini>, 
    rtld_fini=0x4000a610 <_dl_fini>, stack_end=0xbffff71c)
    at ../sysdeps/generic/libc-start.c:90
(gdb) frame 1
#1  0x4143afa0 in PresShell::Release (this=0x875b160) at nsPresShell.cpp:1188
(gdb) print this
Cannot access memory at address 0x0.

so "this" has already been destroyed/released.
adding hyatt and saari because this is probably something with XUL and keys
adding nsbeta3, rtm keywords because this is probably an often-hit situation
adding crash keyword
Keywords: crash, nsbeta3, rtm
I doubt if this qualifies as a beta 3 stopper.  So, marking nsbeta3- and rtm+.
Whiteboard: [nsbeta3-][rtm+]
PDT marking [rtm need info] until patch and code reviews are available.
Whiteboard: [nsbeta3-][rtm+] → [nsbeta3-][rtm need info]
Heikki, please look into this.  Tom is away on vacation this week.  Thanks!
Assignee: joki → heikki
I do not see a crash on Linux. But I do see the crash on NT. Not all the time, 
though. It seems like it happens more often with the Classic skin. I have hit 
various assertions before the crash as well.

Here is a stack trace on NT when I had Classic skin and did Alt-f x:

PresShell::~PresShell() line 1267 + 15 bytes
PresShell::`scalar deleting destructor'() + 15 bytes
PresShell::Release(PresShell * const 0x029cead0) line 1188 + 158 bytes
nsCOMPtr<nsIPresShell>::~nsCOMPtr<nsIPresShell>() line 490
nsMenuFrame::Execute() line 1510 + 16 bytes
nsMenuFrame::Enter(nsMenuFrame * const 0x02c58ca8) line 1153
nsMenuPopupFrame::ShortcutNavigation(nsMenuPopupFrame * const 0x02c2af00, 
unsigned int 0x00000078, int & 0x00000001) line 1017
nsMenuFrame::ShortcutNavigation(nsMenuFrame * const 0x00dde988, unsigned int 
0x00000078, int & 0x00000001) line 1107
nsMenuBarFrame::ShortcutNavigation(nsMenuBarFrame * const 0x00dc8a94, unsigned 
int 0x00000078, int & 0x00000001) line 270
nsMenuListener::KeyPress(nsIDOMEvent * 0x046389b4) line 198
nsEventListenerManager::HandleEvent(nsIPresContext * 0x03d19380, nsEvent * 
0x0012f8bc, nsIDOMEvent * * 0x0012f5d8, nsIDOMEventTarget * 0x028fa1c0, unsigned 
int 0x00000004, nsEventStatus * 0x0012f828) line 1118 + 23 bytes
nsXULDocument::HandleDOMEvent(nsXULDocument * const 0x028fa1a0, nsIPresContext * 
0x03d19380, nsEvent * 0x0012f8bc, nsIDOMEvent * * 0x0012f5d8, unsigned int 
0x00000004, nsEventStatus * 0x0012f828) line 2112
nsXULElement::HandleDOMEvent(nsXULElement * const 0x029ecc00, nsIPresContext * 
0x03d19380, nsEvent * 0x0012f8bc, nsIDOMEvent * * 0x0012f5d8, unsigned int 
0x00000004, nsEventStatus * 0x0012f828) line 3304 + 39 bytes
nsXULElement::HandleDOMEvent(nsXULElement * const 0x03264af0, nsIPresContext * 
0x03d19380, nsEvent * 0x0012f8bc, nsIDOMEvent * * 0x0012f5d8, unsigned int 
0x00000004, nsEventStatus * 0x0012f828) line 3302
nsXULElement::HandleDOMEvent(nsXULElement * const 0x03264880, nsIPresContext * 
0x03d19380, nsEvent * 0x0012f8bc, nsIDOMEvent * * 0x0012f5d8, unsigned int 
0x00000004, nsEventStatus * 0x0012f828) line 3302
nsXULElement::HandleDOMEvent(nsXULElement * const 0x032644f0, nsIPresContext * 
0x03d19380, nsEvent * 0x0012f8bc, nsIDOMEvent * * 0x0012f5d8, unsigned int 
0x00000004, nsEventStatus * 0x0012f828) line 3302
nsXULElement::HandleDOMEvent(nsXULElement * const 0x032643a0, nsIPresContext * 
0x03d19380, nsEvent * 0x0012f8bc, nsIDOMEvent * * 0x0012f5d8, unsigned int 
0x00000004, nsEventStatus * 0x0012f828) line 3302
nsXULElement::HandleChromeEvent(nsXULElement * const 0x032643b4, nsIPresContext 
* 0x03d19380, nsEvent * 0x0012f8bc, nsIDOMEvent * * 0x0012f5d8, unsigned int 
0x00000004, nsEventStatus * 0x0012f828) line 4296 + 39 bytes
GlobalWindowImpl::HandleDOMEvent(GlobalWindowImpl * const 0x04a9c0d0, 
nsIPresContext * 0x03d19380, nsEvent * 0x0012f8bc, nsIDOMEvent * * 0x0012f5d8, 
unsigned int 0x00000004, nsEventStatus * 0x0012f828) line 500
nsDocument::HandleDOMEvent(nsDocument * const 0x03cea6f0, nsIPresContext * 
0x03d19380, nsEvent * 0x0012f8bc, nsIDOMEvent * * 0x0012f5d8, unsigned int 
0x00000004, nsEventStatus * 0x0012f828) line 3043
nsGenericElement::HandleDOMEvent(nsIPresContext * 0x03d19380, nsEvent * 
0x0012f8bc, nsIDOMEvent * * 0x0012f5d8, unsigned int 0x00000001, nsEventStatus * 
0x0012f828) line 1393
nsHTMLHtmlElement::HandleDOMEvent(nsHTMLHtmlElement * const 0x03d19138, 
nsIPresContext * 0x03d19380, nsEvent * 0x0012f8bc, nsIDOMEvent * * 0x00000000, 
unsigned int 0x00000001, nsEventStatus * 0x0012f828) line 186
PresShell::HandleEventInternal(nsEvent * 0x0012f8bc, nsIView * 0x046c3420, 
unsigned int 0x00000001, nsEventStatus * 0x0012f828) line 4255 + 47 bytes
PresShell::HandleEvent(PresShell * const 0x03d76d74, nsIView * 0x046c3420, 
nsGUIEvent * 0x0012f8bc, nsEventStatus * 0x0012f828, int 0x00000000, int & 
0x00000001) line 4190 + 25 bytes
nsView::HandleEvent(nsView * const 0x046c3420, nsGUIEvent * 0x0012f8bc, unsigned 
int 0x00000008, nsEventStatus * 0x0012f828, int 0x00000000, int & 0x00000001) 
line 379
nsView::HandleEvent(nsView * const 0x046c2a10, nsGUIEvent * 0x0012f8bc, unsigned 
int 0x00000008, nsEventStatus * 0x0012f828, int 0x00000000, int & 0x00000001) 
line 352
nsView::HandleEvent(nsView * const 0x03d77830, nsGUIEvent * 0x0012f8bc, unsigned 
int 0x0000001c, nsEventStatus * 0x0012f828, int 0x00000001, int & 0x00000001) 
line 352
nsViewManager2::DispatchEvent(nsViewManager2 * const 0x03d6fe00, nsGUIEvent * 
0x0012f8bc, nsEventStatus * 0x0012f828) line 1439
HandleEvent(nsGUIEvent * 0x0012f8bc) line 68
nsWindow::DispatchEvent(nsWindow * const 0x046c7764, nsGUIEvent * 0x0012f8bc, 
nsEventStatus & nsEventStatus_eIgnore) line 681 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f8bc) line 702
nsWindow::DispatchKeyEvent(unsigned int 0x00000083, unsigned short 0x0078, 
unsigned int 0x00000000) line 2284 + 15 bytes
nsWindow::OnChar(unsigned int 0x00000078, unsigned int 0x00000000, unsigned char 
0x00) line 2408
nsWindow::ProcessMessage(unsigned int 0x00000102, unsigned int 0x00000078, long 
0x002d0001, long * 0x0012fc44) line 2841 + 33 bytes
nsWindow::WindowProc(HWND__ * 0x002f03f2, unsigned int 0x00000102, unsigned int 
0x00000078, long 0x002d0001) line 950 + 27 bytes
USER32! 77e71820()
IsASCIIIdentifier(JSString *) line
Status: NEW → ASSIGNED
OS: Linux → Windows NT
I still crash only on NT. I need to click the content area before I do Ctrl-f x
to see the crash. If the cursor is in the urlbar when I do this it does not crash.

I can prevent the crash by doing an extra addref on the preshell in
nsMenuFrame::Execute(), but then we leak webshells :(

It looks like somebody already deleted the presshell even though we had a ref to
it here. At least the mViewManager pointer in PresShell::~PresShell() is
0xdddddddd (which is where the OS notices something fishy is going on).

There is code from hyatt in the nsMenuFrame::Execute() that is supposed to take
care of this exact Quit+crash scene, but for some reason it is not enough in
this case.
Actually I don't think the presshell has been deleted yet. But some things have
been deleted even though they shouldn't have. mViewManager is a weak ref and the
comment says document viewer owns it. But it looks like the document viewer was
deleted. Making mViewManager a nsWeakReference (or whatever it was) would not
help either, because the release of mFrameManager would still lead to situations
that crash (tried, more cases where pointers are 0xdddddddd).
Severity: normal → critical
This is a potential duplicate of bug 54868.
This is a potential duplicate of bug 54868.
Ok, the first patch is following the same idea that Nisheeth found fixed bug
54868. This is not a dupliate, but we can use the same principle.

Nisheeth, can you r=?
Whiteboard: [nsbeta3-][rtm need info] → [nsbeta3-][rtm need info][fixinhand]
I'm worried by your comment - we cannot rely on declaration order for the 
propery destruction and release order of the nsCOMPtr's. Some compilers may 
optimize this code differently. 

You need to manually release the COMPtrs by setting them to "nsnull" in the 
order that you want the Release() to happen.

And even if yes, even it works on gcc, VC++, and CodeWarrior, this is still an 
incorrect assumption, esp if we can fix it.
Just a clarification of the language here: the order of destruction is a
requirement and is guaranteed by C++'s definition.  See in particular section
6.6 Jump statements [stmt.jump], paragraph 2, which reads (in part)

  On exit from a scope (however accomplished), destructors (12.4) are called
  for all constructed objects with automatic storage duration (3.7.2) (named
  objects or temporaries) that are declared in that scope, in the reverse
  order of their declaration. [...]

This is a promise.  All compilers will destroy named variables local to a scope
on exit from that scope in the reverse order or their declaration.  This promise
is similar to the promises about the order of destruction of member variables
and of the elements of an array.  So we _can_ rely on this to fix order
dependent situations, and this is even a recommendation in the |nsCOMPtr| user
guide.  Quoting from paragraph 2 of the section "How do I ... |Release()| an
|nsCOMPtr| before it goes out of scope?"

  You should note, though, that there is a small performance penalty for
  [|Release()|ing an |nsCOMPtr| by assigning |0| into it before destruction].
  The |nsCOMPtr| will still exercize logic in its destructor to attempt to
  |Release()| the value it has at that time.  The optimal solution is to
  arrange the lifetime of your |nsCOMPtr| to correspond to exactly how long
  you want to hold the reference. [followed by an example]

So I designed it with this specific use in mind.

Alec's comment is conservative and safe and probably the right thing to do,
given that he didn't have the fairly arcane knowledge of this particular
guarantee.  With this in mind though, and with good comments at the point of
declaration, this is probably a reasonable fix; I have used the same technique
as criteria for ordering member variables to fix ownership relationships between
documents and viewers and frames et al.  Alec: is this satisfactory?

Thanks for the exhaustive info, Scott!
I'm surprised that we can actually rely on it, but if scc is that confident, I 
must defer to him. Have we done testing on the necessary-but-tradionally-screwy 
compilers like the ones in HPUX and AIX?

I have to admit that personally I would not do this in my code because I think 
the tiny performance gain is not worth the headache that I percieve exists in 
maintaining patterns like this :)

r=nisheeth.  Once we get this super reviewed by hyatt, lets get this marked rtm+.
a=hyatt

Marking rtm+
Whiteboard: [nsbeta3-][rtm need info][fixinhand] → [nsbeta3-][rtm+][fixinhand]
PDT says: rtm++, okay to land on branch ASAP, but please do not change the
extraneous whitespace on the last line.
Whiteboard: [nsbeta3-][rtm+][fixinhand] → [nsbeta3-][rtm++][fixinhand]
Checked in the patch (sans last whitespace change) on branch & trunk.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Reopening.  Not really VERIFIED on Linux 10-18-09 branch build.  Still having 
problems in NT 10-18-08 branch build, and it also crashed once on Linux for me.  
It seems that the more you play with N6, the more likely it is to crash with a 
keyboard exit on these platforms-in other words, if you open it and close it with 
the kayboard right away, you are much, much less likely to crash.  Intermittent.  
Any thoughts?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I think you do not have my changes. You need build 10-18-14 or later...
since it's only been 5 hours since the original fix went in, there's no build
out there that has it..so re-marking FIXED for hekki :)
Status: REOPENED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
<slap>D'OH!  My bad.  Will check tomorrow.  Sorry about that. </slap>

VERIFIED on Linux 10-19-09, NT 10-19-08 branch build.  Seems to be fine.  Adding 
vtrunk KW.
Keywords: vtrunk
no crash for me on exiting using Alt-F x on the 2000102604 trunk build on win2k.
Verified Fixed on Mozilla trunk builds
win32 102704 NT 4
Setting bug to Verified and removing vtrunk keyword
Status: RESOLVED → VERIFIED
Keywords: vtrunk
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: