8272602: [macos] not all KEY_PRESSED events sent when control modifier is used #5177

prrace · 2021-08-18T23:57:38Z

When Ctrl+Space is pressed mac generates a string that contains the single unicode code point zero.
The fn that converts it from an NSString to a Java String is using NewStringUTF.
The input to that is a null terminated string which also has zero as the code point it contains, so
we actually end up with a zero length Java string instead of the intended one code point in length.
So the fix is to change the way we convert the string.

There's an existing test CtrlAscii.java which sort of tests some of this but it isn't asserting that you
get what you expect, its mostly testing you don't get something unexpected .. it will happily pass if
you don't get keyevents. I did not want to change the purpose of that test for this.
So I wrote a test specific to this Ctrl+Space to verify the fix but also ran all the standard automated tests too.

Progress

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

Issue

JDK-8272602: [macos] not all KEY_PRESSED events sent when control modifier is used

Reviewers

Alexander Zuev (@azuev-java - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5177

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

Using diff file

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

…r is used

bridgekeeper · 2021-08-18T23:58:33Z

👋 Welcome back prr! 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.

…r is used

openjdk · 2021-08-19T00:02:40Z

@prrace The following label will be automatically applied to this pull request:

awt

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2021-08-19T00:06:47Z

Webrevs

00: Full (23e717e3)

prsadhuk · 2021-08-19T07:23:19Z

src/java.desktop/macosx/native/libosxapp/JNIUtilities.m

+    NSRange crange = NSMakeRange(0, len);
+    [str getCharacters:buffer range:crange];
+    jstring jStr = (*env)->NewString(env, buffer, len);
+    free(buffer);
    CHECK_EXCEPTION();


Shouldn't it be better to call CHECK_EXCEPTION_NULL_RETURN() here to return null in case NewString throw exception in which case, I guess jStr will have garbage.

Also, it seems https://developer.apple.com/documentation/foundation/nsstring/1408720-getcharacters says the buffer must be large enough...aRange.length*sizeof(unichar) so shouldn't the NSRange be created for
(0, len * sizeof(unichar))

Shouldn't it be better to call CHECK_EXCEPTION_NULL_RETURN()

If the jstring were garbage in such a case we would NOT want to check it against null, since
that would be a definite problem.

As it is NewString is specified to return NULL if there is a problem so the existing code is fine although the alternative pattern of CHECK_EXCEPTION_NULL_RETURN(jstr, NULL) would also work.

SInce I'm not changing this I think it fine to stick with the existing code.

the buffer must be large enough...aRange.length*sizeof(unichar) so shouldn't the NSRange be created for
(0, len * sizeof(unichar))

The buffer is large enough

buffer = (unichar*) calloc(len, sizeof(unichar));

the NSRange is the number of elements to copy. It is not the buffer storage

azuev-java

Checked - fix works and it does not break input handling including multibyte characters.

openjdk · 2021-08-19T15:41:39Z

@prrace This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8272602: [macos] not all KEY_PRESSED events sent when control modifier is used

Reviewed-by: kizune

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 44 new commits pushed to the master branch:

03b5e99: 8272165: Consolidate mark_must_be_preserved() variants
ab41812: 8272576: G1: Use more accurate integer type for collection set length
82b2f21: 8272579: G1: remove unnecesary null check for G1ParScanThreadStateSet::_states slots
1c80f07: 8272334: com.sun.net.httpserver.HttpExchange: Improve API doc of getRequestHeaders
6d3d479: 8272667: substandard error messages from the docs build
73da66f: 8272318: Improve performance of HeapDumpAllTest
96107e3: 8272573: Redundant unique_concrete_method_4 dependencies
4d6593c: 8272124: Cgroup v1 initialization causes NullPointerException when cgroup path contains colon
30b0f82: 8272626: Avoid C-style array declarations in java.*
e8f1219: 8271276: C2: Wrong JVM state used for receiver null check
... and 34 more: https://git.openjdk.java.net/jdk/compare/17b93500237e74dca964e1805e65c857e7980bbe...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

azuev-java · 2021-08-19T15:47:08Z

On the other hand i now see that AFTER i did a multibyte character input every time i switch into application with - i get following output in the terminal:
2021-08-19 08:44:24.155 java[18144:1448976] Apple AWT Internal Exception: Java Exception

azuev-java · 2021-08-19T15:52:57Z

On the other hand i now see that AFTER i did a multibyte character input every time i switch into application with - i get following output in the terminal:
2021-08-19 08:44:24.155 java[18144:1448976] Apple AWT Internal Exception: Java Exception

It is not related to this change, i rolled it back and the issue persists so has nothing to do with this fix.

prrace · 2021-08-19T20:25:52Z

Yep, it is unrelated to what is being touched in this fix so should be handled separately.
It needs a new bug report and this fix can go ahead.

mrserb · 2021-08-19T21:05:46Z

src/java.desktop/macosx/native/libosxapp/JNIUtilities.m

    if (str == NULL) {
       return NULL;
    }
-    jstring jStr = (*env)->NewStringUTF(env, [str UTF8String]);


Probably we should cleanup all usage of [nsstring UTF8String] in the code at some point.

Depends on what the source data is.
FWIW in the macOS code in java.desktop there are just 5 others and none of them were touched/added/changed by the JNF work. They are all pre-existing.

prrace · 2021-08-20T00:07:38Z

/integrate

openjdk · 2021-08-20T00:08:53Z

Going to push as commit ddcd851.
Since your change was applied there have been 50 commits pushed to the master branch:

d007be0: 8272700: [macos] Build failure with Xcode 13.0 after JDK-8264848
f4be211: 8270041: Consolidate oopDesc::cas_forward_to() and oopDesc::forward_to_atomic()
b40e8f0: 8271951: Consolidate preserved marks overflow stack in SerialGC
7eccbd4: 8266519: Cleanup resolve() leftovers from BarrierSet et al
9569159: 8272674: Logging missing keytab file in Krb5LoginModule
51c1b9a: 8272616: Strange code in java.text.DecimalFormat#applyPattern
03b5e99: 8272165: Consolidate mark_must_be_preserved() variants
ab41812: 8272576: G1: Use more accurate integer type for collection set length
82b2f21: 8272579: G1: remove unnecesary null check for G1ParScanThreadStateSet::_states slots
1c80f07: 8272334: com.sun.net.httpserver.HttpExchange: Improve API doc of getRequestHeaders
... and 40 more: https://git.openjdk.java.net/jdk/compare/17b93500237e74dca964e1805e65c857e7980bbe...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-08-20T00:09:03Z

@prrace Pushed as commit ddcd851.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8272602: [macos] not all KEY_PRESSED events sent when control modifie…

2fdfcba

…r is used

8272602: [macos] not all KEY_PRESSED events sent when control modifie…

23e717e

…r is used

openjdk bot added the rfr Pull request is ready for review label Aug 19, 2021

openjdk bot added the awt client-libs-dev@openjdk.org label Aug 19, 2021

prsadhuk reviewed Aug 19, 2021

View reviewed changes

azuev-java approved these changes Aug 19, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 19, 2021

mrserb reviewed Aug 19, 2021

View reviewed changes

openjdk bot closed this Aug 20, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 20, 2021

8272602: [macos] not all KEY_PRESSED events sent when control modifier is used #5177

8272602: [macos] not all KEY_PRESSED events sent when control modifier is used #5177

prrace commented Aug 18, 2021 •

edited by openjdk bot

bridgekeeper bot commented Aug 18, 2021

openjdk bot commented Aug 19, 2021

mlbridge bot commented Aug 19, 2021

prsadhuk Aug 19, 2021 •

edited

prrace Aug 19, 2021

azuev-java left a comment

openjdk bot commented Aug 19, 2021

azuev-java commented Aug 19, 2021

azuev-java commented Aug 19, 2021

prrace commented Aug 19, 2021

mrserb Aug 19, 2021

prrace Aug 19, 2021 •

edited

prrace commented Aug 20, 2021

openjdk bot commented Aug 20, 2021

openjdk bot commented Aug 20, 2021

8272602: [macos] not all KEY_PRESSED events sent when control modifier is used #5177

8272602: [macos] not all KEY_PRESSED events sent when control modifier is used #5177

Conversation

prrace commented Aug 18, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Aug 18, 2021

openjdk bot commented Aug 19, 2021

mlbridge bot commented Aug 19, 2021

Webrevs

prsadhuk Aug 19, 2021 • edited

Choose a reason for hiding this comment

prrace Aug 19, 2021

Choose a reason for hiding this comment

azuev-java left a comment

Choose a reason for hiding this comment

openjdk bot commented Aug 19, 2021

azuev-java commented Aug 19, 2021

azuev-java commented Aug 19, 2021

prrace commented Aug 19, 2021

mrserb Aug 19, 2021

Choose a reason for hiding this comment

prrace Aug 19, 2021 • edited

Choose a reason for hiding this comment

prrace commented Aug 20, 2021

openjdk bot commented Aug 20, 2021

openjdk bot commented Aug 20, 2021

prrace commented Aug 18, 2021 •

edited by openjdk bot

prsadhuk Aug 19, 2021 •

edited

prrace Aug 19, 2021 •

edited