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

RESOLVED FIXED

Status

()

--
major
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: bzbarsky, Assigned: jaas)

Tracking

(Blocks: 1 bug, {regression})

Trunk
x86
Mac OS X
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
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.  ;)
(Reporter)

Updated

12 years ago
Blocks: 326469
also happens in windows regular people use ;)
Severity: normal → major
Flags: blocking1.9?
Keywords: regression
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
(Assignee)

Updated

12 years ago
Blocks: 372987
(Assignee)

Comment 2

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

Updated

12 years ago
Attachment #257870 - Flags: superreview?(pavlov)
(Assignee)

Comment 4

12 years ago
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.
Attachment #257870 - Attachment is obsolete: true
Attachment #258061 - Flags: superreview?(pavlov)
Attachment #258061 - Flags: review?(mano)
Attachment #257870 - Flags: superreview?(pavlov)
Comment on attachment 258061 [details] [diff] [review]
fix v1.1

yeah, that's certainly better.

r=mano.
Attachment #258061 - Flags: review?(mano) → review+

Updated

12 years ago
Blocks: 372453
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?
Attachment #258061 - Flags: superreview?(pavlov)
Attachment #258061 - Flags: review-
Attachment #258061 - Flags: review+
(Assignee)

Comment 9

12 years ago
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/
(Assignee)

Comment 11

12 years ago
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.
Attachment #258197 - Flags: superreview?(pavlov)
Attachment #258197 - Flags: review?(joshmoz)
Attachment #258197 - Flags: review+

Comment 12

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

Comment 13

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

Updated

12 years ago
Attachment #258197 - Flags: superreview?(pavlov) → superreview+
(Assignee)

Comment 14

12 years ago
fixed on trunk
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Blocks: 333309
You need to log in before you can comment on or make changes to this bug.