Review Request JDK-8189193: FindClass mistakenly uses system class loader when the initiating loader is bootstrap loader

[mandy chung]

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/

I tested this with jdk core groups and hotspot group.

Mandy

On 10/19/17 10:12 AM, coleen.phillimore at 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
>>>>
>>
>