Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8259343: [macOS] Update JNI error handling in Cocoa code. #1967

Closed
wants to merge 4 commits into from
Closed

8259343: [macOS] Update JNI error handling in Cocoa code. #1967

wants to merge 4 commits into from

Conversation

@prrace
Copy link
Contributor

@prrace prrace commented Jan 6, 2021

Proposed updates to JNI error handling.


Progress

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

Issue

  • JDK-8259343: [macOS] Update JNI error handling in Cocoa code.

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1967/head:pull/1967
$ git checkout pull/1967

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 6, 2021

👋 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.

@openjdk
Copy link

@openjdk openjdk bot commented Jan 6, 2021

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

  • 2d
  • awt
  • build

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

* Together these provide alternate behaviours for more debugging info
* or maybe a way for the app to continue running depending on the exact
* nature of the problem that has been detected and how survivable it is.
*/
#define CHECK_EXCEPTION() \
Copy link
Member

@mrserb mrserb Jan 7, 2021

Since this macro does not try to return from the outer code we can change it to the method?

Copy link
Contributor Author

@prrace prrace Jan 8, 2021

But then "env" would need to be passed explicitly. I don't think the churn is worth it.
And a method would then need a .m or .c file ...

Copy link
Member

@mrserb mrserb Jan 8, 2021

But that could be merged to the CallXXXMethod and placed somewhere in the libosxapp

Copy link
Contributor Author

@prrace prrace Jan 8, 2021

You mean those methods or macros that we already discussed and I explained why I do not want to do that ?
This is a small update for error checking not revisiting everything done before.

#define CHECK_EXCEPTION() \
if ((*env)->ExceptionOccurred(env) != NULL) { \
(*env)->ExceptionClear(env); \
if ([NSThread isMainThread] == YES) { \
Copy link
Member

@mrserb mrserb Jan 7, 2021

Isn't it is a similar code to the


Where we just catch an exception log it, and continue to run our infinite loop? Why do we need to do something specific here? I mean we cannot drop any try/catch blocks anyway since an nsexception may be generated by some external system code.

Copy link
Contributor Author

@prrace prrace Jan 8, 2021

In my testing that code does not get called. The new code does. So the old code could probably be deleted as useless but it is also harmless and just maybe it'll catch something else, even if it isn't doing anything that I can see.

Copy link
Member

@mrserb mrserb Jan 8, 2021

That code for sure should be called, it is even improved recently by JDK-8255681
I'll check how it was intended to work.

Copy link
Contributor Author

@prrace prrace Jan 8, 2021

I hadn't noticed that line - and definintely not realised it was recent.
I suppose he must have had a way to trigger it.
But I don't think it hurts to have both.

Copy link
Member

@mrserb mrserb Jan 11, 2021

I just tried to understand why we need to complicate this, to me, it is unclear why handling the same errors in the NSApplicationAWT.m does not work.

Copy link
Contributor Author

@prrace prrace Jan 11, 2021

Because of the reason I've said before. That logging in NSApplication AWT is not being seen. Something is swallowing it. So I'd say if anything remove that logging and keep the new one but as I also said it isn't harmful to have provision in case it also logs some case we aren't catching.

/******** LOGGING SUPPORT *********/

#define LOG_NULL(dst_var, name) \
if (dst_var == NULL) { \
NSLog(@"Bad JNI lookup %s\n", name); \
NSLog(@"%@",[NSThread callStackSymbols]); \
if ([NSThread isMainThread] == NO) { \
JNU_ThrowInternalError(env, "Bad JNI Lookup"); \
Copy link
Member

@mrserb mrserb Jan 7, 2021

It will be possible that(most of the time) the JNU_ThrowInternalError will be called while we already have a raised java exception.

Copy link
Contributor Author

@prrace prrace Jan 8, 2021

I don't think "most" of the time is very likely. JNI lookup failures don't cause exceptions to be thrown.
Is that what you are tihinking ?

And separately, with either the current code of clearing exceptions or the change here to the practice of throwing NSException on seeing a Java Exception I think it very unlikely.

Copy link
Member

@mrserb mrserb Jan 8, 2021

Then how we get NoSuchMethodError here? https://bugs.openjdk.java.net/browse/JDK-8259232?
https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#GetMethodID
Calling Instance Methods: GetMethodID
RETURNS:
Returns a method ID, or NULL if the specified method cannot be found.
THROWS:
NoSuchMethodError: if the specified method cannot be found.
ExceptionInInitializerError: if the class initializer fails due to an exception.
OutOfMemoryError: if the system runs out of memory.

Same for fields:
https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#GetFieldID

Copy link
Contributor Author

@prrace prrace Jan 8, 2021

Ok. I will only create my own if none is pending.

} else { \
if ((*env)->ExceptionOccurred(env) != NULL) { \
(*env)->ExceptionDescribe(env); \
} \
Copy link
Contributor Author

@prrace prrace Jan 8, 2021

So the update here is that if we are not on the appkit thread, make sure a java exception is thrown.
If we are on the appkit thread, clear any java exception since it isn't going anywhere but do it using
describe which prints it.

Copy link
Member

@mrserb mrserb Jan 11, 2021

I read the logic of the method differently, probably the wrong indents?

  • If we are not on the toolkit thread then
    • Check ExceptionOccurred -> throw JNU_ThrowInternalError if needed or check exception again ->call ExceptionDescribe
  • NSException raise at the end.

Copy link
Contributor Author

@prrace prrace Jan 11, 2021

I have a paren in the wrong place ! I've pushed an update.

@prrace prrace marked this pull request as ready for review Jan 9, 2021
@openjdk openjdk bot added the rfr label Jan 9, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 9, 2021

Webrevs

/******** LOGGING SUPPORT *********/

#define LOG_NULL(dst_var, name) \
if (dst_var == NULL) { \
NSLog(@"Bad JNI lookup %s\n", name); \
NSLog(@"%@",[NSThread callStackSymbols]); \
if ([NSThread isMainThread] == NO) { \
if ((*env)->ExceptionOccurred(env) == NULL) { \
Copy link
Member

@mrserb mrserb Jan 11, 2021

Not sure that the check for ExceptionOccurred is needed, in all other places where we check the ref to methods/field we only check the return value, and if it is null then return immediately assuming that an exception is rased already, for example :

cls = env->FindClass("java/awt/Button");

Note that the exception in the static initializer is fatal for the application.

Copy link
Contributor Author

@prrace prrace Jan 11, 2021

Not sure that the check for ExceptionOccurred is needed,
It may not be needed in practice but if the code path is never taken no harm ...

in all other places where we check the ref to methods/field we only check the return value, and if it is null then return immediately assuming that an exception is rased already, for example :

cls = env->FindClass("java/awt/Button");

Note that the exception in the static initializer is fatal for the application.

Nothing new here.

Copy link
Member

@mrserb mrserb Jan 12, 2021

The new thing here is that you check the result of the ExceptionOccurred if NULL is returned from the GetMethodID/etc, we usually never do it. If such checks are necessary then I'll create a separate bug to update other similar use cases.

Copy link
Contributor Author

@prrace prrace Jan 12, 2021

Here I don't have immediate control over why this is being called.
So we could have null but no pending exception depending on what the caller did.
It won't be executed except in the rare case there's a problem so it certainly isn't causing overhead so what is the problem ?
I don't see a need to change other code unless there's a really good reason.

} else { \
if ((*env)->ExceptionOccurred(env) != NULL) { \
(*env)->ExceptionDescribe(env); \
} \
Copy link
Member

@mrserb mrserb Jan 11, 2021

I read the logic of the method differently, probably the wrong indents?

  • If we are not on the toolkit thread then
    • Check ExceptionOccurred -> throw JNU_ThrowInternalError if needed or check exception again ->call ExceptionDescribe
  • NSException raise at the end.

#define CHECK_EXCEPTION() \
if ((*env)->ExceptionOccurred(env) != NULL) { \
(*env)->ExceptionClear(env); \
if ([NSThread isMainThread] == YES) { \
Copy link
Member

@mrserb mrserb Jan 11, 2021

I just tried to understand why we need to complicate this, to me, it is unclear why handling the same errors in the NSApplicationAWT.m does not work.

@@ -93,6 +93,7 @@ ifeq ($(call isTargetOs, macosx), true)
LDFLAGS := $(LDFLAGS_JDKLIB) \
$(call SET_SHARED_LIBRARY_ORIGIN), \
LIBS := \
-ljava \
Copy link
Member

@erikj79 erikj79 Jan 11, 2021

There should be a dependency declaration added too. Something like this right after the SetupJdkLibrary macro call:

$(BUILD_LIBOSXAPP): $(call FindLib, java.base, java)

Copy link
Contributor Author

@prrace prrace Jan 11, 2021

There should be a dependency declaration added too. Something like this right after the SetupJdkLibrary macro call:

$(BUILD_LIBOSXAPP): $(call FindLib, java.base, java)

Fixed.

Copy link
Member

@erikj79 erikj79 left a comment

Build changes look good.

@openjdk
Copy link

@openjdk openjdk bot commented Jan 11, 2021

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

8259343: [macOS] Update JNI error handling in Cocoa code.

Reviewed-by: erikj, serb

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

  • c338f11: 8259349: -XX:AvgMonitorsPerThreadEstimate=1 does not work right
  • ccac7aa: 8258932: AArch64: Enhance floating-point Min/MaxReductionV with fminp/fmaxp
  • 4c75d14: 8259374: Make ThreadInVMfromNative have ResetNoHandleMark
  • 563b268: 8257709: C1: Double assignment in InstructionPrinter::print_stack
  • 400dc76: 8252015: [macos11] java.awt.TrayIcon requires updates for template images
  • ac2dee5: 8259539: JDK-8255711 broke trap messages
  • 4697cfa: 8259576: Misplaced curly brace in Matcher::find_shared_post_visit
  • a6ab9e4: 8258576: Try to get zerobased CCS if heap is above 32 and CDS is disabled
  • a3561ae: 8258243: C2: assert failed ("Bad derived pointer") with -XX:+VerifyRegisterAllocator
  • 4663704: 8259583: Remove unused decode_env::_codeBuffer
  • ... and 98 more: https://git.openjdk.java.net/jdk/compare/80544e4d5f402f67171eb5aca11897f1c5c3d431...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.

@openjdk openjdk bot added the ready label Jan 11, 2021
} \
} \
if (getenv("JNU_NO_COCOA_EXCEPTION") == NULL) { \
[NSException raise:NSGenericException format:@"Java Exception"]; \
Copy link
Member

@mrserb mrserb Jan 12, 2021

How did you check that the logging in the NSApplication was swallowing? Both macro will throw the NSException on the toolkit thread now, does it mean that in both cases the logging in the NSApplication will be ignored/no output?

Copy link
Contributor Author

@prrace prrace Jan 12, 2021

See the bug assigned to you that I filed last month : https://bugs.openjdk.java.net/browse/JDK-8258797
This error should have been logged by that NSApplicationAWT code but was not (and I mean in JDK 16 as well before I started on this) and in JDK 17 it was seen only when adding the new logging.

Copy link
Member

@mrserb mrserb Jan 12, 2021

I have found it down to the absence of NSApplication#reportException() method and logging in it. Ok will update that code later in the separate update.

mrserb
mrserb approved these changes Jan 12, 2021
@prrace
Copy link
Contributor Author

@prrace prrace commented Jan 12, 2021

/integrate

@openjdk openjdk bot closed this Jan 12, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jan 12, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 12, 2021

@prrace Since your change was applied there have been 108 commits pushed to the master branch:

  • c338f11: 8259349: -XX:AvgMonitorsPerThreadEstimate=1 does not work right
  • ccac7aa: 8258932: AArch64: Enhance floating-point Min/MaxReductionV with fminp/fmaxp
  • 4c75d14: 8259374: Make ThreadInVMfromNative have ResetNoHandleMark
  • 563b268: 8257709: C1: Double assignment in InstructionPrinter::print_stack
  • 400dc76: 8252015: [macos11] java.awt.TrayIcon requires updates for template images
  • ac2dee5: 8259539: JDK-8255711 broke trap messages
  • 4697cfa: 8259576: Misplaced curly brace in Matcher::find_shared_post_visit
  • a6ab9e4: 8258576: Try to get zerobased CCS if heap is above 32 and CDS is disabled
  • a3561ae: 8258243: C2: assert failed ("Bad derived pointer") with -XX:+VerifyRegisterAllocator
  • 4663704: 8259583: Remove unused decode_env::_codeBuffer
  • ... and 98 more: https://git.openjdk.java.net/jdk/compare/80544e4d5f402f67171eb5aca11897f1c5c3d431...master

Your commit was automatically rebased without conflicts.

Pushed as commit d6a2105.

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

@prrace prrace deleted the jnf_update branch Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants