STEPS TO REPRODUCE: 1) Build with the layout-debug extension 2) Start "firefox -layoutdebug" 3) Try to quit via Cmd-Q or the menu. EXPECTED RESULTS: Quit ACTUAL RESULTS: ###!!! ASSERTION: Carbon event for menu hit not sent!: 'err == noErr', file ../../../../mozilla/widget/src/cocoa/nsMenuBarX.mm, line 1115 and nothing else happens. If I close the window first, _then_ try quitting, it works. That's not really Mac-cool, so to say. ;)
also happens in windows regular people use ;)
Severity: normal → major
Summary: Quitting the layout debugger doesn't work → Quit menu-item/Cmd+Q is broken in windows which don't have their own menu_FileQuitItem set
Created attachment 257870 [details] [diff] [review] fix v1.0 The problem is we're saving off the results of a content node search for every window/document when we're looking for pref and quit nodes. This is a mistake I made when implementing menus a long time ago. We should be saving the pref and quit nodes from the hidden window and falling back on those when the current window/document does not have them.
Attachment #257870 - Flags: review?(mano)
Comment on attachment 257870 [details] [diff] [review] fix v1.0 >Index: nsMenuBarX.mm >=================================================================== >RCS file: /cvsroot/mozilla/widget/src/cocoa/nsMenuBarX.mm,v >retrieving revision 1.40 >diff -U8 -p -r1.40 nsMenuBarX.mm >--- nsMenuBarX.mm 19 Dec 2006 19:26:41 -0000 1.40 >+++ nsMenuBarX.mm 8 Mar 2007 22:34:13 -0000 >@@ -70,25 +70,30 @@ NS_IMPL_ISUPPORTS6(nsMenuBarX, nsIMenuBa > nsIChangeManager, nsIMenuCommandDispatcher, nsISupportsWeakReference) > > EventHandlerUPP nsMenuBarX::sCommandEventHandler = nsnull; > NativeMenuItemTarget* nsMenuBarX::sNativeEventTarget = nil; > NSWindow* nsMenuBarX::sEventTargetWindow = nil; > NSMenu* sApplicationMenu = nil; > BOOL gSomeMenuBarPainted = NO; > >+// We keep references to the first quit and pref item content nodes we find, which >+// will be from the hidden window. We use these when the document for the current >+// window does not have a quit or pref item. >+static nsIContent* sQuitItemContent = nsnull; >+static nsIContent* sPrefItemContent = nsnull; Add a comment here explaining why a strong ref isn't necessary. r=mano only because carbon code sucked in the same manner.
Attachment #257870 - Flags: review?(mano) → review+
Created attachment 258061 [details] [diff] [review] fix v1.1 This fixes some comments and provides fallback to native quit if there is no quit content provided.
Comment on attachment 258061 [details] [diff] [review] fix v1.1 yeah, that's certainly better. r=mano.
Attachment #258061 - Flags: review?(mano) → review+
Comment on attachment 258061 [details] [diff] [review] fix v1.1 See bug 372453 - if there's no hidden window, you're taking the using the menuitems from the first window which has them, and when that window is closed you're left with dead pointers. Could you make these static members of nsMenuBarX and invalidate them from nsMenuItemX's destructor?
Or just from nsMenuBarX itself.
Created attachment 258197 [details] [diff] [review] like this
When would we not have a hidden window? I think we're in much more trouble if that happens...
In XULRunner, see the bug i've linked you too. And even if you do have a hidden window, it might not have these items/
Comment on attachment 258197 [details] [diff] [review] like this That change is fine with me. However, I don't think that XULRunner app should be running without a hidden window. And if somehow it is doing that, then closing the window should shut down the app.
Created attachment 258323 [details] Crash stack after patch Mano asked on the dep bug about whether or not the patch helps. It no longer crashes in the same place, but now it ends up crashing in nsAppShell, see the attached stack. I also still can't quit the app by using the quit menu even when there are windows present (it's just a no-op in that case). Note that the app I'm using doesn't ship hiddenWindow.xul, which is what the promptservice's alert is about in that stack. This may also otherwise affect the quit behaviour, but I don't know mac widget code and I currently don't have the cycles to go and explore it all. Maybe this can give a clue. If you need specific info, let me know.
I don't think that crash is related to this bug. This may change the stack, but that is still a different bug.
Attachment #258197 - Flags: superreview?(pavlov) → superreview+
fixed on trunk
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.