8242544: CMD+ENTER key event crashes the application when invoked on dialog

[andreas-heger]

This also solves JDK-8205915 [macOS] Accelerator assigned to button in dialog fires menuItem in owning stage.

I must admit that I don't have 100% insight about what actually caused the problems and how the event flow completely works.

In MacOS, key events with a command or control modifier are handled via the performKeyEquivalent method, first. If no view or window handles such a shortcut key event (indicated by a return value NO in performKeyEquivalent), the key event is sent a second time via the sendEvent method. Before this fix, the method sendJavaKeyEvent in GlassViewDelegate.m seemed to be called twice: first by performKeyEquivalent and then by sendEvent, since no responder returned YES in performKeyEquivalent. For not handling the same event twice in JFX, there seemed to be a workaround in sendJavaKeyEvent in GlassViewDelegate.m. It only handled the first call. The second call checked if the given event was exactly the same like in the last call. In this case it simply returned and did not forward the event. This means that all key events with a CMD or CTRL modifier were handled in JFX in the performKeyEquivalent event loop, even though they actually weren't used by the MacOS keyEquivalent functionality and all responders returned NO in performKeyEquivalent. So MacOS sent the event again to all responders in the sendEvent loop. I assume that the window has been destroyed already, when MacOS tried to send the second event to it, because the closing was handled in the first call. Sending an event to a no longer existing window probably caused the crash by an unhandled exception.

It seems that this problem existed in the past and there was a workaround in GlassWindow.m. In the method performKeyEquivalent, it was checked if the window was closed and if so, it returned YES. However, nowadays the window closing check did not work... self->gWindow->isClosed returned NO when I debugged it, although the window should be closed by the key event. Returning YES in case of a closed window is not a clean solution, anyway, because YES should mean that the shortcut key equivalent is handled. Another point is that Apple writes that overwriting performKeyEquivalent in an NSWindow subclass is discouraged. So, I completely deleted the method from GlassWindow.m, since it only returned the value of the super function, except for the non working closing check.

Since the default version of performKeyEquivalent in NSWindow always returns NO, only deleting the method from GlassWindow.m did not fix the crash. So I tried to solve the problem that a shortcut key event is handled in the performKeyEquivalent loop. The call of sendJavaKeyEvent in GlassViewDelegate.m which is caused by a performKeyEquivalent should not be handled, actually. So, I simply removed this call which is done in GlassView3D.m performKeyEquivalent method. By removing the call, there is also no longer any need to check if the same key event was passed to sendJavaKeyEvent in GlassViewDelegate.m, so this check is also removed.

I'm curious about your comments and reviews and I wonder if this is a clean solution. All my tests so far seemed to be fine.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8242544: CMD+ENTER key event crashes the application when invoked on dialog

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/704/head:pull/704
$ git checkout pull/704

Update a local copy of the PR:
$ git checkout pull/704
$ git pull https://git.openjdk.java.net/jfx pull/704/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 704

View PR using the GUI difftool:
$ git pr show -t 704

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/704.diff

[bridgekeeper]

👋 Welcome back andreas-heger! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[mlbridge]

Webrevs

• 00: Full (12d975f5)

[andreas-heger]

This fix seems to also solve javafxports/openjdk-jfx#370.

[beldenfox]

I can reproduce this with a small Mac app that does nothing but set the NSWindow's contentView to nil while handling processKeyEvent:. I'm not handling the event twice or destroying the NSWindow. The crash happens immediately after returning from my implementation of processKeyEvent: and back into Apple's code. There's something about Apple's implementation of this routine that's sensitive to having the contentView nulled out. I don't see the same crash if I set the contentView to nil inside a keyDown: event.

I would love to get rid of our performKeyEquivalent: methods but we can't. JavaFX assumes that the focused Node will get the first opportunity to process a KeyEvent. For example, if a TextArea has the focus it gets the first opportunity to process Cmd-A to select all text. A KeyEvent should only bubble up to the top level menus if the focused node doesn't handle it. When using the system menubar on Mac this means we need to implement processKeyEquivalent: to ensure that we can hand the key event to the focused JavaFX node before it gets handled by the main NSMenu.

Unfortunately there's currently no way to determine that the event was consumed by the node so the Mac glass code hands it over to the main menu anyway. This leads to bugs where one event triggers multiple actions like JDK-8087863 and JDK-8088897 and the ones listed over in javafxports. Most of the changes for fixing this are in PR #694.

You're correct that the implementation of performKeyEquivalent: in GlassWindow.m is probably not necessary. The close selector is sent to the NSWindow asynchronously so the isClosed flag will never be set at the end of performKeyEquivalent:. This has been true since sometime in 2015, see the comments in Java_com_sun_glass_ui_mac_MacWindow__1close.

[kevinrushforth]

/reviewers 2

[openjdk]

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

[jperedadnr]

I've tested successfully this PR, running:

• JDK-8242544: https://bugs.openjdk.java.net/secure/attachment/87645/CrashMe.java
• JDK-8205915: https://bugs.openjdk.java.net/secure/attachment/77302/Main.java
• And sample code from Accelerator + System MenuBar javafxports/openjdk-jfx#370

The three of them work fine with this PR and fail without it.

I've been thinking about the need of having some tests for this, but since this is only for Mac, and we need performKeyEvent being called, the system tests won't work.

However, I guess a manual test is good enough? Any of the above cases can be easily added to tests/manual.

[beldenfox]

I reviewed the Mac glass code and it seems the smallest change we can make to prevent the crash is to set the NSWindow's contentView to a dummy NSView instead of nil. I don't see any place in the code where the contentView is inspected or is assumed to be a glass NSView subclass. I can submit the PR since I already have a branch set up to test it.

I suspect performKeyEquivalent was originally implemented because before macOS 10.5 it was the only way to receive key events with the Cmd modifier. These days we can receive those events via keyDown. For now it's main purpose is to ensure we get key events before the system menu bar does.

@jperedadnr The test case for JDK-8205915 tests a particular aspect of performKeyEquivalent that I just learned about (I've been watching some exciting WWDC videos lately). If the main and key windows are not the same they can both end up processing the same key event via performKeyEquivalent. That's a great test case.

I don't think the javafxports test case reveals the underlying ordering issue. Cmd+V should be handled by the textField directly and the MenuItem should not fire. I suspect with performKeyEquivalent removed the opposite is happening and only the MenuItem is firing.

[kevinrushforth]

We will likely close this PR in favor of the less-intrusive #714 unless our testing or review of that PR reveals any problems.

[kevinrushforth]

Closed in favor of #714, which has now been integrated.