[OpenJDK 2D-Dev] [9] RFR JDK-8162461: Hang due to JNI up-call made whilst holding JNI critical lock.

[Prasanta Sadhukhan]

Don't we need to call RELEASE_ARRAYS for these GET_ARRAYS 578 || 
!GET_ARRAYS(env, data, &(src->next_input_byte))) {

588 || !GET_ARRAYS(env, data, 994             || !GET_ARRAYS(env, data, &(src->next_input_byte))) {

1085             || !GET_ARRAYS(env, data, &(src->next_input_byte))) {

Also, we are calling
2856         RELEASE_ARRAYS(env, data, (const JOCTET *)(dest->next_output_byte));

before

2861             JNU_ThrowByName(env, "javax/imageio/IIOException", buffer);

but not before
2843         JNU_ThrowByName( env,
2844                          "java/lang/OutOfMemoryError",
2845                          "Writing JPEG Stream");
Shouldn't we need to call RELEASE_ARRAYS before line 2843 too?
I don't see any GET_ARRAYS here so probably 2856's RELEASE_ARRAYS is not needed!!


Lastly, I hope JPRT passes with this change.

Regards
Prasanta
On 9/9/2016 2:56 AM, Philip Race wrote:
> I think the v.1 will be better even if it is not showing up as an issue,
> given that you seem to have a safe + simple fix to that part
>
> So "+1" to "1"
>
> -phil.
>
> On 9/8/16, 8:09 AM, Jayathirth D V wrote:
>> Hi Phil,
>>
>> More observations:
>> All emit_message() calls come under macros defined in jerror.h like 
>> WARNXX and TRACEXX.
>> I made changes in IJG library so that we call these WARNXX and 
>> TRACEXX macros forcefully in turn calling 
>> emit_message()(emit_message() also changed to call output_message() 
>> everytime).
>>
>> With the above change and without RELEASE/GET_ARRAYS change in 
>> sun_jpeg_output_message() also JVM is not throwing any error. It 
>> means when we enter sun_jpeg_output_message()  we don't have any 
>> active JNI critical lock.
>>
>> We can actually use 
>> http://cr.openjdk.java.net/~jdv/8162461/webrev.00/ without 
>> RELEASE/GET_ARRAYS change in sun_jpeg_output_message()  or we can use 
>> http://cr.openjdk.java.net/~jdv/8162461/webrev.01/ having 
>> RELEASE/GET_ARRAYS change in sun_jpeg_output_message() for future 
>> proofing.
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Jayathirth D V
>> Sent: Thursday, September 08, 2016 7:21 PM
>> To: Philip Race; 2d-dev
>> Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-8162461: Hang due to JNI 
>> up-call made whilst holding JNI critical lock.
>>
>> Hi Phil,
>>
>> Thanks for pointing me to  sun_jpeg_output_message().
>>
>> We need to make up calls from sun_jpeg_output_message()  as it is 
>> needed if user has added IIOReadWarningListener.
>> In imageioJPEG.c we are overriding IJG output_message() of jerror.c 
>> with our own  sun_jpeg_output_message().
>>
>> Call from IJG library can reach output_message() through two 
>> functions in jerror.c :
>>     1) error_exit()
>>     2) emit_message()
>>
>> We are overriding error_exit() also with sun_jpeg_error_exit() where 
>> we are not calling output_message(), so sun_jpeg_output_message() can 
>> be reached only through emit_message() .
>>
>> emit_message() always takes j_common_ptr as argument and not 
>> j_compress_ptr or j_decompress_ptr. But I noticed that before calling 
>> emit_message() we are always type casting j_compress_ptr or 
>> j_decompress_ptr to j_common_ptr and passing it as argument to 
>> emit_message().
>>
>> Since cinfo->  is_decompressor tells us whether it is read or write 
>> operation we can cast j_common_ptr to j_compress_ptr or 
>> j_decompress_ptr. Based on this I am creating jpeg_source_mgr or 
>> jpeg_destination_mgr object. Using this we can call 
>> RELEASE/GET_ARRAYS in sun_jpeg_output_message().
>>
>> Please find updated webrev for review:
>> http://cr.openjdk.java.net/~jdv/8162461/webrev.01/
>>
>> Parallel to this I tried using two separate functions like 
>> sun_jpeg_reader_output_message(j_decompress_ptr)  and 
>> sun_jpeg_writer_output_message(j_compress_ptr). But then we need to 
>> replicate error_exit() and emit_message() also to accept 
>> j_decompress_ptr and j_compress_ptr. It needs lot of changes at many 
>> places where we are using error_exit() and emit_message() in IJG 
>> library.
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Phil Race
>> Sent: Wednesday, September 07, 2016 10:57 PM
>> To: Jayathirth D V; 2d-dev
>> Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-8162461: Hang due to JNI 
>> up-call made whilst holding JNI critical lock.
>>
>> This all looks reasonable. But I wonder if you missed something.
>> Take a look at sun_jpeg_output_message(). That may also make up-calls.
>> A pointer to this function is passed to the IJG library and I don't 
>> know the circumstances under which it may call this function.
>> If it ever might do it whilst we are holding these locks then that 
>> would mean we'd need the same RELEASE/GET in there .. although the 
>> challenge would be that it does not have access to the data to 
>> release the arrays.
>> So
>> 1) Investigate if it is an actual issue,
>> 2) If it is, then decide between a way to ensure the arrays are 
>> available to this function (looks tricky to me), or somehow deferring 
>> these up-calls or eliminating them.
>>
>> -phil.
>>
>> On 9/6/2016 11:49 PM, Jayathirth D V wrote:
>>> Fixed typo.
>>>
>>> *From:* Jayathirth D V
>>> *Sent:* Wednesday, September 07, 2016 12:11 PM
>>> *To:* Philip Race; 2d-dev
>>> *Subject:* [OpenJDK 2D-Dev] [9] RFR JDK-8162461: Hang due to JNI
>>> up-call made whilst holding JNI critical lock.
>>>
>>> Hi,
>>>
>>> Please review the following fix in JDK9 at your convenience:
>>>
>>> Bug : https://bugs.openjdk.java.net/browse/JDK-8162461
>>>
>>> Webrev : http://cr.openjdk.java.net/~jdv/8162461/webrev.00/
>>> <http://cr.openjdk.java.net/%7Ejdv/8162461/webrev.00/>
>>>
>>> Issue : If we try to perform operations like
>>> reader.abort()/reader.dispose()/ reader.reset() in any of the
>>> IIOReadUpdateListener callbacks, JVM will throw deadlock error.
>>>
>>> Root cause : We are making callbacks from native side to Java by
>>> holding some resources in JNI critical lock.
>>>
>>> Solution : We have to release the JNI critical lock on the resources
>>> before we call Java function from native side. If we have JNI critical
>>> lock and we throw an Exception in that case also we should release the
>>> lock.
>>>
>>> Thanks,
>>>
>>> Jay
>>>