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

[coleen.phillimore at oracle.com]

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
>>>
>