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

[Jayathirth D V]

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
>