Crash in menus, but only in debug builds

RESOLVED FIXED

Status

()

--
critical
RESOLVED FIXED
14 years ago
10 years ago

People

(Reporter: aaronlev, Assigned: aaronlev)

Tracking

({crash, fixed1.8})

Trunk
x86
All
crash, fixed1.8
Points:
---
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

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

*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++
(Assignee)

Comment 1

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

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

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

Updated

14 years ago
Severity: normal → critical
(Assignee)

Comment 4

14 years ago
Created attachment 191617 [details] [diff] [review]
Some DeCOMTamination
Assignee: nobody → aaronleventhal
Status: NEW → ASSIGNED
Attachment #191617 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #191617 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Updated

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
>+    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.
Attachment #191617 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
(Assignee)

Comment 6

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

Updated

14 years ago
Attachment #191617 - Flags: review?(neil.parkwaycc.co.uk) → review?(dbaron)
(Assignee)

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

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

Updated

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

Comment 12

13 years ago
Fixed on trunk.

Updated

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

Updated

13 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Updated

13 years ago
Keywords: fixed1.8

Updated

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.