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

[David Holmes]

On 19/10/2017 2:03 PM, mandy chung wrote:
> 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.
>>
> 
> Coleen also studied this and no naked oop (GC could happen in the java 
> method call but loader is set after it).

loader will be unchanged if the call is from OnUnload.

> I will look into making it a handle which may feel more secure.
>> The test seems very lonely, where are the onLoad tests?
>>
> 
> The onLoad test is with JDK-8164512 which I plan to push to jdk10/master 
> but pending for this hotspot integration.  In this case, I think it's 
> good to have a separate test case (it's not the same as the onLoad tests).

No problem with separate test, just wondering if it should be somewhere 
else (with the other tests), or whether those other tests will end up in 
the FindClass directory?

BTW may be a while before hs pushes up to master.

> BTW there is another onLoad test in the closed test.
>> In Driver:
>>
>> 30  * @run main/othervm/native
>>
>> what does the "native" do? I don't see it documented.
>>
> 
> I have to find the documentation.  There are many hotspot tests using 
> native that builds native library.
> 
>> 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).
>>
> 
> Hmm.... let me take a closer look at it.

I already saw and commented on the bug you filed.

Thanks,
David

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