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

2021-01-12 Thread Sergey Bylokhov
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]

2021-01-12 Thread Sergey Bylokhov
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]

2021-01-12 Thread Phil Race
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]

2021-01-12 Thread Phil Race
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]

2021-01-11 Thread Sergey Bylokhov
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]

2021-01-11 Thread Sergey Bylokhov
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]

2021-01-11 Thread Erik Joelsson
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]

2021-01-11 Thread Phil Race
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]

2021-01-11 Thread Phil Race
> 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]

2021-01-11 Thread Erik Joelsson
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]

2021-01-11 Thread Phil Race
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]

2021-01-11 Thread Phil Race
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]

2021-01-11 Thread Phil Race
> 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]

2021-01-11 Thread Phil Race
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.

2021-01-10 Thread Sergey Bylokhov
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.

2021-01-10 Thread Sergey Bylokhov
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.

2021-01-10 Thread Sergey Bylokhov
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.

2021-01-08 Thread Phil Race
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.

2021-01-08 Thread Phil Race
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.

2021-01-08 Thread Phil Race
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.

2021-01-08 Thread Sergey Bylokhov
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.

2021-01-08 Thread Sergey Bylokhov
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