Crash in menus, but only in debug builds




14 years ago
10 years ago


(Reporter: aaronlev, Assigned: aaronlev)


({crash, fixed1.8})

crash, fixed1.8
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)



(1 attachment)



14 years ago
This crash does not happen in release builds on either Windows or Linux.

1) Click on a toolbar bookmark folder, 
2) right click on an entry,
3) press escape


It crashes because the menulistener caller to Escape gets destroyed. Then there
is nothing to return to. Perhaps we need a kungFuDeathGrip?

This is the stack that shows the nsMenuListener both calling and getting
destroyed before we can return to it.

 	gklayout.dll!nsMenuListener::~nsMenuListener()  Line 86	C++
 	gklayout.dll!nsMenuListener::`scalar deleting destructor'()  + 0xf	C++
 	gklayout.dll!nsMenuListener::Release()  Line 72 + 0x97	C++
 	gklayout.dll!nsMenuPopupFrame::RemoveKeyboardNavigator()  Line 1917 + 0x1b	C++
>	gklayout.dll!nsPopupSetFrame::OpenPopup(nsPopupFrameList * aEntry=0x03a31b88,
int aActivateFlag=0x00000000)  Line 529	C++
 	gklayout.dll!nsPopupSetFrame::DestroyPopup(nsIFrame * aPopup=0x039073dc, int
aDestroyEntireChain=0x00000000)  Line 442	C++
 	gklayout.dll!nsMenuPopupFrame::Escape(int & aHandledFlag=0x00000000)  Line
1422	C++
 	gklayout.dll!nsMenuListener::KeyPress(nsIDOMEvent * aKeyEvent=0x03a4e7d0) 
Line 190	C++

Comment 1

14 years ago
All platforms -- was tested by folks on IRC.
OS: Windows XP → All

Comment 2

14 years ago
I was wrong it's not destroying |this|, that's a different nsMenuListener.

One strange thing is that the Tab key destroys the parent menu but not the
current menu.

Comment 3

14 years ago
I crashes in |nsMenuPopupFrame::Escape()| when |nsCOMPtr<nsIMenuParent>
contextMenu| goes out of scope, because the vptr for it is 0xdddddddd so it
can't ->Release()


14 years ago
Severity: normal → critical

Comment 4

14 years ago
Created attachment 191617 [details] [diff] [review]
Some DeCOMTamination
Assignee: nobody → aaronleventhal
Attachment #191617 - Flags: superreview?(
Attachment #191617 - Flags: review?(


14 years ago
Flags: blocking1.8b4?

Comment 5

14 years ago
Comment on attachment 191617 [details] [diff] [review]
Some DeCOMTamination

Please get a module peer to review.

>+  virtual nsIMenuFrame *GetCurrentMenuItem() = 0;
That's annoying, because both implementations look so similar.

>+    // Prevents us from getting destroyed by Escape(), we need to return to ourselves
> 	  mMenuParent->Escape(handled);
>     if (!handled)
>       mMenuParent->DismissChain();
Surely "this" will now destroyed when you call NS_RELEASE_THIS(); so you
probably need to call DismissChain first. sr=me with this fixed, if this is
still needed - on irc you (also) mentioned a crash in an
nsCOMPtr<nsIMenuParent> destructor - if this fixes that (too) please could you
point out which nsCOMPtr for completeness.

>+    if (grandparent ) {
Nit: no space before ) please.
Attachment #191617 - Flags: superreview?( → superreview+

Comment 6

14 years ago
Oops, the NS_RELEASE_THIS/NS_ADDREF_THIS was a mistake from debugging. 


14 years ago
Attachment #191617 - Flags: review?( → review?(dbaron)

Comment 7

14 years ago
David, the NS_ADDREF_THIS/NS_RELEASE_THIS will be removed before I check in.
That was not supposed to be part of the patch.

The basic problem was that these menu objects are frames and shouldn't be used
with com ptrs. If the object gets destroyed and then the comptr goes out of
scope it crashes.

Comment 8

14 years ago
not blocking the release on debug only problem but we'd certainly approve a
patch if the risk is low to the release builds.
Flags: blocking1.8b4? → blocking1.8b4+

Comment 9

14 years ago
(In reply to comment #8)
> not blocking the release on debug only problem but we'd certainly approve a
> patch if the risk is low to the release builds.

I'm not really sure if it never crashes in release. It should crash -- I'm not
sure why it doesn't.
In nsMenuFrame.h, nsMenuBarFrame.h, and nsMenuPopupFrame.h, please mark the
methods that are virtual as such (be careful in the last).

Likewise, in nsMenuPopupFrame.cpp and nsMenuBarFrame.cpp, add "/* virtual */"
before the type in the functions that are virtual.

In nsMenuBarFrame.h, you missed removing the "aResult" in
> +  nsIMenuFrame* GetNextMenuItem(nsIMenuFrame* aStartaResult);

In nsMenuBarFrame::FindMenuWithShortcut, you should probably check the result of
CallQueryInterface and assign null if it fails, i.e.,
  if (NS_FAILED(CallQueryInterface(..., &menuFrame)))
    menuFrame = nsnull;

Same in nsMenuBarFrame::GetNextMenuItem, twice, and twice in GetPreviousMenuItem.

In nsMenuFrame::SetActiveChild, you should check NS_FAILED(CallQ... instead of
checking menuFrame (right after the two lines you change).

I'm up to nsMenuListener.cpp.
(In reply to comment #10)
> Same in nsMenuBarFrame::GetNextMenuItem, twice, and twice in GetPreviousMenuItem.

And likewise for the same methods in nsMenuPopupFrame.

nsMenuPopupFrame::FindMenuWithShortcut should check the nsresult of the QI
instead of null-checking the out param (in two places).

Likewise for nsMenuPopupFrame::HideChain (but be careful, it's negated!).

In nsMenuPopupFrame::Notify, removing the null-initialization of currentMenuItem
seems dangerous, as does removing the initial null-check (implied by
do_QueryInterface) on child.  It seems like the while loop condition should be
while (child && NS_SUCCEEDED(CallQueryInterface(child, &menuParent))) and the
null-assignment inside should be to child instead of menuParent.  This would
allow condensing two chunks outside and inside the loop.

nsPopupSetFrame::HidePopup should check the nsresult instead of a null-check, as
should DestroyPopup.

With those changes, r=dbaron.


13 years ago
Attachment #191617 - Flags: approval1.8b4?

Comment 12

13 years ago
Fixed on trunk.


13 years ago
Attachment #191617 - Flags: approval1.8b4? → approval1.8b4+


13 years ago
Last Resolved: 13 years ago
Resolution: --- → FIXED


13 years ago
Keywords: fixed1.8


10 years ago
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: xptoolkit.menus → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.