8263846: Bad JNI lookup getFocusOwner in accessibility code on Mac OS X #3099

azuev-java · 2021-03-19T23:10:46Z

Progress

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

Issue

JDK-8263846: Bad JNI lookup getFocusOwner in accessibility code on Mac OS X

Reviewers

Alexander Zvegintsev (@azvegint - Reviewer) ⚠️ Review applies to bc18607
Phil Race (@prrace - Reviewer)

Download

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

To update a local copy of the PR:
$ git checkout pull/3099
$ git pull https://git.openjdk.java.net/jdk pull/3099/head

Initialize sjc_CAccessibility before using it

bridgekeeper · 2021-03-19T23:11:42Z

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

openjdk · 2021-03-19T23:12:57Z

@azuev-java 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-03-19T23:16:44Z

Webrevs

azvegint

Since there is no test, I assume that the JBS issue should have noreg-hard label.

openjdk · 2021-03-19T23:42:43Z

@azuev-java 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:

8263846: Bad JNI lookup getFocusOwner in accessibility code on Mac OS X

Reviewed-by: azvegint, prr

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 6 new commits pushed to the master branch:

cb742f9: 8255255: Update Apache Santuario (XML Signature) to version 2.2.1
d2c137d: 8263558: Possible NULL dereference in fast path arena free if ZapResourceArea is true
ab66d69: 8263138: Initialization of sun.font.SunFontManager.platformFontMap is not thread safe
5b8233b: 8263871: On sem_destroy() failing we should assert
96e5c3f: 8263890: Broken links to Unicode.org
4d9517d: 8263834: Work around gdb for HashtableEntry

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

mrserb · 2021-03-20T02:13:46Z

src/java.desktop/macosx/native/libawt_lwawt/awt/JavaComponentAccessibility.m

@@ -1456,6 +1456,7 @@ - (id)accessibilityHitTest:(NSPoint)point withEnv:(JNIEnv *)env
 - (id)accessibilityFocusedUIElement
 {
    JNIEnv *env = [ThreadUtilities getJNIEnv];
+    GET_CACCESSIBILITY_CLASS_RETURN(nil);


Is it caused by the JDK-8257853? Should we update the accessibilityIndexOfChild as well?

It made it visible however it is just a good practice not to assume that class reference is already initialized before using it. So, yes, while i was not seeing any failures in accessibilityIndexOfChild i might fix it as well.

If this was missed in the JNF work, add the label "jnf" to the bug.

azuev-java · 2021-03-20T13:59:13Z

Since there is no test, I assume that the JBS issue should have noreg-hard label.

Yes, as with most of platform-specific accessibility issues just configuring a system to be able to invoke this native functionality is quite hard. I will put the corresponding keyword into the bug.

prrace · 2021-03-20T17:06:38Z

src/java.desktop/macosx/native/libawt_lwawt/awt/JavaComponentAccessibility.m

@@ -799,6 +799,7 @@ - (NSUInteger)accessibilityIndexOfChild:(id)child
    }

    JNIEnv *env = [ThreadUtilities getJNIEnv];
+    GET_CACCESSIBILITY_CLASS_RETURN(0);


This isn't necessary. Because the line below takes care of it. The definition looks like
#define GET_ACCESSIBLEINDEXINPARENT_STATIC_METHOD_RETURN(ret)
GET_CACCESSIBILITY_CLASS_RETURN(ret);
GET_STATIC_METHOD_RETURN(sjm_getAccessibleIndexInParent, sjc_CAccessibility, "getAccessibleIndexInParent",
"(Ljavax/accessibility/Accessible;Ljava/awt/Component;)I", ret);

Ah, ok, was missed that. Will revert this change.

prrace · 2021-03-20T17:07:41Z

src/java.desktop/macosx/native/libawt_lwawt/awt/JavaComponentAccessibility.m

@@ -1456,6 +1456,7 @@ - (id)accessibilityHitTest:(NSPoint)point withEnv:(JNIEnv *)env
 - (id)accessibilityFocusedUIElement
 {
    JNIEnv *env = [ThreadUtilities getJNIEnv];
+    GET_CACCESSIBILITY_CLASS_RETURN(nil);


If this was missed in the JNF work, add the label "jnf" to the bug.

there takes care of the class initialization.

azuev-java · 2021-03-20T17:57:36Z

/integrate

openjdk · 2021-03-20T17:59:00Z

@azuev-java Since your change was applied there have been 6 commits pushed to the master branch:

cb742f9: 8255255: Update Apache Santuario (XML Signature) to version 2.2.1
d2c137d: 8263558: Possible NULL dereference in fast path arena free if ZapResourceArea is true
ab66d69: 8263138: Initialization of sun.font.SunFontManager.platformFontMap is not thread safe
5b8233b: 8263871: On sem_destroy() failing we should assert
96e5c3f: 8263890: Broken links to Unicode.org
4d9517d: 8263834: Work around gdb for HashtableEntry

Your commit was automatically rebased without conflicts.

Pushed as commit 118a49f.

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

8263846: Bad JNI lookup getFocusOwner in accessibility code on Mac OS X

bc18607

Initialize sjc_CAccessibility before using it

openjdk bot added awt client-libs-dev@openjdk.org rfr Pull request is ready for review labels Mar 19, 2021

azvegint approved these changes Mar 19, 2021

View reviewed changes

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

mrserb reviewed Mar 20, 2021

View reviewed changes

Fixing accessibilityIndexOfChild in the same way

46b6e41

prrace reviewed Mar 20, 2021

View reviewed changes

Reverting the change for accessibilityIndexOfChild since macro used

e0c3b9d

there takes care of the class initialization.

prrace approved these changes Mar 20, 2021

View reviewed changes

openjdk bot closed this Mar 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 Mar 20, 2021

azuev-java deleted the JDK-8263846 branch August 23, 2021 19:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8263846: Bad JNI lookup getFocusOwner in accessibility code on Mac OS X #3099

8263846: Bad JNI lookup getFocusOwner in accessibility code on Mac OS X #3099

azuev-java commented Mar 19, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 19, 2021

openjdk bot commented Mar 19, 2021

mlbridge bot commented Mar 19, 2021 •

edited

Loading

azvegint left a comment

openjdk bot commented Mar 19, 2021 •

edited

Loading

mrserb Mar 20, 2021

azuev-java Mar 20, 2021

prrace Mar 20, 2021

azuev-java Mar 20, 2021

azuev-java commented Mar 20, 2021

prrace Mar 20, 2021

azuev-java Mar 20, 2021

prrace Mar 20, 2021

azuev-java commented Mar 20, 2021

openjdk bot commented Mar 20, 2021

8263846: Bad JNI lookup getFocusOwner in accessibility code on Mac OS X #3099

8263846: Bad JNI lookup getFocusOwner in accessibility code on Mac OS X #3099

Conversation

azuev-java commented Mar 19, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Mar 19, 2021

openjdk bot commented Mar 19, 2021

mlbridge bot commented Mar 19, 2021 • edited Loading

Webrevs

azvegint left a comment

Choose a reason for hiding this comment

openjdk bot commented Mar 19, 2021 • edited Loading

mrserb Mar 20, 2021

Choose a reason for hiding this comment

azuev-java Mar 20, 2021

Choose a reason for hiding this comment

prrace Mar 20, 2021

Choose a reason for hiding this comment

azuev-java Mar 20, 2021

Choose a reason for hiding this comment

azuev-java commented Mar 20, 2021

prrace Mar 20, 2021

Choose a reason for hiding this comment

azuev-java Mar 20, 2021

Choose a reason for hiding this comment

prrace Mar 20, 2021

Choose a reason for hiding this comment

azuev-java commented Mar 20, 2021

openjdk bot commented Mar 20, 2021

azuev-java commented Mar 19, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 19, 2021 •

edited

Loading

openjdk bot commented Mar 19, 2021 •

edited

Loading