[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: Review Request JDK-8189193: FindClass mistakenly uses system class loader when the initiating lo
From:       mandy chung <mandy.chung () oracle ! com>
Date:       2017-10-20 0:40:34
Message-ID: 32d4f20d-91e7-aa44-683d-1ecb461e0cbb () oracle ! com
[Download RAW message or body]



On 10/19/17 5:32 PM, David Holmes wrote:
> Hi Mandy,
>
> On 20/10/2017 4:11 AM, mandy chung wrote:
>> Thanks Coleen.     I renamed Driver.java to FindClassFromBoot.java 
>> which is a better test name than Driver (that should address David's 
>> comment).   No other change.
>>
>> http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8189193/webrev.02/
>
> Sorry I missed the test updates till now. I don't think you needed to 
> complicate the NCDFE handling at the native level. I would have just 
> defined a negative test that expects to get NCDFE. (If the subsequent 
> FindClass(env, "java/lang/NoClassDefFoundError"); itself fails (eg 
> OOME) then you may crash on the instanceof call.)
>
> Up to you whether to proceed with current approach or not.
>

I actually like the current approach that BootNativeLibrary::findClass 
to return null if class not found.   In fact this was my initial version 
until I realized FindClass throws NCDFE as well.   I don't find the 
native code complicated.

This test is a othervm test and so OOME should be very unlikely.

I will push as is.

Thanks
Mandy

> Thanks,
> David
>
>> I tested this with jdk core groups and hotspot group.
>>
>> Mandy
>>
>> On 10/19/17 10:12 AM, coleen.phillimore@oracle.com wrote:
>>> Looks great!
>>> Coleen
>>>
>>> On 10/19/17 11:53 AM, mandy chung wrote:
>>>> David, Coleen,
>>>>
>>>> I have revised the patch to use Handle (much simpler, thanks) and 
>>>> update the test to clear the exception:
>>>> http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8189193/webrev.01/index.html 
>>>>
>>>>
>>>> Thanks
>>>> Mandy
>>>>
>>>> On 10/18/17 5:26 PM, David Holmes wrote:
>>>>> Hi Mandy,
>>>>>
>>>>> On 19/10/2017 7:52 AM, mandy chung wrote:
>>>>>> The bug creeped in when I improved the code to use 
>>>>>> loader.is_null() to set to system class loader per Coleen's 
>>>>>> suggestion.   It's my oversight of the null loader case.   The is 
>>>>>> the version I pushed.
>>>>>> http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8188052/webrev.02/
>>>>>>
>>>>>> The previous version was correct.
>>>>>> http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8188052/webrev.01/
>>>>>
>>>>> Yes I see what happened - and we all missed it. And there was no 
>>>>> existing test it seems :(
>>>>>
>>>>>> I didn't go back to webrev.01. The fix is an improvement to it.
>>>>>
>>>>> Okay, only concern I have is that this:
>>>>>
>>>>> 403     oop loader = SystemDictionary::java_system_loader();
>>>>>
>>>>> certainly looks like a "naked oop" - and the Java call may lead to 
>>>>> GC occurring.
>>>>>
>>>>> The test seems very lonely, where are the onLoad tests?
>>>>>
>>>>> In   Driver:
>>>>>
>>>>> 30   * @run main/othervm/native
>>>>>
>>>>> what does the "native" do? I don't see it documented.
>>>>>
>>>>> Regarding this comment in BootLoaderTest:
>>>>>
>>>>> // The JNI spec says FindClass returns NULL if class not found but
>>>>> // it also says that NCDFE is thrown if no definition can be found.
>>>>> // The current implementation throws NCDFE.
>>>>>
>>>>> The NCDFE is set pending by FindClass. If you return (or call 
>>>>> into) java code then it will be thrown. But if you were remaining 
>>>>> in native then you'd use the NULL return to determine success or 
>>>>> failure (or check pending exception).
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> Mandy
>>>>>>
>>>>>> On 10/18/17 2:48 PM, David Holmes wrote:
>>>>>>> Hi Mandy,
>>>>>>>
>>>>>>> Do you still have the webrev for 8188052? I need to see how this 
>>>>>>> got through given we looked at it so many times. :(
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>>
>>>>>>> On 19/10/2017 7:30 AM, mandy chung wrote:
>>>>>>>> This is a regression caused by JDK-8188052. FindClass should 
>>>>>>>> only use system class loader when there is no caller class or 
>>>>>>>> when it's called from JNI_OnUnload. When there is a caller 
>>>>>>>> class, it should either use its class loader or the one 
>>>>>>>> associated with JNI_Onload.
>>>>>>>>
>>>>>>>> Webrev:
>>>>>>>> http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8189193/webrev.00/ 
>>>>>>>>
>>>>>>>>
>>>>>>>> JBS:https://bugs.openjdk.java.net/browse/JDK-8189193
>>>>>>>>
>>>>>>>> thanks
>>>>>>>> Mandy
>>>>>>
>>>>
>>>
>>

[prev in list] [next in list] [prev in thread] [next in thread]