RFR (S) 8210842: Handle JNIGlobalRefLocker.cpp

[David Holmes]

Hi Jc,

On 18/09/2018 1:59 PM, JC Beyler wrote:
> Hi all,
> 
> I've slowly started working on JDK-8191519 
> <https://bugs.openjdk.java.net/browse/JDK-8191519>. However before 
> starting to really refactor all the tests, I thought I'd get a few 
> opinions. I am working on internalizing the error handling of JNI calls 
> via a SafeJNIEnv class that redefines all the JNI calls to add an error 
> checker.
> 
> The advantage is that the test code will look and feel like normal JNI 
> code and calls but will have the checks we want to have for our tests. 

Not sure I get that. Normal JNI code has to check for errors/exceptions 
after every call. The tests need those checks too. Today they are 
explicit, with this change they become implicit. Not sure what we are 
gaining here ??

> If agreed with this, we can augment the SafeJNIEnv class as needed. 
> Also, if the tests require something else than fatal errors, we can add 
> a different marker and make it a parameter to the base class.
> 
> Webrev: http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.00/ 
> <http://cr.openjdk.java.net/%7Ejcbeyler/8210842/webrev.00/>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8210842
> 
> Let me know what you think,

Two initial suggestions:

1. FatalOnException should be constructed with a description string so 
that it can report the failing operation when calling FatalError rather 
than the general "Problem with a JNI call".

2. Make the local SafeJNIEnv a pointer called env so that the change is 
less disruptive. All the env->op() calls will remain and only the local 
error checking will be removed.

The switch from, e.g., checking NULL returns to checking for pending 
exceptions, need to be sure that the JNI operations clearly specify that 
NULL implies there will be an exception pending. This change may be an 
issue for static code analysis if not smart enough to understand the 
RAII wrappers.

Thanks,
David

> Jc