303404 - Crash in menus, but only in debug builds

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Aaron Leventhal]

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

*crash*

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

[Aaron Leventhal]

All platforms -- was tested by folks on IRC.

Comment 2

[Aaron Leventhal]

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

[Aaron Leventhal]

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()

Comment 4

[Aaron Leventhal]

Attachment: Some DeCOMTamination

Comment 5

[[e-mail]]

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
>+    NS_ADDREF_THIS();
> 	  mMenuParent->Escape(handled);
>+    NS_RELEASE_THIS();
>     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.

Comment 6

[Aaron Leventhal]

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

Comment 7

[Aaron Leventhal]

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

[Asa Dotzler [:asa]]

not blocking the release on debug only problem but we'd certainly approve a
patch if the risk is low to the release builds.

Comment 9

[Aaron Leventhal]

(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.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 12

[Aaron Leventhal]

Fixed on trunk.