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

[David Holmes]

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
>