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

[coleen.phillimore at oracle.com]

Hi Mandy,
Version 02 looks fine.  David already commented on the inconsistency we 
saw with protection domain.
thanks,
Coleen

On 10/5/17 5:56 PM, 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.
>
> 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?
>
> Mandy
>