Firefox app menu contains only "Quit" in certain edgecases (e.g. starting jsconsole before the main window)

RESOLVED FIXED in Firefox 43

Status

()

Firefox
Menus
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Gijs, Assigned: smichaud)

Tracking

(Blocks: 1 bug, {regression})

38 Branch
Firefox 43
x86
Mac OS X
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
+++ This bug was initially created as a clone of Bug #1151345 +++

From bug 1151345 comment 136:

(In reply to Dan Stillman from comment #136)
> I can still reproduce this consistently in Nightly in a new empty profile
> with the following command line:
> 
> /Applications/FirefoxNightly.app/Contents/MacOS/firefox -p "Test" -jsconsole
> -purgecaches
> 
> One difference seems to be that, with -purgecaches, the Browser Console
> opens first, whereas without it the browser window opens first.
> 
> I've also gotten it to occur intermittently in other situations without
> -purgecaches (but still with -jsconsole) where the Browser Console opening
> first isn't predictive.
> 
> (We're also seeing it with standard launching in Zotero Standalone, using
> Firefox 39 as XULRunner, but since that's a more complicated situation I'll
> hold off on that until we debug this in Firefox itself. But started there
> with recent versions as well.)

I can reproduce this.
Flags: needinfo?(smichaud)
(Reporter)

Updated

2 years ago
No longer blocks: 1177785
(Assignee)

Comment 1

2 years ago
> /Applications/FirefoxNightly.app/Contents/MacOS/firefox -p "[existingprofile]" -jsconsole

This is all I need to reproduce this bug in today's mozilla-central nightly ... but only with e10s off.  With e10s on I need to add -purgecaches.

Thanks for the easy STR!

Yes, we do need to deal with these edgecases at some point, but for now I'm busy with more urgent bugs.  I hope to find time to get back to this bug in the next few weeks.

Gijs and others, please dup similar bugs to this one.
Flags: needinfo?(smichaud)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1177197

Comment 3

2 years ago
My understanding is that the problem here is that Firefox constructs the application menu based on the first window to be opened and never changes it after that. Prior to bug 1127205 Firefox didn't try to construct the application menu until a window with a menubar tag was opened, but now it does. With Dan's STR the first window to open is the JS console, which has no menubar and gets the fallback application menu.

Would it be sufficient just to replace the fallback application menu once a window with a menubar is opened? Or should this code be refactored so that the application menu actually changes when switching between windows?
(Assignee)

Comment 4

2 years ago
Simon, what you say is reasonable.  I'll keep it in mind when I get back to this.

Or would you like to work on it yourself? :-)

Updated

2 years ago
Depends on: 1186600
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1195049
(Assignee)

Comment 6

2 years ago
Created attachment 8651262 [details] [diff] [review]
Fix

After testing a bit, I believe Simon's comment #3 gets to the heart of the matter.

> Would it be sufficient just to replace the fallback application menu once
> a window with a menubar is opened?

That's what this patch does.  It seems to do the trick in my limited testing.  (It also backs out part of the patch for bug 1151345.)

I've started a set of tryserver builds.  When they're done I'll ask the people who still see this bug to test with them.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=06397e4fd63b
Assignee: nobody → smichaud
Status: NEW → ASSIGNED
Attachment #8651262 - Flags: review?(spohl.mozilla.bugs)
Comment on attachment 8651262 [details] [diff] [review]
Fix

Review of attachment 8651262 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/cocoa/nsMenuBarX.mm
@@ +32,5 @@
>  NativeMenuItemTarget* nsMenuBarX::sNativeEventTarget = nil;
>  nsMenuBarX* nsMenuBarX::sLastGeckoMenuBarPainted = nullptr; // Weak
>  nsMenuBarX* nsMenuBarX::sCurrentPaintDelayedMenuBar = nullptr; // Weak
>  NSMenu* sApplicationMenu = nil;
> +BOOL sApplicationMenuIsFallback = false;

s/false/NO/

@@ +186,5 @@
>                                                    keyEquivalent:keyStr] autorelease];
>    [quitMenuItem setTarget:nsMenuBarX::sNativeEventTarget];
>    [quitMenuItem setTag:eCommand_ID_Quit];
>    [sApplicationMenu addItem:quitMenuItem];
> +  sApplicationMenuIsFallback = true;

s/true/YES/

@@ +210,5 @@
>    NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
>  
> +  // If we've created only yet created a fallback global Application menu
> +  // (using ContructFallbackNativeMenus()), destroy it before recreating it
> +  // properly.

The beginning of this comment should be rephrased. Maybe:
"If we've only been able to create a fallback global Application menu previously (using ContructFallbackNativeMenus()), destroy it before recreating it properly."

@@ +513,5 @@
>  {
>    [sApplicationMenu removeAllItems];
>    [sApplicationMenu release];
>    sApplicationMenu = nil;
> +  sApplicationMenuIsFallback = false;

s/false/NO/
Attachment #8651262 - Flags: review?(spohl.mozilla.bugs) → review+

Comment 8

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf3c8f309134
(Assignee)

Comment 9

2 years ago
Created attachment 8651820 [details] [diff] [review]
What I landed (with Stephen's corrections)

Carrying over r+.

> s/false/NO/
> s/true/YES/

Argh :-(

> The beginning of this comment should be rephrased.

Argh again :-(  I left in an extra word.  I removed it in what I landed.
Attachment #8651820 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8651262 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/cf3c8f309134
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43

Updated

2 years ago
Blocks: 1198091
Whoever still saw this bug after the patch for bug 1151345 landed, please test with current mozilla-central nightlies.  Starting with today's m-c nightly, they contain the patch for this bug.

Comment 12

2 years ago
It's working now for me. Thanks.
You need to log in before you can comment on or make changes to this bug.