Emails from smfr: "One problem that I can see in the menu code is that keep a ref to 'gDefaultRootMenu' around, which just happens to be the first menubar we create. However, if that menubar gets destroyed, then we delete all its menu items (the nsMenu code releases its MenuRefs), but we don't remove those deleted MenuRefs from their parent menu bar In addition, it seems that the parent menu is not holding refs to submenus (as you might expect). "So, the garbage that I am seeing on 9 is related to the ::SetRootMenu(gDefaultRootMenu); on a root menu whose submenus have been deleted, but not removed from their parent menubar. I verified this by inserting a new dummy menu into this menu bar." --- "One thing I think we should definately fix is that the dtor for nsMenuX should remove its menuRef from the parent menubar; right now that doesn't happen." --- This is seen more often with non-jar builds (which smfr runs, bless his heart) since there appears to be a race condition with which menubar (hidden window or nav window gets loaded first. It's all good if the hidden window loads first, but if the nav window does, we get garbage as the menu is populated with bad menuRefs.

the os9 menu code doesn't seem to share this problem.

Also -- should we be making the Apple menu every time?

here is what this patch fixes: - no more 'root' menu that we slam into the menubar whenever a window goes away. This menu was generally wrong, and was immediately replaced by the window who got an activate event. - since we now rely solely on the activate, this also fixes the bug where if you closed a window that wasn't the frontmost, you'd get a blank menubar. - finally, since we no longer slam in a menubar that could be defunct, there is no chance for a menubar to be visible that has menus that have been removed (smfr's original garbage menubar problem). We let the menu die with the menus in it when it goes away, but patch up the case where a single menu is removed from the menubar via the DOM with a call to DeleteMenuItem(). looking for r/sr.

cc'ing the dagmeister like i told him i would.

this is now nsbranch+, please get it reviewd and on the trunk to be considered for PDT+

How well has this been tested? I'm not 100% certain of the activate event reliablility on X, so it makes me a little nervous. Of course the alternative isn't nice either.

we've been relying on the activate event to set the menu bar for months now. this is the one true path.

saari, sfraser or sdagley - do you want to review/sr this for pinkerton so we can check into the trunk for a day or so before we try to lobby this to get into the branch? I'm interested in this bug since it fixes this usability case: "- since we now rely solely on the activate, this also fixes the bug where if you closed a window that wasn't the frontmost, you'd get a blank menubar" thank you.

Comment on attachment 51294 [details] [diff] [review] fix for a bunch of things Checked out, looks good. sr=sfraser

Comment on attachment 51294 [details] [diff] [review] fix for a bunch of things r=sdagley

Wonderful! I will email PDT to inquire about letting this into the branch after a day or so on the trunk.

pls check it into the branch - PDT+

Comment on attachment 51294 [details] [diff] [review] fix for a bunch of things a=asa (on behalf of drivers) for checkin to 0.9.5.

checked into branch and trunk