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

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[:Gijs (he/him)]

+++ 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.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

> /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.

Comment 3

[Simon Kornblith]

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?

Comment 4

[Steven Michaud [:smichaud] (Retired)]

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? :-)

Comment 6

[Steven Michaud [:smichaud] (Retired)]

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

Comment 7

[Stephen A Pohl [:spohl]]

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/

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf3c8f309134

Comment 9

[Steven Michaud [:smichaud] (Retired)]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cf3c8f309134

Comment 11

[Steven Michaud [:smichaud] (Retired)]

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

[Dan Stillman]

It's working now for me. Thanks.