menu frame's destroy can lead to access of deleted frames

RESOLVED FIXED in mozilla1.2alpha

Status

()

Core
Layout
P1
critical
RESOLVED FIXED
17 years ago
16 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

Trunk
mozilla1.2alpha
x86
Windows 2000
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

17 years ago
nsMenuFrame's destroy method can lead to access of deleted frames since it 
causes removal from the primary frame map, which should really only be done 
from the very top (the caller to destroy, by a call to DeletingFrameSubtree), 
I'd think.  Either that, or we should destroy it first.

ns_if_addref(nsIContent * 0xdddddddd) line 122 + 9 bytes
nsIFrame::GetContent(nsIContent * * 0x0012eac0) line 523 + 39 bytes
PrimaryFrameMapGetKey(PLDHashTable * 0x03045e78, PLDHashEntryHdr * 0x03b6ef28) 
line 155 + 35 bytes
ChangeTable(PLDHashTable * 0x03045e78, int -1) line 459 + 19 bytes
PL_DHashTableOperate(PLDHashTable * 0x03045e78, const void * 0x03d3b0c8, int 2) 
line 553 + 11 bytes
FrameManager::SetPrimaryFrameFor(FrameManager * const 0x03045e60, nsIContent * 
0x03d3b0c8, nsIFrame * 0x00000000) line 676 + 18 bytes
DoDeletingFrameSubtree(nsIPresContext * 0x0303ead0, nsIPresShell * 0x030450e0, 
nsIFrameManager * 0x03045e60, nsVoidArray & {...}, nsIFrame * 0x03853ad4, 
nsIFrame * 0x03d394d8) line 8900
DoDeletingFrameSubtree(nsIPresContext * 0x0303ead0, nsIPresShell * 0x030450e0, 
nsIFrameManager * 0x03045e60, nsVoidArray & {...}, nsIFrame * 0x03853ad4, 
nsIFrame * 0x03d39444) line 8948 + 29 bytes
DoDeletingFrameSubtree(nsIPresContext * 0x0303ead0, nsIPresShell * 0x030450e0, 
nsIFrameManager * 0x03045e60, nsVoidArray & {...}, nsIFrame * 0x03853ad4, 
nsIFrame * 0x03d39364) line 8948 + 29 bytes
DoDeletingFrameSubtree(nsIPresContext * 0x0303ead0, nsIPresShell * 0x030450e0, 
nsIFrameManager * 0x03045e60, nsVoidArray & {...}, nsIFrame * 0x03853ad4, 
nsIFrame * 0x03bf67f4) line 8948 + 29 bytes
DoDeletingFrameSubtree(nsIPresContext * 0x0303ead0, nsIPresShell * 0x030450e0, 
nsIFrameManager * 0x03045e60, nsVoidArray & {...}, nsIFrame * 0x03853ad4, 
nsIFrame * 0x03bf6718) line 8948 + 29 bytes
DoDeletingFrameSubtree(nsIPresContext * 0x0303ead0, nsIPresShell * 0x030450e0, 
nsIFrameManager * 0x03045e60, nsVoidArray & {...}, nsIFrame * 0x03853ad4, 
nsIFrame * 0x03853c6c) line 8948 + 29 bytes
DoDeletingFrameSubtree(nsIPresContext * 0x0303ead0, nsIPresShell * 0x030450e0, 
nsIFrameManager * 0x03045e60, nsVoidArray & {...}, nsIFrame * 0x03853ad4, 
nsIFrame * 0x03853ba4) line 8948 + 29 bytes
DoDeletingFrameSubtree(nsIPresContext * 0x0303ead0, nsIPresShell * 0x030450e0, 
nsIFrameManager * 0x03045e60, nsVoidArray & {...}, nsIFrame * 0x03853ad4, 
nsIFrame * 0x03853ad4) line 8948 + 29 bytes
DeletingFrameSubtree(nsIPresContext * 0x0303ead0, nsIPresShell * 0x030450e0, 
nsIFrameManager * 0x03045e60, nsIFrame * 0x03853ad4) line 8985 + 29 bytes
nsCSSFrameConstructor::RemoveMappingsForFrameSubtree(nsCSSFrameConstructor * 
const 0x0303fb00, nsIPresContext * 0x0303ead0, nsIFrame * 0x03853ad4, 
nsILayoutHistoryState * 0x00000000) line 9056 + 31 bytes
nsMenuFrame::Destroy(nsMenuFrame * const 0x03bcc8dc, nsIPresContext * 
0x0303ead0) line 338
nsFrameList::DestroyFrames(nsIPresContext * 0x0303ead0) line 131
nsContainerFrame::Destroy(nsContainerFrame * const 0x03bb93e4, nsIPresContext * 
0x0303ead0) line 140
nsBoxFrame::Destroy(nsBoxFrame * const 0x03bb93e4, nsIPresContext * 0x0303ead0) 
line 1180 + 13 bytes
nsFrameList::DestroyFrames(nsIPresContext * 0x0303ead0) line 131
nsContainerFrame::Destroy(nsContainerFrame * const 0x03bb92dc, nsIPresContext * 
0x0303ead0) line 140
nsBoxFrame::Destroy(nsBoxFrame * const 0x03bb92dc, nsIPresContext * 0x0303ead0) 
line 1180 + 13 bytes
nsFrameList::DestroyFrames(nsIPresContext * 0x0303ead0) line 131
nsContainerFrame::Destroy(nsContainerFrame * const 0x03b62c64, nsIPresContext * 
0x0303ead0) line 140
nsBoxFrame::Destroy(nsBoxFrame * const 0x03b62c64, nsIPresContext * 0x0303ead0) 
line 1180 + 13 bytes
nsFrameList::DestroyFrames(nsIPresContext * 0x0303ead0) line 131
nsContainerFrame::Destroy(nsContainerFrame * const 0x03b62bd0, nsIPresContext * 
0x0303ead0) line 140
nsBoxFrame::Destroy(nsBoxFrame * const 0x03b62bd0, nsIPresContext * 0x0303ead0) 
line 1180 + 13 bytes
nsFrameList::DestroyFrames(nsIPresContext * 0x0303ead0) line 131
nsContainerFrame::Destroy(nsContainerFrame * const 0x03a2dd5c, nsIPresContext * 
0x0303ead0) line 140
nsBoxFrame::Destroy(nsBoxFrame * const 0x03a2dd5c, nsIPresContext * 0x0303ead0) 
line 1180 + 13 bytes
nsMenuPopupFrame::Destroy(nsMenuPopupFrame * const 0x03a2dd5c, nsIPresContext * 
0x0303ead0) line 1795
nsFrameList::DestroyFrames(nsIPresContext * 0x0303ead0) line 131
nsMenuFrame::Destroy(nsMenuFrame * const 0x0373b7f0, nsIPresContext * 
0x0303ead0) line 345
nsFrameList::DestroyFrames(nsIPresContext * 0x0303ead0) line 131
nsContainerFrame::Destroy(nsContainerFrame * const 0x036f7294, nsIPresContext * 
0x0303ead0) line 140
nsBoxFrame::Destroy(nsBoxFrame * const 0x036f7294, nsIPresContext * 0x0303ead0) 
line 1180 + 13 bytes
nsFrameList::DestroyFrames(nsIPresContext * 0x0303ead0) line 131
nsContainerFrame::Destroy(nsContainerFrame * const 0x036b6ea8, nsIPresContext * 
0x0303ead0) line 140
nsBoxFrame::Destroy(nsBoxFrame * const 0x036b6ea8, nsIPresContext * 0x0303ead0) 
line 1180 + 13 bytes
nsFrameList::DestroyFrames(nsIPresContext * 0x0303ead0) line 131
nsContainerFrame::Destroy(nsContainerFrame * const 0x03692dfc, nsIPresContext * 
0x0303ead0) line 140
nsBoxFrame::Destroy(nsBoxFrame * const 0x03692dfc, nsIPresContext * 0x0303ead0) 
line 1180 + 13 bytes
nsFrameList::DestroyFrames(nsIPresContext * 0x0303ead0) line 131
nsContainerFrame::Destroy(nsContainerFrame * const 0x03692d14, nsIPresContext * 
0x0303ead0) line 140
nsBoxFrame::Destroy(nsBoxFrame * const 0x03692d14, nsIPresContext * 0x0303ead0) 
line 1180 + 13 bytes
nsFrameList::DestroyFrames(nsIPresContext * 0x0303ead0) line 131
nsContainerFrame::Destroy(nsContainerFrame * const 0x0334aa58, nsIPresContext * 
0x0303ead0) line 140
nsBoxFrame::Destroy(nsBoxFrame * const 0x0334aa58, nsIPresContext * 0x0303ead0) 
line 1180 + 13 bytes
nsFrameList::DestroyFrames(nsIPresContext * 0x0303ead0) line 131
nsContainerFrame::Destroy(nsContainerFrame * const 0x0334a758, nsIPresContext * 
0x0303ead0) line 140
nsBoxFrame::Destroy(nsBoxFrame * const 0x0334a758, nsIPresContext * 0x0303ead0) 
line 1180 + 13 bytes
nsFrameList::DestroyFrames(nsIPresContext * 0x0303ead0) line 131
nsContainerFrame::Destroy(nsContainerFrame * const 0x0334a71c, nsIPresContext * 
0x0303ead0) line 140
ViewportFrame::Destroy(ViewportFrame * const 0x0334a71c, nsIPresContext * 
0x0303ead0) line 157
FrameManager::Destroy(FrameManager * const 0x03045e60) line 508
PresShell::Destroy(PresShell * const 0x030450e0) line 1697
DocumentViewerImpl::Destroy(DocumentViewerImpl * const 0x0227ddb8) line 1335
nsDocShell::Destroy(nsDocShell * const 0x022924b4) line 2477
nsWebShell::Destroy(nsWebShell * const 0x022924b4) line 1424
nsXULWindow::Destroy(nsXULWindow * const 0x021f38cc) line 365
nsWebShellWindow::Destroy(nsWebShellWindow * const 0x021f38cc) line 1760 + 9 
bytes
nsWebShellWindow::Close(nsWebShellWindow * const 0x021f3938) line 389
nsWebShellWindow::HandleEvent(nsGUIEvent * 0x0012f598) line 464
nsWindow::DispatchEvent(nsWindow * const 0x014b202c, nsGUIEvent * 0x0012f598, 
nsEventStatus & nsEventStatus_eIgnore) line 846 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f598) line 867
nsWindow::DispatchStandardEvent(unsigned int 101) line 887 + 15 bytes
nsWindow::ProcessMessage(unsigned int 16, unsigned int 0, long 0, long * 
0x0012f980) line 3178
nsWindow::WindowProc(HWND__ * 0x00f704bc, unsigned int 16, unsigned int 0, long 
0) line 1114 + 27 bytes
USER32! 77e13eb0()
USER32! 77e1591b()
USER32! 77e1595d()
NTDLL! 77f9fb83()
USER32! 77e169a7()
USER32! 77e13eb0()
USER32! 77e16469()
USER32! 77e1a6f8()
nsWindow::WindowProc(HWND__ * 0x00f704bc, unsigned int 274, unsigned int 61536, 
long 1901786) line 1121 + 31 bytes
USER32! 77e13eb0()
USER32! 77e1591b()
USER32! 77e1595d()
NTDLL! 77f9fb83()
USER32! 77e169a7()
USER32! 77e13eb0()
USER32! 77e16469()
USER32! 77e1a6f8()
nsWindow::WindowProc(HWND__ * 0x00f704bc, unsigned int 161, unsigned int 20, 
long 1901786) line 1121 + 31 bytes
USER32! 77e13eb0()
USER32! 77e1401a()
USER32! 77e192da()
nsAppShellService::Run(nsAppShellService * const 0x0143ecd8) line 303
main1(int 3, char * * 0x003c6e20, nsISupports * 0x00000000) line 1269 + 32 bytes
main(int 3, char * * 0x003c6e20) line 1599 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e87903()
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla0.9.8
(Assignee)

Updated

17 years ago
Blocks: 114219
(Assignee)

Updated

17 years ago
Target Milestone: mozilla0.9.8 → mozilla0.9.9
(Assignee)

Updated

16 years ago
Target Milestone: mozilla0.9.9 → Future
(Assignee)

Comment 1

16 years ago
I'm seeing what's probably the same bug, with nested context menus:  load
http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey , right click on the
link in the ports iframe, *click* the submenu, and then click in the blank area
of the page (or use one of the popup iframes, and add the step of pressing
escape to the end).  That crashes here:

#8  0x429af820 in nsMenuPopupFrame::DismissChain (this=0x8d37710)
    at /builds/trunk/mozilla/layout/xul/base/src/nsMenuPopupFrame.cpp:1912
#9  0x429be17c in nsMenuDismissalListener::Rollup (this=0x8d38370)
---Type <return> to continue, or q <return> to quit---
    at
/builds/trunk/mozilla/layout/xul/base/src/nsMenuDismissalListener.cpp:106#10
0x40ecf1c8 in nsWindow::OnButtonPressSignal (this=0x899c540, 
    aGdkButtonEvent=0x8491878)
    at /builds/trunk/mozilla/widget/src/gtk/nsWindow.cpp:1601
#11 0x40ecf5bb in nsWindow::HandleGDKEvent (this=0x899c540, event=0x8491878)
    at /builds/trunk/mozilla/widget/src/gtk/nsWindow.cpp:1714
#12 0x40ec0e1e in dispatch_superwin_event (event=0x8491878, window=0x899c540)
    at /builds/trunk/mozilla/widget/src/gtk/nsGtkEventHandler.cpp:958
#13 0x40ec09d4 in handle_gdk_event (event=0x8491878, data=0x0)
    at /builds/trunk/mozilla/widget/src/gtk/nsGtkEventHandler.cpp:816

(gdb) frame 8
#8  0x429af820 in nsMenuPopupFrame::DismissChain (this=0x8d37710)
    at /builds/trunk/mozilla/layout/xul/base/src/nsMenuPopupFrame.cpp:1912
1912
      menuParent->DismissChain();
(gdb) p *this
$4 = {<nsBoxFrame> = {<nsContainerFrame> = {<nsSplittableFrame> = {<nsFrame> =
{<nsIFrame> = {<nsISupports> = {_vptr.nsISupports = 0x0}, mRect = {
              x = -572662307, y = -572662307, width = -572662307, 
              height = -572662307}, mContent = 0xdddddddd, 
            mStyleContext = 0xdddddddd, mParent = 0xdddddddd, 
            mNextSibling = 0xdddddddd, 
...
(Assignee)

Comment 2

16 years ago
Created attachment 90817 [details]
stack traces showing the problem with nested context menus
(Assignee)

Comment 3

16 years ago
Comment on attachment 90817 [details]
stack traces showing the problem with nested context menus

Note that the crash here is releasing the nsCOMPtr.
(Assignee)

Updated

16 years ago
Priority: P2 → P1
Target Milestone: Future → mozilla1.2alpha
(Assignee)

Updated

16 years ago
Whiteboard: [patch]
Comment on attachment 90822 [details] [diff] [review]
patch

>@@ -1918,9 +1918,11 @@
>   nsCOMPtr<nsIAtom> tag;
>   nsresult          rv;
> 
>-  nsCOMPtr<nsIMenuParent> menuPar(do_QueryInterface(aFrameList));
>+  nsIMenuParent *menuPar;
>+  CallQueryInterface(aFrameList, &menuPar);
>   if (menuPar) {
>-    nsCOMPtr<nsIBox> menupopup(do_QueryInterface(aFrameList));
>+    nsIBox *menupopup;
>+    CallQueryInterface(aFrameList, &menupopup);
>     NS_ASSERTION(menupopup,"Popup is not a box!!!");
>     menupopup->SetParentBox(this);
>     mPopupFrames.InsertFrames(nsnull, nsnull, aFrameList);


I think we should be consistent about whether nsIBox objects should be
refcounted.  There are other places that your patch doesn't touch where
nsCOMPtr's are used for nsIBox.  If there are no implementations of nsIBox that
_are_ refcounted, I think we should make this change globally.
(Assignee)

Comment 6

16 years ago
The things that inherit from nsIBox are:

    nsIBox
      |
     nsBox
     /  \ `---------------------------------.
    /    `-------------------.               \
nsBoxToBlockAdaptor   nsContainerBox      nsLeafBoxFrame
                             |
                        nsBoxFrame

Everything here (and the derived frames) has AddRef and Release as no-ops.

I'll attach a new patch that makes the same changes for nsIBox.
(Assignee)

Comment 7

16 years ago
Created attachment 93950 [details] [diff] [review]
patch, v. 2

Make all the changes for nsIBox as well, rather than just some.
Attachment #90822 - Attachment is obsolete: true
Comment on attachment 93950 [details] [diff] [review]
patch, v. 2

r/sr=bryner
Attachment #93950 - Flags: review+
In xul/base/src/nsMenuFrame.cpp

@@ -1931,9 +1931,11 @@
-  nsCOMPtr<nsIMenuParent> menuPar(do_QueryInterface(aFrameList));
+  nsIMenuParent *menuPar;
+  CallQueryInterface(aFrameList, &menuPar);

Are we guaranteed that aFrameList is non-null?

@@ -1960,9 +1962,11 @@

Same thing.

In xul/base/src/nsPopupSetFrame.cpp

@@ -438,7 +440,8 @@
-    nsCOMPtr<nsIMenuParent> childPopup = do_QueryInterface(activeChild);
+    nsIMenuParent* childPopup;
+    CallQueryInterface(activeChild, &childPopup);

Same thing for activeChild.

If those are safe, sr=bzbarsky

(Assignee)

Comment 10

16 years ago
Created attachment 93959 [details] [diff] [review]
patch, v. 3

Adds the three null-checks that bz pointed out.
Attachment #93950 - Attachment is obsolete: true
Comment on attachment 93959 [details] [diff] [review]
patch, v. 3

sr=bzbarsky
Attachment #93959 - Flags: superreview+
(Assignee)

Comment 12

16 years ago
Fix checked in 2002-08-06 05:48 PDT.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.