Closed Bug 114235 Opened 24 years ago Closed 23 years ago

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

Categories

(Core :: Layout, defect, P1)

Product:
Core
Component:
Layout
Platform:
x86
Windows 2000
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.2alpha

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

(Whiteboard: [patch])

Attachments

(2 files, 2 obsolete files)

stack traces showing the problem with nested context menus
6.30 KB, text/plain
Details
patch, v. 3
17.41 KB, patch
bzbarsky
: superreview+
Details | Diff | Splinter Review
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()
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla0.9.8
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Target Milestone: mozilla0.9.9 → Future
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, ...
Comment on attachment 90817 [details] stack traces showing the problem with nested context menus Note that the crash here is releasing the nsCOMPtr.
Priority: P2 → P1
Target Milestone: Future → mozilla1.2alpha
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.
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.
Attached patch patch, v. 2 (obsolete) — — Splinter Review
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
Attached patch patch, v. 3 — — Splinter Review
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+
Fix checked in 2002-08-06 05:48 PDT.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: