102204 - Need to remove menu from its menubar when the menu goes away

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[Mike Pinkerton (not reading bugmail)]

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.

Comment 1

[Mike Pinkerton (not reading bugmail)]

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

Comment 2

[Simon Fraser [no longer active]]

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

Comment 3

[Mike Pinkerton (not reading bugmail)]

Attachment: fix for a bunch of things

Comment 4

[Mike Pinkerton (not reading bugmail)]

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.

Comment 5

[Mike Pinkerton (not reading bugmail)]

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

Comment 6

[Steve Dagley]

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

Comment 7

[saari (gone)]

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.

Comment 8

[Mike Pinkerton (not reading bugmail)]

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

Comment 9

[lchiang]

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 10

[Simon Fraser [no longer active]]

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

Checked out, looks good. sr=sfraser

Comment 11

[Steve Dagley]

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

r=sdagley

Comment 12

[lchiang]

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

Comment 13

[Jaime Rodriguez, Jr.]

pls check it into the branch - PDT+

Comment 14

[Asa Dotzler [:asa]]

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.

Comment 15

[Mike Pinkerton (not reading bugmail)]

checked into branch and trunk