Re: RFR: 8259343: [macOS] Update JNI error handling in Cocoa code. [v3]
On Mon, 11 Jan 2021 19:27:12 GMT, Phil Race wrote: >> Proposed updates to JNI error handling. > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8259343: [macOS] Update JNI error handling in Cocoa code. Marked as reviewed by serb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1967
Re: RFR: 8259343: [macOS] Update JNI error handling in Cocoa code. [v3]
On Tue, 12 Jan 2021 17:21:53 GMT, Phil Race wrote: >> src/java.desktop/macosx/native/libosxapp/JNIUtilities.h line 197: >> >>> 195: } \ >>> 196: if (getenv("JNU_NO_COCOA_EXCEPTION") == NULL) { \ >>> 197: [NSException raise:NSGenericException format:@"Java >>> Exception"]; \ >> >> 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? > > 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/1967
Re: RFR: 8259343: [macOS] Update JNI error handling in Cocoa code. [v3]
On Tue, 12 Jan 2021 02:31:56 GMT, Sergey Bylokhov wrote: >> Phil Race has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8259343: [macOS] Update JNI error handling in Cocoa code. > > src/java.desktop/macosx/native/libosxapp/JNIUtilities.h line 197: > >> 195: } \ >> 196: if (getenv("JNU_NO_COCOA_EXCEPTION") == NULL) { \ >> 197: [NSException raise:NSGenericException format:@"Java >> Exception"]; \ > > 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? 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. - PR: https://git.openjdk.java.net/jdk/pull/1967
Re: RFR: 8259343: [macOS] Update JNI error handling in Cocoa code. [v3]
On Tue, 12 Jan 2021 02:38:11 GMT, Sergey Bylokhov wrote: >>> 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 : >>> >>> https://github.com/openjdk/jdk/blob/b72de3c5fc99f365e9fb25114ddd28eceddfa6e8/src/java.desktop/windows/native/libawt/windows/awt_Button.cpp#L357 >>> >>> Note that the exception in the static initializer is fatal for the >>> application. >> >> Nothing new here. > > 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/1967
Re: RFR: 8259343: [macOS] Update JNI error handling in Cocoa code. [v3]
On Mon, 11 Jan 2021 17:09:14 GMT, Phil Race wrote: >> src/java.desktop/macosx/native/libosxapp/JNIUtilities.h line 41: >> >>> 39:NSLog(@"%@",[NSThread callStackSymbols]); \ >>> 40:if ([NSThread isMainThread] == NO) { \ >>> 41:if ((*env)->ExceptionOccurred(env) == NULL) { \ >> >> 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 : >> https://github.com/openjdk/jdk/blob/b72de3c5fc99f365e9fb25114ddd28eceddfa6e8/src/java.desktop/windows/native/libawt/windows/awt_Button.cpp#L357 >> >> Note that the exception in the static initializer is fatal for the >> application. > >> 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 : >> >> https://github.com/openjdk/jdk/blob/b72de3c5fc99f365e9fb25114ddd28eceddfa6e8/src/java.desktop/windows/native/libawt/windows/awt_Button.cpp#L357 >> >> Note that the exception in the static initializer is fatal for the >> application. > > Nothing new here. 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. - PR: https://git.openjdk.java.net/jdk/pull/1967
Re: RFR: 8259343: [macOS] Update JNI error handling in Cocoa code. [v3]
On Mon, 11 Jan 2021 19:27:12 GMT, Phil Race wrote: >> Proposed updates to JNI error handling. > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8259343: [macOS] Update JNI error handling in Cocoa code. src/java.desktop/macosx/native/libosxapp/JNIUtilities.h line 197: > 195: } \ > 196: if (getenv("JNU_NO_COCOA_EXCEPTION") == NULL) { \ > 197: [NSException raise:NSGenericException format:@"Java > Exception"]; \ 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? - PR: https://git.openjdk.java.net/jdk/pull/1967
Re: RFR: 8259343: [macOS] Update JNI error handling in Cocoa code. [v3]
On Mon, 11 Jan 2021 19:27:12 GMT, Phil Race wrote: >> Proposed updates to JNI error handling. > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8259343: [macOS] Update JNI error handling in Cocoa code. Build changes look good. - Marked as reviewed by erikj (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1967
Re: RFR: 8259343: [macOS] Update JNI error handling in Cocoa code. [v2]
On Mon, 11 Jan 2021 18:56:38 GMT, Erik Joelsson wrote: > 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. - PR: https://git.openjdk.java.net/jdk/pull/1967
Re: RFR: 8259343: [macOS] Update JNI error handling in Cocoa code. [v3]
> Proposed updates to JNI error handling. Phil Race has updated the pull request incrementally with one additional commit since the last revision: 8259343: [macOS] Update JNI error handling in Cocoa code. - Changes: - all: https://git.openjdk.java.net/jdk/pull/1967/files - new: https://git.openjdk.java.net/jdk/pull/1967/files/08a41150..913b37c2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1967=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1967=01-02 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/1967.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1967/head:pull/1967 PR: https://git.openjdk.java.net/jdk/pull/1967
Re: RFR: 8259343: [macOS] Update JNI error handling in Cocoa code. [v2]
On Mon, 11 Jan 2021 17:52:19 GMT, Phil Race wrote: >> Proposed updates to JNI error handling. > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8259343: [macOS] Update JNI error handling in Cocoa code. make/modules/java.desktop/Lib.gmk line 96: > 94: $(call SET_SHARED_LIBRARY_ORIGIN), \ > 95: LIBS := \ > 96: -ljava \ There should be a dependency declaration added too. Something like this right after the SetupJdkLibrary macro call: $(BUILD_LIBOSXAPP): $(call FindLib, java.base, java) - PR: https://git.openjdk.java.net/jdk/pull/1967
Re: RFR: 8259343: [macOS] Update JNI error handling in Cocoa code. [v2]
On Mon, 11 Jan 2021 05:58:28 GMT, Sergey Bylokhov wrote: >> src/java.desktop/macosx/native/libosxapp/JNIUtilities.h line 46: >> >>> 44: if ((*env)->ExceptionOccurred(env) != NULL) { \ >>> 45: (*env)->ExceptionDescribe(env); \ >>> 46: } \ >> >> 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. > > 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. I have a paren in the wrong place ! I've pushed an update. - PR: https://git.openjdk.java.net/jdk/pull/1967
Re: RFR: 8259343: [macOS] Update JNI error handling in Cocoa code. [v2]
On Mon, 11 Jan 2021 06:01:36 GMT, Sergey Bylokhov wrote: >> 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. > > 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/1967
Re: RFR: 8259343: [macOS] Update JNI error handling in Cocoa code. [v2]
> Proposed updates to JNI error handling. Phil Race has updated the pull request incrementally with one additional commit since the last revision: 8259343: [macOS] Update JNI error handling in Cocoa code. - Changes: - all: https://git.openjdk.java.net/jdk/pull/1967/files - new: https://git.openjdk.java.net/jdk/pull/1967/files/0e745aa6..08a41150 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1967=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1967=00-01 Stats: 3 lines in 1 file changed: 1 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1967.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1967/head:pull/1967 PR: https://git.openjdk.java.net/jdk/pull/1967
Re: RFR: 8259343: [macOS] Update JNI error handling in Cocoa code. [v2]
On Mon, 11 Jan 2021 05:52:02 GMT, Sergey Bylokhov wrote: > 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 : > > https://github.com/openjdk/jdk/blob/b72de3c5fc99f365e9fb25114ddd28eceddfa6e8/src/java.desktop/windows/native/libawt/windows/awt_Button.cpp#L357 > > Note that the exception in the static initializer is fatal for the > application. Nothing new here. - PR: https://git.openjdk.java.net/jdk/pull/1967
Re: RFR: 8259343: [macOS] Update JNI error handling in Cocoa code.
On Fri, 8 Jan 2021 19:17:17 GMT, Phil Race wrote: >> That code for sure should be called, it is even improved recently by >> JDK-8255681 >> I'll check how it was intended to work. > > 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/1967
Re: RFR: 8259343: [macOS] Update JNI error handling in Cocoa code.
On Wed, 6 Jan 2021 21:14:06 GMT, Phil Race wrote: > Proposed updates to JNI error handling. src/java.desktop/macosx/native/libosxapp/JNIUtilities.h line 41: > 39:NSLog(@"%@",[NSThread callStackSymbols]); \ > 40:if ([NSThread isMainThread] == NO) { \ > 41:if ((*env)->ExceptionOccurred(env) == NULL) { \ 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 : https://github.com/openjdk/jdk/blob/b72de3c5fc99f365e9fb25114ddd28eceddfa6e8/src/java.desktop/windows/native/libawt/windows/awt_Button.cpp#L357 Note that the exception in the static initializer is fatal for the application. - PR: https://git.openjdk.java.net/jdk/pull/1967
Re: RFR: 8259343: [macOS] Update JNI error handling in Cocoa code.
On Fri, 8 Jan 2021 19:05:09 GMT, Phil Race wrote: >> Proposed updates to JNI error handling. > > src/java.desktop/macosx/native/libosxapp/JNIUtilities.h line 46: > >> 44: if ((*env)->ExceptionOccurred(env) != NULL) { \ >> 45: (*env)->ExceptionDescribe(env); \ >> 46: } \ > > 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/1967
Re: RFR: 8259343: [macOS] Update JNI error handling in Cocoa code.
On Wed, 6 Jan 2021 21:14:06 GMT, Phil Race wrote: > Proposed updates to JNI error handling. src/java.desktop/macosx/native/libosxapp/JNIUtilities.h line 46: > 44: if ((*env)->ExceptionOccurred(env) != NULL) { \ > 45: (*env)->ExceptionDescribe(env); \ > 46: } \ 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. - PR: https://git.openjdk.java.net/jdk/pull/1967
Re: RFR: 8259343: [macOS] Update JNI error handling in Cocoa code.
On Fri, 8 Jan 2021 04:40:36 GMT, Sergey Bylokhov wrote: >> 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 ... > > But that could be merged to the CallXXXMethod and placed somewhere in the > libosxapp 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. >> 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. > > 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 Ok. I will only create my own if none is pending. >> 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. > > That code for sure should be called, it is even improved recently by > JDK-8255681 > I'll check how it was intended to work. 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. - PR: https://git.openjdk.java.net/jdk/pull/1967
Re: RFR: 8259343: [macOS] Update JNI error handling in Cocoa code.
On Thu, 7 Jan 2021 00:35:27 GMT, Sergey Bylokhov wrote: >> Proposed updates to JNI error handling. > > src/java.desktop/macosx/native/libosxapp/JNIUtilities.h line 180: > >> 178: * nature of the problem that has been detected and how survivable it >> is. >> 179: */ >> 180: #define CHECK_EXCEPTION() \ > > Since this macro does not try to return from the outer code we can change it > to the method? 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 ... > src/java.desktop/macosx/native/libosxapp/JNIUtilities.h line 41: > >> 39:NSLog(@"%@",[NSThread callStackSymbols]); \ >> 40:if ([NSThread isMainThread] == NO) { \ >> 41:JNU_ThrowInternalError(env, "Bad JNI Lookup"); \ > > It will be possible that(most of the time) the JNU_ThrowInternalError will be > called while we already have a raised java exception. 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. > src/java.desktop/macosx/native/libosxapp/JNIUtilities.h line 182: > >> 180: #define CHECK_EXCEPTION() \ >> 181: if ((*env)->ExceptionOccurred(env) != NULL) { \ >> 182: if ([NSThread isMainThread] == YES) { \ > > Isn't it is a similar code to the > https://github.com/openjdk/jdk/blob/67c221148159d94142a3a6d9ddadce2dff8c858b/src/java.desktop/macosx/native/libosxapp/NSApplicationAWT.m#L334 > 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/1967
Re: RFR: 8259343: [macOS] Update JNI error handling in Cocoa code.
On Fri, 8 Jan 2021 02:40:58 GMT, Phil Race wrote: >> src/java.desktop/macosx/native/libosxapp/JNIUtilities.h line 41: >> >>> 39:NSLog(@"%@",[NSThread callStackSymbols]); \ >>> 40:if ([NSThread isMainThread] == NO) { \ >>> 41:JNU_ThrowInternalError(env, "Bad JNI Lookup"); \ >> >> It will be possible that(most of the time) the JNU_ThrowInternalError will >> be called while we already have a raised java exception. > > 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. 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 >> src/java.desktop/macosx/native/libosxapp/JNIUtilities.h line 180: >> >>> 178: * nature of the problem that has been detected and how survivable it >>> is. >>> 179: */ >>> 180: #define CHECK_EXCEPTION() \ >> >> Since this macro does not try to return from the outer code we can change it >> to the method? > > 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 ... But that could be merged to the CallXXXMethod and placed somewhere in the libosxapp >> src/java.desktop/macosx/native/libosxapp/JNIUtilities.h line 182: >> >>> 180: #define CHECK_EXCEPTION() \ >>> 181: if ((*env)->ExceptionOccurred(env) != NULL) { \ >>> 182: if ([NSThread isMainThread] == YES) { \ >> >> Isn't it is a similar code to the >> https://github.com/openjdk/jdk/blob/67c221148159d94142a3a6d9ddadce2dff8c858b/src/java.desktop/macosx/native/libosxapp/NSApplicationAWT.m#L334 >> 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. > > 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. That code for sure should be called, it is even improved recently by JDK-8255681 I'll check how it was intended to work. - PR: https://git.openjdk.java.net/jdk/pull/1967
Re: RFR: 8259343: [macOS] Update JNI error handling in Cocoa code.
On Wed, 6 Jan 2021 21:14:06 GMT, Phil Race wrote: > Proposed updates to JNI error handling. src/java.desktop/macosx/native/libosxapp/JNIUtilities.h line 180: > 178: * nature of the problem that has been detected and how survivable it is. > 179: */ > 180: #define CHECK_EXCEPTION() \ Since this macro does not try to return from the outer code we can change it to the method? src/java.desktop/macosx/native/libosxapp/JNIUtilities.h line 182: > 180: #define CHECK_EXCEPTION() \ > 181: if ((*env)->ExceptionOccurred(env) != NULL) { \ > 182: if ([NSThread isMainThread] == YES) { \ Isn't it is a similar code to the https://github.com/openjdk/jdk/blob/67c221148159d94142a3a6d9ddadce2dff8c858b/src/java.desktop/macosx/native/libosxapp/NSApplicationAWT.m#L334 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. src/java.desktop/macosx/native/libosxapp/JNIUtilities.h line 41: > 39:NSLog(@"%@",[NSThread callStackSymbols]); \ > 40:if ([NSThread isMainThread] == NO) { \ > 41:JNU_ThrowInternalError(env, "Bad JNI Lookup"); \ It will be possible that(most of the time) the JNU_ThrowInternalError will be called while we already have a raised java exception. - PR: https://git.openjdk.java.net/jdk/pull/1967