Hi Naoto, the new webrev looks ok to me, too. Thanks for fixing.
And I agree to the point that the NULL check can be removed...if you like. Best regards Christoph > -----Original Message----- > From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On Behalf > Of David Holmes > Sent: Mittwoch, 7. Dezember 2016 03:15 > To: Naoto Sato <naoto.s...@oracle.com>; core-libs-dev <core-libs- > d...@openjdk.java.net> > Subject: Re: [9] RFR: 8170465, 8170466: JNI exception pending in > jni_util.c:190 > > On 7/12/2016 11:04 AM, Naoto Sato wrote: > > Thanks, David. The fix is modified accordingly: > > > > http://cr.openjdk.java.net/~naoto/8170465.8170466/webrev.01/ > > Thanks for fixing that. > > The null checks after the exception checks are now redundant as you can > only get a null return from those methods if an exception becomes > pending. But leaving them in as a precaution is okay I guess. > > Thanks, > David > > > Naoto > > > > On 12/6/16 4:53 PM, David Holmes wrote: > >> Hi Naoto, > >> > >> On 7/12/2016 10:02 AM, Naoto Sato wrote: > >>> Hello, > >>> > >>> Please review the proposed fix to the following issues: > >>> > >>> https://bugs.openjdk.java.net/browse/JDK-8170465 > >>> https://bugs.openjdk.java.net/browse/JDK-8170466 > >>> > >>> The proposed fix is located at: > >>> > >>> http://cr.openjdk.java.net/~naoto/8170465.8170466/webrev.00/ > >>> > >>> These two JNI calls (NewStringUTF() and JNU_CallMethodByName()) in > >>> src/java.base/share/native/libjava/jni_util.c can throw an exception. > >>> Those possible exceptions need to be properly checked. > >> > >> JNU_CHECK_EXCEPTION will do a return from the method in which it is > >> placed, so you need to be careful to ensure you do all needed cleanup > >> before that eg: > >> > >> 203 JNU_CHECK_EXCEPTION(env); > >> 204 free(str1); > >> > >> needs to be reversed else we won't free str1. Similarly: > >> > >> 210 JNU_CHECK_EXCEPTION(env); > >> 211 (*env)->DeleteLocalRef(env, s2); > >> > >> needs to be reversed so we don't leak the local ref. (It is safe to call > >> DeleteLocalRef with an exception pending.) > >> > >> Thanks, > >> David > >> > >>> Naoto
