RFR: 8189633: Missing -Xcheck:jni checking for DeleteWeakGlobalRef

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8189633: Missing -Xcheck:jni checking for DeleteWeakGlobalRef

Kim Barrett
Please review this change to the checking form of DeleteWeakGlobalRef
to actually perform some checking.  The checking that is now being
done was cribbed from DeleteGlobalRef.

CR:
https://bugs.openjdk.java.net/browse/JDK-8189633

Webrev:
http://cr.openjdk.java.net/~kbarrett/8189633/open.00/

Testing:
Mach5 hs-tier1,2,3

I tried running tests locally with -Xcheck:jni, but that ran afoul of
JDK-8189766.

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189633: Missing -Xcheck:jni checking for DeleteWeakGlobalRef

David Holmes
Hi Kim,

On 3/01/2018 9:30 AM, Kim Barrett wrote:
> Please review this change to the checking form of DeleteWeakGlobalRef
> to actually perform some checking.  The checking that is now being
> done was cribbed from DeleteGlobalRef.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8189633
>
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8189633/open.00/

Doesn't the fact it is a weak-ref mean that resolving it could return
NULL, in which case validate_object will trigger a JNI fatal error:

oop jniCheck::validate_object(JavaThread* thr, jobject obj) {
     if (!obj)
         return NULL;
     ASSERT_OOPS_ALLOWED;
     oop oopObj = jniCheck::validate_handle(thr, obj);
     if (!oopObj) {
       ReportJNIFatalError(thr, fatal_bad_ref_to_jni);
     }
     return oopObj;
}

?

David
-----

> Testing:
> Mach5 hs-tier1,2,3
>
> I tried running tests locally with -Xcheck:jni, but that ran afoul of
> JDK-8189766.
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8189633: Missing -Xcheck:jni checking for DeleteWeakGlobalRef

Kim Barrett
> On Jan 3, 2018, at 12:31 AM, David Holmes <[hidden email]> wrote:
>
> Hi Kim,
>
> On 3/01/2018 9:30 AM, Kim Barrett wrote:
>> Please review this change to the checking form of DeleteWeakGlobalRef
>> to actually perform some checking.  The checking that is now being
>> done was cribbed from DeleteGlobalRef.
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8189633
>> Webrev:
>> http://cr.openjdk.java.net/~kbarrett/8189633/open.00/
>
> Doesn't the fact it is a weak-ref mean that resolving it could return NULL, in which case validate_object will trigger a JNI fatal error:
>
> oop jniCheck::validate_object(JavaThread* thr, jobject obj) {
>    if (!obj)
>        return NULL;
>    ASSERT_OOPS_ALLOWED;
>    oop oopObj = jniCheck::validate_handle(thr, obj);
>    if (!oopObj) {
>      ReportJNIFatalError(thr, fatal_bad_ref_to_jni);
>    }
>    return oopObj;
> }
>
> ?

You’re right.  I thought I’d used validate_handle, but obviously not.

I think I should leave this issue alone for now, and come back to it after JDK-8194312 is done.
There are some changes to jniCheck there already, and I think some more, like here, can
be simplified or improved by relying on the JDK-8194312 changes to JNIHandles.