RFR (S): 8238676: jni crashes on accessing it from process exit hook

[David Holmes]

On 3/03/2020 9:13 am, gerard ziemski wrote:
> Sorry, yes I am OK with the change.
> 
> I think there is some room to interpretation here, and I'd have done it 
> slightly differently, but I'm fine with your interpretation.

Thanks Gerard!

David

> 
> cheers
> 
> On 2/28/20 11:16 PM, David Holmes wrote:
>> Hi Gerard,
>>
>> Are you okay with this?
>>
>> Thanks,
>> David
>>
>> On 26/02/2020 10:11 am, David Holmes wrote:
>>> Hi Gerard,
>>>
>>> Thanks for taking a look at this.
>>>
>>> On 26/02/2020 2:45 am, gerard ziemski wrote:
>>>> hi David,
>>>>
>>>> On 2/18/20 8:04 PM, David Holmes wrote:
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8238676
>>>>> webrev: http://cr.openjdk.java.net/~dholmes/8238676/webrev/
>>>>>
>>>>> If an application tries to use JNI from an atexit handler, the 
>>>>> attempt can't succeed (the VM has already logically terminated) but 
>>>>> it should not crash. The JNI Invocation API code was missing some 
>>>>> checks in places and wasn't aware of the possibility of trying to 
>>>>> make calls from the VMThread.
>>>>>
>>>>> Testing:
>>>>>  - new test added for the JNI Invocation API
>>>>>  - tiers 1-3
>>>>>
>>>>> Thanks,
>>>>> David
>>>>
>>>> In jni_GetEnv() we have:
>>>>
>>>> 4120   if (vm_created == 0) {
>>>> 4121     *penv = NULL;
>>>> 4122     ret = JNI_EDETACHED;
>>>> 4123     return ret;
>>>> 4124   }
>>>>
>>>> but in jni_DetachCurrentThread() we have:
>>>>
>>>> 4063   if (vm_created == 0) {
>>>> 4064     HOTSPOT_JNI_DETACHCURRENTTHREAD_RETURN(JNI_ERR);
>>>> 4065     return JNI_ERR;
>>>> 4066   }
>>>>
>>>> should lines 4064,4065 perhaps be:
>>>>
>>>> 4064 HOTSPOT_JNI_DETACHCURRENTTHREAD_RETURN(JNI_EDETACHED);
>>>> 4065     return JNI_EDETACHED;
>>>>
>>>> to be consistent?
>>>
>>> Well ... ideally the JNI spec would have considered the question of 
>>> "what if the JavaVM is no longer live?" and had an JNI_ENOJVM errro 
>>> code. But it doesn't. Nor does it allow for any JNI method to return 
>>> JNI_ERR for unexpected errors situations. So we have to adhere to the 
>>> specifications for each method.
>>>
>>> For GetEnv the spec only allows for returning JNI_EDETACHED, 
>>> JNI_EVERSION or JNI_OK. As you can't be attached to a VM that doesn't 
>>> exist then JNI_EDETACHED is the only possible return code - and it 
>>> isn't wrong, it just doesn't tell you why you aren't attached.
>>>
>>> If you apply the same logic to DetachCurrentThread, and follow the 
>>> spec then it states
>>>
>>> "Trying to detach a thread that is not attached is a no-op."
>>>
>>> which suggests we do nothing and report JNI_OK. But I think that 
>>> would be a disservice to the programmer in the case where the JVM is 
>>> no longer live. And we're allowed to return "a suitable JNI error 
>>> code on failure" so that is what I chose to do. And I return JNI_ERR 
>>> rather than JNI_EDETACHED for two reasons:
>>>
>>> 1. It is consistent with what we do for attach in the same circumstance:
>>>
>>> jint JNICALL jni_AttachCurrentThread(JavaVM *vm, void **penv, void 
>>> *_args) {
>>>    HOTSPOT_JNI_ATTACHCURRENTTHREAD_ENTRY(vm, penv, _args);
>>>    if (vm_created == 0) {
>>>      HOTSPOT_JNI_ATTACHCURRENTTHREAD_RETURN((uint32_t) JNI_ERR);
>>>      return JNI_ERR;
>>>    }
>>>
>>> 2. It would be confusing to return JNI_EDETACHED as that means 
>>> "thread detached from the VM" yet the spec says "Trying to detach a 
>>> thread that is not attached is a no-op.". So I need to convey that 
>>> there is a more specific underlying error here.
>>>
>>>> Looks good otherwise.
>>>
>>> Thanks for the review.
>>>
>>> David
>>> -----
>>>
>>>>
>>>> cheers
>>>>
>