Pavel Kankovsky Reporter
	Description • 19 years ago

User-Agent:       Mozilla/5.0 (Mozilla rulez!)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.1) Gecko/20060221 SUSE/1.5.0.1-11 Firefox/1.5.0.1

The browser crashes when a silly XUL file with a <toolbarbutton type="menu"> containing <menubar> is loaded and the silly toolbarbutton is clicked.

Reproducible: Always

Steps to Reproduce:
1. Open the attached XUL.
2. Click on the "KILL" button.

Actual Results:  
The browser went down in flames.

Expected Results:  
Who knows? But I am sure it should not kill the browser.

My private build of Mozilla 1.7.12 crashes as well. According to gdb, it segfaults in nsMenuFrame::OpenMenuInternal() after a call to nsIFrame::GetView(), probably at "nsIViewManager* vm = view->GetViewManager();".

	Martijn Wargers (dead)
	Comment 2 • 19 years ago

Confirmed, using 2006-03-13 trunk build on winxp.

	Martijn Wargers (dead)
	Comment 3 • 19 years ago

Talkback ID: TB16358220Y
nsMenuFrame::DoLayout   nsIFrame::Layout  

	Nickolay_Ponomarev Assignee
	Comment 5 • 19 years ago

So as I understand it, the problem is that both nsMenuPopupFrame and nsMenuBarFrame implement nsIMenuParent, but only the former can be used as a popup. The code in nsMenuFrame::SetInitialChildList assumes that if a child QIs to nsIMenuParent, it's a popup. This crashes later because the nsMenuBarFrame doesn't have a view.

The suggested fix is to check whether the class implementing nsIMenuParent is a menubar by asking it. I don't check the rv because it's always NS_OK and nsIMenuParent should be deCOMtaminated anyways.

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 19 years ago

Comment on attachment 215601 [details] [diff] [review]
menubar is not a menupopup

Nice catch!  r+sr=bzbarsky

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 7 • 19 years ago

mozilla/layout/xul/base/src/nsMenuFrame.cpp; new revision: 1.314;

	Stephen Donner [:stephend] Not actively reading bugmail
	Comment 8 • 19 years ago

Verified FIXED using trunk build SeaMonkey 1.5a;Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060324 SeaMonkey/1.5a

(2006-03-34-21, that is.)

	Stephen Donner [:stephend] Not actively reading bugmail
	Comment 9 • 19 years ago

(In reply to comment #8)
> Verified FIXED using trunk build SeaMonkey 1.5a;Mozilla/5.0 (Windows; U;
> Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060324 SeaMonkey/1.5a
> 
> (2006-03-34-21, that is.)

Sorry, typo: 2006-03-24-21.

	Martijn Wargers (dead)
	Comment 10 • 18 years ago

Maybe the patch is something for 1.8.1 branch?

	Nickolay_Ponomarev Assignee
	Comment 11 • 18 years ago

The patch is rather safe, yes. I didn't try to get it on the branch because I thought few people would be evil enough to put a menubar inside a toolbarbutton ;)
If you think this should go on branch, feel free to request approval for me.

	Martijn Wargers (dead)
	Comment 12 • 18 years ago

Comment on attachment 215601 [details] [diff] [review]
menubar is not a menupopup

Well, better safe than sorry, not?

	Mike Schroepfer
	Comment 13 • 18 years ago

Comment on attachment 215601 [details] [diff] [review]
menubar is not a menupopup

181 is closed for RC1.

	Jay Patel [:jay]
	Comment 14 • 18 years ago

Comment on attachment 215601 [details] [diff] [review]
menubar is not a menupopup

Approved for 1.8.0 & 1.8.1 branches, a=Jay for drivers. Please land asap. Thanks!

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 18 years ago

Fixed for 1.8.1.1, 1.8.0.9.

	Jay Patel [:jay]
	Comment 16 • 18 years ago

v.fixed on 1.8.0 and 1.8.1 branches with 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.9pre) Gecko/20061128 Firefox/1.5.0.9pre 
and 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.1pre) Gecko/20061128 BonEcho/2.0.0.1pre

No crashes with XUL testcase (was crashing FF2.0).