PING: Re: RFR (XS): 8244571: assert(!_thread->is_pending_jni_exception_check()) failed: Pending JNI Exception Check during class loading

[serguei.spitsyn at oracle.com]

Hi David,

Thank you for the comments!


On 5/21/20 23:58, David Holmes wrote:
> Hi Serguei,
>
> On 22/05/2020 4:17 pm, serguei.spitsyn at oracle.com wrote:
>> PING: This is pretty small and easy to review fix.
>>
>> Thanks!
>> Serguei
>>
>>
>> On 5/19/20 09:28, serguei.spitsyn at oracle.com wrote:
>>> Please, review fix for:
>>> https://bugs.openjdk.java.net/browse/JDK-8244571
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.1/ 
>>>
>>>
>>> Summary:
>>>   There are two places in the native part of test that cause assert 
>>> and WARNING with the -Xcheck:jni.
>>>   The assert is because there is no check for pending exception 
>>> after the call to:
>>> jni->CallBooleanMethod(klass, is_hid_mid);
>>>   Using a JNI ExceptionCheck()after the call fixes the issue.
>
>   bool res = jni->CallBooleanMethod(klass, is_hid_mid);
>   if (jni->ExceptionCheck()) {
>     LOG0("is_hidden: Exception in jni CallBooleanMethod\n");
>   }
>   return res;
>
> That will fix the pending_jni_exception_check error, but if an 
> exception actually occurs what will be returned? And whatever is 
> returned, the callers of this method don't themselves check for 
> pending exceptions so they will treat it as if the exception didn't 
> occur - at least until we finally return to Java code. Perhaps any 
> exception should result in jni->FatalError as happens with any JVMTI 
> error?
You are right, it would be more clean to call jni->FatalError.
I was also thinking about it but also worried to get the exception details.
The exception can be printed before call to FatalError.


>>>   The following call to the JVM TI function:
>>> err = jvmti->GetClassLoaderClasses(loader, &count, &loader_classes);
>>>   produces the warning (with a java level stack trace): WARNING: JNI 
>>> local refs: 94, exceeds capacity: 32
>>>   It is because the GetClassLoaderClasses returns an array of local 
>>> references to the loader classes.
>>>   Using a JNI EnsureLocalCapacity() before the JVM TI call also 
>>> fixes the issue.
>
> The warning suggests using 1024 is a bit of overkill. :)

What capacity would be more reasonable, 256 or 512?
Let's pick 256. This is just a warning, the test is still passing.

Thanks!
Serguei

> Cheers,
> David
>
>>>
>>> Testing:
>>>   Running the test 
>>> test/hotspot/jtreg/serviceability/jvmti/HiddenClass locally.
>>>   Will run a mach5 job as well.
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>>
>>