Closed Bug 344249 Opened 18 years ago Closed 18 years ago

Menu key commands processed twice after loading Java


(Core Graveyard :: Java: OJI, defect)

1.8 Branch
Not set


(Not tracked)



(Reporter: mark, Assigned: mark)



(Keywords: fixed1.8.1, verified1.8.0.5)


(2 files, 7 obsolete files)

After loading Java on the Mac via JEP, key equivalents for menu commands may be processed twice.  This can occur on any branch, but is guaranteed to occur now on the trunk after the patch for bug 50590.

Steps to reproduce:
1. Open a browser window
2. Where bug 50590 has not yet landed (1.8.0 and 1.8[.1] branches),
   open a menu to ensure it's built.  For this example, open the File menu.
   It is not important to select any command from the menu, just close the
   menu once it's open.
3. Load a Java applet, such as
4. Use key equivalents corresponding to the menu in step 2: press command-T
   to open a new tab.  Press command-W to close.

Expected behavior: each menu command key equivalent should only cause a single action to be taken.

Observed behavior: menu command key equivalents cause the action to be taken twice.

The action is performed once as a result of the menu command being selected, and once again on a key handler.  Normally, once a menu is constructed, we only take action on menu commands, and we don't receive key events at all.  Loading Java is causing us to take key events as well.

We may be able to fix this by not installing key event handlers for menu commands, now that we reliably have constructed menus available at all times.  Even so, loading Java should not suddenly make these key events available to the browser.

On the current trunk, and anywhere else the patch to bug 50590 lands, the steps to reproduce are much more straightforward.  That patch changed things to ensure that menus are always constructed, without the need to open the menu first.
I've confirmed this with Firefox plus JEP 0.9.5+f and JEP
0.9.5+e, running on OS X 10.3.9.  JEP 0.9.5+d doesn't have the
problem, so I guess the bug was introduced with JEP 0.9.5+e.

It's really too bad nobody (including me) noticed this earlier.  But
that may mean that it doesn't often get triggered.  I usually use
keyboard shortcuts instead of the menu ... which is my excuse for not
having seen this :-(
JEP should be fixed anyway, but...

This adds a layer of security by allowing widget to disable the key handlers for any commands that go into the menu system.  The key commands are extraneous because we now expect only to take these commands via nsMenuBarX::CommandEventHandler, and when everything is working properly, we won't take any of these commands through the XBL handlers.
Assignee: nobody → mark
Attachment #228856 - Flags: review?(joshmoz)
Comment on attachment 228856 [details] [diff] [review]
Add XBL disabled-hard attribute, use for disabling commands attached to menus

Looks fine to me, but I think we should come up with a better name than "disabledHard". Also a comment about that attribute outside of widget code would be nice, so its easy to find out what its for.
Attachment #228856 - Flags: review?(joshmoz) → review+
Comment on attachment 228856 [details] [diff] [review]
Add XBL disabled-hard attribute, use for disabling commands attached to menus

For the attribute, how about "no-process?"  I can add a brief comment to nsGkAtomList.h describing the use of this attribute to disable XBL commands without touching "disabled" and causing problems for things that monitor that attribute.
Attachment #228856 - Flags: superreview?(bzbarsky)
Attachment #228856 - Flags: superreview?(bzbarsky) → superreview?(bryner)
adding more more people who might care about this.

Mark, I'm not sure I understand your explanation completely -- why do the XBL handlers get fired in this case?
On the Mac, with menus functioning properly, we don't handle key equivalents on keypress handlers.  We handle them directly through the menu system.  When the user presses command-N, we don't want to see a keypress for command-N.  The system's menu manager picks up the keypress and generates the same menu command event that would have been generated if the user had used the mouse to select File:New.

On other platforms, the XBL handlers for these same keypress events are called as a result of receiving native keypress events, which are turned into Gecko keypress events.  On the Mac, we don't normally see these native keypress events, because once we set a key equivalent for a native menu item, we stop receiving native keypress events for that keystroke.  That's what usually keeps key equivalents from causing two actions to occur.  This problem with the JEP shows that it's possible for us to receive both a native menu command event AND the native keypress event.

Because we always expect to have the native menus synced up with the keypress handlers that they replace, the handlers that run for a keypress should never be called.  This patch allows us to disable those handlers so that in the event that they are called erroneously, they won't have any harmful effects.
So shouldn't the handlers just be #ifdef whatever the relevant thing is?

Alternately, should the window handler thing obey the "disabled" attr?
(In reply to comment #7)
> So shouldn't the handlers just be #ifdef whatever the relevant thing is?

No, we need the command handlers - we pull the key equivalents out of them and use them to build the native menu items, and continue to monitor them for changes to update the native menu items as necessary.

> Alternately, should the window handler thing obey the "disabled" attr?

It does.  That's why we need a new attribute.  We watch the "disabled" attribute on the command elements with an nsIChangeObserver so that we can update the native menu item's state as the command is enabled and disabled when AttributeChanged is called.  Because we need to observe this change, we can't just set the command's existing "disabled" attribute, because then we won't get a change notification when something really does need to disable the command.
Hm, so it seems like the menu code could just have an event listener that calls preventDefault() on events that shouldn't fire the key handlers...
bryner's suggestion doesn't work.  What I tried is in this patch.  HandleEvent is called (for both native menu items and for keystrokes, so we'd need to keep track of things) but preventDefault has no effect.
*** Bug 344734 has been marked as a duplicate of this bug. ***
Nominating as a 1.8.1 blocker. This is a significant regression, and it will be even more obvious once 50590 lands on the 1.8 branch. 
Flags: blocking1.8.1?
Bryner, can we get some sr=magic?
Flags: blocking1.8.1? → blocking1.8.1+
Target Milestone: --- → mozilla1.8.1beta2
Flags: blocking1.8.0.6+
Flags: blocking1.8.0.5?
Attached patch Partial 1.8.0 branch fix (obsolete) — Splinter Review
This fix is only partial, because I can't find the right nsIContent node in nsXBLPrototypeHandler::ExecuteHandler to look for the noProcess attribute on.  The patch as I've got it here fixes double-processing for certain things (like command-T for new tab and command-W for close), but not for others (like command-V for paste.  Yuck.)

To see what's wrong with this patch, have a letter copied the clipboard, and load a Java applet.  (  Focus the search box and open the Edit menu.  (Focusing the search box ensures that the Paste command is enabled.)  Close the Edit menu and press command-V.

If someone can tell how to get from that method to the node that I'm setting noProcess on in nsMenuItemX, we should be able to make this work with the following snippet:

#ifdef XP_MACOSX
  nsAutoString noProcess;
  CONTENTNODE->GetAttr(kNameSpaceID_None, nsXULAtoms:noProcess, noProcess);
  if (noProcess.EqualsLiteral("true")) 
    return NS_OK;
Attached patch Partial 1.8.0 branch fix (obsolete) — Splinter Review
Last version was missing a file.
Attachment #230190 - Attachment is obsolete: true
I've got this fixed (from the JEP side) in my current version of the
JEP.  I'm working very hard to get this out (as JEP 0.9.5+g) tomorrow
(Saturday 7/22) or the day after.

Once it's out I'll ask Mark to bang away at it ... since he's lately
been particularly good at finding JEP bugs :-)

Then, if it passes muster, I'll ask that it be landed on the trunk and
  <!-- These key nodes are here only for show. The real bindings come from
       XBL, in platformHTMLBindings.xml. See bugs 57078 and 71779. -->

If Java can get at the original keystrokes for menuitems, why can't we?
That would then solve bug 199019, bug 194147 etc.
Steven: While it's great to have a new JEP with this problem fixed so soon, we are most likly not going to take it for the release. Even landing Marks patch this late in the game is very scary and might end up affecting when we can release.

However it's of course great if we can have a new release for

Mark: is there any way I can help with completing the patch? Ping me on irc if so.
> If Java can get at the original keystrokes for menuitems, why can't
> we?

The answer is complicated, and is (as far as I can tell) irrelevant to
this bug -- Mark wants to stop using keypress events and only use
"native menu command events".

But in any case, here's a very basic answer to your question:  Once a
Java applet has been loaded in a given browser window (for a
Carbon-based browser), the JEP needs to intervene for the brower to
receive _any_ keyboard or mouse events for that window (aside from
mouse-moved events).  (This is due to a severe design flaw in Apple's
Cocoa-Carbon interface.)  As far as I know the JEP now always sends
the browser events that it needs to see.  But there are still some
cases in which it sends a given event more than once.  This is one of
those cases ... though (as I've already said) it's now been fixed in
my current version.
>   <!-- These key nodes are here only for show. The real bindings come from
>        XBL, in platformHTMLBindings.xml. See bugs 57078 and 71779. -->

That sounds, but something's not quite right - the real bindings aren't coming from that file on the Mac, at least, because it doesn't contain them.

> Mark: is there any way I can help with completing the patch? Ping me on irc
> if so.

Yeah, we need to do one of two things:
 - In widget, find and set noProcess on the actual content node that's in
   nsXBLPrototypeHandler::mHandlerElement when you press one of these
   tempermental keystrokes like command-C.
 - In nsXBLPrototypeHandler, find the command content node that we're
   already setting noProcess on in widget.

I will be back later this evening to look into this some more.
Thanks Mark for the super rush job on this. I'll be around through the evening to start testing once you have a fix.
Attached patch More 1.8.0 branch fix (180v4) (obsolete) — Splinter Review
This is a little more heinous than I had expected.  Some of the menu items, such as copy, were set as XBL bindings instead of XUL elements.  The strategy here is to use getElementById in ExecuteHandler to determine if no-process has been set.  This is done after deciding that the handler would be the default and calling preventDefault - if preventDefault is not called, some other handler might pick it up and wind up routing it to the unwanted command again.

I put this patch through the paces, testing all of the menu item key equivalents in an ordinary browser window after loading Java.  Here's are the anomalies:

Load Java, open File menu, press command-P to print, then cancel the operation.  The browser crashes.  I debugged this and we're getting a garbage refcon in an Apple event from the system (nsMacTSMMessagePump::UnicodeNotFromInputMethodHandler is getting a bogus eventHandler from the Apple Event's keyAETSMDocumentRefcon parameter.)  This seems like an unrelated bug.  It occurs with or without this patch.

With or without loading Java, have a single tab open and open the Bookmarks menu.  Open a new tab.  Press command-shift-D for Bookmark All Tabs.  The command is not executed.  This is an unrelated issue and also occurs in

Load Java, open the Help menu, and press command-shift-? for Help.  Two help windows open.  This is because the help menu item is not created with a command node but an oncommand attribute with inline javascript:

Your call.  It fixes the data loss, but I'm not 100% behind this approach now that I've been down in the trenches.  On the other hand, it's probably more palatable than downgrading JEP.  The current JEP version is two releases newer than what shipped with - Steven's been busy in the past couple of months, fixing a very nontrivial number of crash and display bugs.  Check the JEP Changes and History files for more details.

A possible alternative would be to get a JEP build of 095+f plus a fix for the cause of this particular bug, as opposed to the whole 095+g, and use that for a Mac 1805 respin.  I'm not sure if Steven's able to do that, but I'd gladly review the event redispatch changes that it needs.  (I'm looking forward to seeing what happens there anyway.)
Attachment #230192 - Attachment is obsolete: true
Another workaround, which we can do entirely in widget/src/mac, is to walk the menu hierarchy for each keypress event and reject any keypresses that are tied to menu items.  That's safe - it's exactly what the system does - but there's a slight time penalty as we'll need to walk the menu tree a second time for each keypress, and we'd need to implement the entire procedure ourselves.  We can't use the system's because we aren't using the right event model for that call on the 1.8.0 branch.
I think there are a few acceptable options here:

1. Revert to the JEP used in This would of course be sad, but we'd be in a no worse state then for, and there's always in a few weeks.

2. Get a 095f+ JEP that does a spot-fix for this regression. But we'd have to get it fast, as in this weekend latest and even that might be too late.

3. Test a patch that does the walking of the menu items to see what the perf hit would be like. Again, we'd need such a patch urgently.

I'm actually leaning towards 1 at this point. Getting out the door is important for many other reasons then a better JEP, and there are more releases comming up where we can get the latest and greatest JEP.

It's up to the other people in the release time of course.
Attached patch Alternate 1.8.0 branch fix (obsolete) — Splinter Review
I had forgotten about this function.  It's crappy because it doesn't do a good job of distinguishing shifted key equivalents.  Since we've got command-D and command-shift-D in our menus, this is powerless against preventing two "add bookmark" sheets from showing up.  Other than that, it's a quick way to get the job done.
> 1. Revert to the JEP used in

Bad idea, I think.  Do (as Mark suggests) take a look at the changes
since JEP 0.9.5+d (the version that was bundled with Firefox
In the JEP 0.9.5+f distro, the information is in its Changes.txt and
the first section of its History.txt.  Particularly important are
items 2, 4, 5 and 7 in the change log for 0.9.5+e and items 1 and 2 in
the change log for 0.9.5+f.

> 2. Get a [095+f] JEP that does a spot-fix for this regression.

As it happens, this would be a one-line change.  I won't do it as an
official release.  But I'd be happy to put together a "0.9.5+f+" and
either email it to someone (Mark, presumably) or arrange a time where
I can temporarily post it in a "Testing" section on my SourceForge
Attached patch Another 1.8.0 branch fix (obsolete) — Splinter Review
I'm now convinced that, due to other bugs in the way we handle menus, the only things we can do to resolve this problem in Mozilla (xbl, widget, anywhere) are imperfect band-aids.  As such, I can't in good conscience recommend any of them, and the path of least resistance to fix the menus in a way that would enable these patches to do the right thing, although trivial, probably involves more risk than anyone's willing to take this late in the cycle.

The primary bug that causes problems is that we leave a toplevel window's native menu bar up even when we activate certain windows (like the help window) that don't have their own menu bars.  If you were to use any of the patches here, open the file menu, open the help window, and press command-W, the window would not close.  You can experience this bug for yourself by opening the help window and selecting File:Close from the menus instead of using a keystroke.  That's the current behavior, but making it the behavior for the keystroke too would be a perceptible regression that I'm not comfortable introducing.  (The solution would be to put the hidden window's menu bar up when a window without its own menu bar is active.)
Attachment #228856 - Attachment is obsolete: true
Attachment #229891 - Attachment is obsolete: true
Attachment #230231 - Attachment is obsolete: true
Attachment #230234 - Attachment is obsolete: true
Attachment #228856 - Flags: superreview?(bryner)
Comment on attachment 230271 [details] [diff] [review]
Another 1.8.0 branch fix

I'd happily try a one-line fix to the JEP (comment 26).  Going back to JEP v. D is more than dropping the "latest and greatest" - it reintroduces a number of crashes, some of which are easily reproducible.
Attachment #230271 - Attachment is obsolete: true
I've just created a JEP 0.9.5+f+ with my one-line fix for this bug,
and tested it against the original report (comment #0), using Firefox RC4 on both OS X 10.4.7 (PPC) and 10.3.9 (I'll test 10.4.7
Intel later).  JEP 0.9.5+f+ does clear up the problem.

> Load Java, open File menu, press command-P to print, then cancel the
> operation.  The browser crashes.

> Load Java, open the Help menu, and press command-shift-? for Help.
> Two help windows open.

Firefox RC4 doesn't have these problems with JEP 0.9.5+f+
... but I wasn't able to reproduce them with JEP 0.9.5+f either.

I'll create a JEP 0.9.5+f+ distro.  It should be ready within an hour.
Then Mark and I can arrange how I'll get a copy to Mark.

(Once I've created the distro I'll also post a diff here.)
Steven: Can I get a copy of the 0.9.5+f+ build as well? You can email it to me at or just zip it up and post it as an attachment to this bug. I'd like to get started testing 1505 with it so that the release team can make an informed decision about how to proceed. 

Thanks for all your work on this,

> or just zip it up and post it as an attachment to this bug

I hadn't thought of that.  Seems like the best way ... but I'm afraid
it might be too big (about 3.25 megs).  I'll try it anyway.
Alright, it _was_ just slightly too big (and I really can't cut any
more of it out).

Zach, I've emailed you a copy.  Feel free to share it with whoever you
like.  Let me know right away if there are any urgent problems.

Mark, should I just email you a copy?

(By the way, I can't email a copy to everyone who wants one -- my ISP
wouldn't be too happy.  Please share your copies as you see fit.  It
might be best to put a copy on some internal server, and
post a link here.)
(In reply to comment #32)
> Mark, should I just email you a copy?

Yes, please - I'll test it this evening.
>> Mark, should I just email you a copy?
>Yes, please - I'll test it this evening.

Steven - thanks for your quick work on this.  My own $.02 on backout to .9.5d vs. .9.5+f is that we at least understand the issues with .9.5d - wheres .9.5+f will get very little testing prior to wide release.  So I'd be concerned we'd introduce an issue that is worse than one of ones you are fixing.   You and Mark are the experts in this area of code - just want to make sure you undertand the risks.
The "recently" introduced regressions (basically the ones that Mark's
found) were all introduced by JEP 0.9.5+e, which has been out for a
while (I released it on 5-30, and it was landed on the trunk the same
day, on the 1.8 branch on 5-31, and on the 1.8.0 branch on 6-13).  As
you call tell from its change log, 0.9.5+e was quite ambitious -- so
these regressions aren't too surprising.  But it's been out for long
enough (and exposed to enough users) that any really serious bugs
should have been discovered by now.

JEP 0.9.5+f was released more recently (6-29, landed on trunk and all
branches by 7-05), and so has had less exposure.  But it was more
conservative than 0.9.5+e, and so should be less risky.

So, to my mind, you have JEP 0.9.5+d's fairly large number of known
problems against what I think is a pretty low likelihood of major,
undiscovered problems in 0.9.5+f (and 0.9.5+f+).  I think 0.9.5+f+
wins hands down :-)

Nonetheless, people working on the release should test
0.9.5+f+ as much as they can (and I will continue to test it through
the weekend).
I have been having some rather significant issues with 0.9.5+f+. When I load a simple applet (such as the bar chart example Sun provides at, it often does not display--the space where the applet should appear is blank. Usually, switching Firefox to the background and back to the foreground (i.e. clicking on the desktop and then back into ff) causes the applet to suddenly appear. Pressing the back and forward buttons has also caused the applet to appear, but pressing reload causes it to disappear again. This does not happen when I go back to 0.9.5+f. 

I'm not entirely sure how this could happen considering the diff, so it's quite possible something is wrong on my end. I installed the new version by replacing the JavaEmbeddingPlugin.bundle file inside the .app package with the new one provided by 0.9.5+f+. Is there something else I need to do as well?
> I installed the new version by replacing the
> JavaEmbeddingPlugin.bundle file inside the .app package with the new
> one provided by 0.9.5+f+.

Please read the Readme :-)

You need to replace _both_ JavaEmbeddingPlugin.bundle and

What you currently have (with your mismatched versions of those two
files) is Java 1.3.1 ... which does have its own display problems.
Comment on attachment 230274 [details]
Diff from JEP 0.9.5+f to JEP 0.9.5+f+

Those of you following along from home who would like to give 0.9.5+f+ a spin can pick up a copy from:

To install, replace both MRJPlugin.plugin and JavaEmbeddingPlugin.bundle in your Mozilla app bundle at Contents/MacOS/plugins with the copies in the Binaries directory of this archive.
Comment on attachment 230274 [details]
Diff from JEP 0.9.5+f to JEP 0.9.5+f+

r=me for this change for  With approval, and provided QA is satisfied as well, I'd like to land this on the branch so that the build team can do the Mac respins.

I spent some time following the relevant events from the user, through JEP, into the system, and into the browser.  This JEP patch will cause all menu command key equivalents to be handled on key event handlers, bypassing the possibility of being processed twice by being handled on a native menu item.  This is the same behavior as JEP 0.9.5+d and earlier, and the redispatch model used here is very similar to what was used in 0.9.5+d.  I'd like to thank Steven for his work here.  Someone, send that man a t-shirt!
Attachment #230274 - Flags: review+
Attachment #230274 - Flags: approval1.8.0.5?
(In reply to comment #35)
> My own $.02 on backout to .9.5d vs. .9.5+f is that we at least understand the
> issues with .9.5d -

one of them being no universal binary build I believe.

I'm more comfortable with f+ than going back to d assuming the above patch is an accurate representation of the difference. We were otherwise comfortable with 'f' up to this regression.
Comment on attachment 230274 [details]
Diff from JEP 0.9.5+f to JEP 0.9.5+f+

can't find any other drivers, going out on a limb and guessing they'll agree, a=dveditz
Attachment #230274 - Flags: approval1.8.0.5? → approval1.8.0.5+
FYI, we'll also respin SeaMonkey 1.0.3 mac builds once this fix is checked in.
JEP 0.9.5+f+ checked in on MOZILLA_1_8_0_BRANCH for  Respins are needed for Firefox and Seamonkey on the Mac to pick up the change.  No respin is needed for Thunderbird, as it does not bundle JEP.

> one of them being no universal binary build I believe.

0.9.5+d was available as a universal binary, but it was the initial release and there were still a couple of x86-ppc parity bugs, as I recall.  We even had a 0.9.5+c universal binary rebuild - I don't recall which one was included with
Keywords: fixed1.8.0.5
Can confirm fix for this bug in Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060711 SeaMonkey/1.5a with JEP 0.9.5+f+ copied into the application package replacing 0.9.5+f. Thanks for the download link at

All my test websites rendered okay except for one ( where the mouseover effects in the blue menu do not work (but this has only ever worked in Safari or when using Apple Java 1.3.1 in SeaMonkey without JEP), and some links in this menu aren't clickable [but *are* in Safari], maybe related to umlauts, or the menu not loading completely), and sometimes applets drawing in the wrong tabs, and ghost images of applets from tabs which were closed long ago. 

Otherwise large improvement over 0.9.5+e and f both in general application stability and website compatibility.
Flags: blocking1.8.0.6+
Flags: blocking1.8.0.5?
Flags: blocking1.8.0.5+
Verified fixed on 1505 rc5 with JEP 0.9.5+f+. Thanks again to Steven and Mark for the quick weekend work!
Would be good to get a fix in to the 1.8 branch for 1.8.1 soon.
Comment on attachment 230274 [details]
Diff from JEP 0.9.5+f to JEP 0.9.5+f+

The fix would be to land the 0.9.5+f+ JEP on the 1.8 branch.
Attachment #230274 - Flags: approval1.8.1?
Steven tells me that 0.9.5+g will be out soon.  I'd rather wait for that on the 1.8 [.1] branch as it will fix some other serious regressions only experienced on that branch and on the trunk.
I'm hoping to release 0.9.5+g tomorrow (Wednesday) or Thursday.

I'd originally planned to release it Sunday (7-23) or Monday, but ran
into some last-minute problems.  The additional bmo bugs fixed by
0.9.5+g are 344006, 344153 (I think) and what's described at bmo bug
344701 comment 6 and following.  There are (now) also some other,
minor fixes (having to do with keyboard input problems in Java applets
in multiple tabs with multiple windows).
Comment on attachment 230274 [details]
Diff from JEP 0.9.5+f to JEP 0.9.5+f+

a=drivers. Please land this on the MOZILLA_1_8_BRANCH.
Attachment #230274 - Flags: approval1.8.1? → approval1.8.1+
Unless there's a +g by then, I'll land +f+ on the 1.8 branch tomorrow because this bug is preventing bug 50590 from landing.  No pressure, Steven, I don't think we'll have trouble getting +g in any event because those other bugs are also pretty serious.
Checked in on MOZILLA_1_8_BRANCH, expecting another JEP update soon.
Keywords: fixed1.8.1
JEP 0.9.5+g has been checked in on the trunk, this problem is now fixed on all branches.
Closed: 18 years ago
Depends on: 346156
Resolution: --- → FIXED
No longer depends on: 346156
Fixed by bug 346156.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.