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

[mandy chung]

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

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

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.

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