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

Prasanta Sadhukhan prasanta.sadhukhan at oracle.com
Tue Sep 13 07:01:23 UTC 2016 

Looks ok to me.

Regards
Prasanta
On 9/12/2016 9:12 PM, Jayathirth D V wrote:
> Hi Prasanta,
>
> Thanks for your review.
>
> There is no need to RELEASE_ARRAYS() for GET_ARRAYS() in line number 578, 588, 994 or 1085.
> All these calls are coming internally from IJG library and if we have to just make sure that we include these up calls in RELEASE_ARRAY- GET_ARRAY pair.
> If everything goes right at the end of readImage(), readImageHeader(), writeImage() or writeImageHeader() we actually call RELEASE_ARRAY(), which will unpin these buffers which were acquired at the start of these functions.
>
> Regarding  "2856         RELEASE_ARRAYS(env, data, (const JOCTET *)(dest->next_output_byte));". We don't need this RELEASE_ARRAY() call at this place as we have not yet pinned any buffer. So I have removed it.
> Please find updated webrev for review :
> http://cr.openjdk.java.net/~jdv/8162461/webrev.02/
>
> And I have verified jtreg, JCK and JPRT for all the changes.
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Prasanta Sadhukhan
> Sent: Monday, September 12, 2016 2:05 PM
> To: Jayathirth D V
> Cc: 2d-dev
> Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-8162461: Hang due to JNI up-call made whilst holding JNI critical lock.
>
> 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
>>>>

More information about the 2d-dev mailing list