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
