368911 - Quit menu-item/Cmd+Q is broken in windows which don't have their own menu_FileQuitItem set

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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.  ;)

Comment 1

[Asaf Romano (gone)]

also happens in windows regular people use ;)

Comment 2

[Josh Aas]

Attachment: 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.

Comment 3

[Asaf Romano (gone)]

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.

Comment 4

[Josh Aas]

Attachment: fix v1.1

This fixes some comments and provides fallback to native quit if there is no quit content provided.

Comment 5

[Asaf Romano (gone)]

Comment on attachment 258061 [details] [diff] [review]
fix v1.1

yeah, that's certainly better.

r=mano.

Comment 6

[Asaf Romano (gone)]

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?

Comment 7

[Asaf Romano (gone)]

Or just from nsMenuBarX itself.

Comment 8

[Asaf Romano (gone)]

Attachment: like this

Comment 9

[Josh Aas]

When would we not have a hidden window? I think we're in much more trouble if that happens...

Comment 10

[Asaf Romano (gone)]

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 11

[Josh Aas]

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.

Comment 12

[:Gijs (he/him)]

Attachment: 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.

Comment 13

[Josh Aas]

I don't think that crash is related to this bug. This may change the stack, but that is still a different bug.

Comment 14

[Josh Aas]

fixed on trunk