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

[David Holmes]

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.

Thanks,
David

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