Review Request JDK-8188052: JNI_FindClass needs to specify the class loading context used for library lifecycle hooks

[David Holmes]

On 6/10/2017 7:56 AM, mandy chung wrote:
> On 10/5/17 1:13 PM, coleen.phillimore at oracle.com wrote:
>>
>> I should say, besides these style comments this change looks good.
>> Coleen
>>
>> On 10/5/17 3:28 PM, coleen.phillimore at oracle.com wrote:
>>>
>>> http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8188052/webrev.00/src/hotspot/share/prims/jni.cpp.udiff.html 
>>>
>>>
>>> The type Klass has a class_loader() and protection_domain() accessors 
>>> so it is unnecessary to cast to InstanceKlass.  We went through and 
>>> removed many of these unnecessary InstanceKlass::casts.
>>>
> 
> That's from existing code which may not be touched for a long time. 
> Since I'm on this file, I can clean this up.
>>> 407     if (loader.is_null() &&
>>>
> 
> - if (loader.is_null() &&
> + if (k->class_loader() == NULL && Already fixed per David's comment.
> 
>>> I agree with David's comment that this line will always evaluate to 
>>> true so should be cleaned up also.
>>>
>>> 413                              thread);
>>>  414       if (HAS_PENDING_EXCEPTION) {
>>>  415         Handle ex(thread, thread->pending_exception());
>>>  416         CLEAR_PENDING_EXCEPTION;
>>>  417         THROW_HANDLE_0(ex);
>>>  418       }
>>>
>>>
>>> This is strange too.  Why doesn't this just pass CHECK_NULL at line 
>>> 413, which is the same thing as checking for and clearing and 
>>> throwing the pending exception?
>>>
> 
> This is existing code.   I removed these lines and pass CHECK_NULL to 
> call_static.

Which, by the way, highlights the existing incorrect use of '0' instead 
of NULL in this code.

> How does this version look?
> http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8188052/webrev.02/src/hotspot/share/prims/jni.cpp.sdiff.html 
> 
> 
> One other thing I notice: it only sets the protection_domain to the 
> caller class when FindClass is called from JNI_OnLoad/JNI_OnUnload.
> 
> For other caller class, it uses null protection_domain. I would expect 
> it should use the caller's protection domain  to look up a class.  I 
> think it's a bug, isn't?

Neither Class.forName nor ClassLoader.loadClass take ProtectionDomains 
as arguments, so I think the "normal" behaviour of FindClass to not use 
a PD is correct. That of course begs the question as to why the 
OnLoad/OnUnload contexts are special. I don't have an answer but can do 
some research.

Thanks,
David

> Mandy
>