RFR (S): 8194309: JNI handle allocation failure not reported correctly

[coleen.phillimore at oracle.com]

On 7/23/20 9:39 AM, David Holmes wrote:
> On 23/07/2020 10:46 pm, coleen.phillimore at oracle.com wrote:
>>
>> http://cr.openjdk.java.net/~dholmes/8194309/webrev.with-test-hooks/src/hotspot/share/runtime/jniHandles.cpp.udiff.html 
>>
>>
>> These functions shouldn't use UseNewCode which is false by default. 
>> UseNewCode is for testing experimental features locally, not for 
>> checked in code.  You should use logging for this.
>
> That code is not being checked in. It was purely for my testing purposes.

I really need to start reading to the bottom of the mail, I clicked 
first.  The real version looks good to me.
Coleen

>
> David
> -----
>
>> Coleen
>>
>> On 7/23/20 4:52 AM, David Holmes wrote:
>>> webrev: http://cr.openjdk.java.net/~dholmes/8194309/webrev/
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8194309
>>>
>>> The JNI specification states that NewLocalref and NewGloalRef return 
>>> NULL on out-of-memory, whilst NewWeakGlobalRef throws 
>>> OutofMemoryError. But the hotspot implementation will abort on 
>>> out-of-memory in all cases.
>>>
>>> The oop storage code for global and weak-global handles already 
>>> supports taking an AllocFailStrategy, so we simply pass the 
>>> RETURN_NULL strategy through - and in the weak case throw a newly 
>>> defined OutOfMemoryError for "C heap space" (as a corollary for 
>>> "Java heap space").
>>>
>>> For NewLocalRef we pass the strategy through to 
>>> JNIHandleBlock::allocate_block, where we explicitly use "new 
>>> (std::nothrow)" to get a NULL on out-of-memory, so that we can pass 
>>> it back.
>>>
>>> There are three internal calls to NewGlobalRef in jni.cpp that 
>>> needed a NULL check added to support the new behaviour.
>>>
>>> In reality we know that if any of these things actually return NULL 
>>> because C-heap is exhausted then chances are we are going to abort 
>>> soon in any case. But to be spec compliant we make the changes.
>>>
>>> Note that I deliberately do not change any of the internal 
>>> JNIHandle::make_local calls (contained in the majority of JNI 
>>> methods) to get NULL on out-of-memory. This is because none of those 
>>> APIs are specified in a way that even considers what should happen 
>>> if an internal request to create a local-ref fails - so we can 
>>> neither return NULL nor throw an exception in general. All this fix 
>>> addresses are the three specific JNI entry points themselves.
>>>
>>> Also note there is no attempt with this changeset to add NULL checks 
>>> to all the JNI code in the other JDK libraries that uses these 
>>> API's. Interestingly quite a number already include the NULL checks.
>>>
>>> Testing:
>>>
>>> There is no practical way to test this for real so I had to use 
>>> fault-injection. A version of the webrev with the fault-injection 
>>> hooks and a test case, is presented here (for the record):
>>>
>>> http://cr.openjdk.java.net/~dholmes/8194309/webrev.with-test-hooks/
>>>
>>> The test is constructed so that only the JNI calls in the test 
>>> should possibly encounter the NULL returns.
>>>
>>> Otherwise only sanity testing of tiers 1-3.
>>>
>>> Thanks,
>>> David
>>