[OpenJDK 2D-Dev] RFR: 8172813 test/java/awt/font/JNICheck/JNICheck.sh fails on Linux

[Prahalad Kumar Narayanan]

Hello Phil

Thanks for the detailed follow-up. The change looks good.

Btw, a small correction in the webrev link. ( , has been replaced with . )
http://cr.openjdk.java.net/~prr/8172813.2

Thanks
Have a good day

Prahalad N.


-----Original Message-----
From: Phil Race 
Sent: Thursday, January 19, 2017 5:36 AM
To: Prahalad Kumar Narayanan; 2d-dev at openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] RFR: 8172813 test/java/awt/font/JNICheck/JNICheck.sh fails on Linux

A couple of updates made : http://cr.openjdk.java.net/~prr/8172813,2

Comments in-line below

-phil.



On 01/18/2017 09:51 AM, Prahalad Kumar Narayanan wrote:
> Hello Phil
>
> The change looks good - Reduces the number of local references held by the JNI function.
> Here are some findings :
>
> 1.  I found additional references that could be released. You can check the severity of the problem and decide to release or skip.
>             Line: 1129    Concerned Object: cacheDirArray.
>                          I believe the existing code may not create trouble as reference to object is outside the loop.

This is not really any different than the hundreds of other JNI methods that use some small number of local refs. It is indeed the ones in a loop that matter here.
>
>             Line: 1168 and
>             Line: 1175     Concerned Object: fcCompFontObj
>                           Both these lines, either terminate /or continue the loop without releasing the reference.

1168 I updated but 1175 causes a return to Java so freeing happens then automatically.
>
>             Line: 1330     Concerned Object: fcFont
>                           When jstr becomes NULL, for loop is exited 
> with break without releasing the reference

I looked at these before the first webrev and decided they weren't a problem in practice since this really should never happen.
>
>             Line: 1364    Concerned Object: fcFontArr
>                          fcFontArr (new object array ) is created in every iteration of the loop.
>                          This could be released in this line alongside 
> release of fcCompFontObj

I added a Delete here - line 1368 in the new version.
>
> 2. Copyright notice to have 2017 instead of 2016

I rarely-to-never bother to update these. A script will be run at some point.

>
> 3. Question: Should we mask the other warning messages with 'grep -v' in JNICheck.sh ?

I am not sure what you are asking here.
I am masking the other warnings with grep -v .. and since I added doing that obviously I think we should :-)

-phil.

>
> Thanks
> Have a good day
>
> Prahalad N.
>
> ------------------------------
>
> Message: 2
> Date: Tue, 17 Jan 2017 12:29:24 -0800
> From: Phil Race <philip.race at oracle.com>
> To: Sergey Bylokhov <sergey.bylokhov at oracle.com>
> Cc: 2d-dev <2d-dev at openjdk.java.net>
> Subject: Re: [OpenJDK 2D-Dev] RFR: 8172813
> 	test/java/awt/font/JNICheck/JNICheck.sh fails on Linux
> Message-ID: <65d232e2-47e4-2a78-c454-c144876af564 at oracle.com>
> Content-Type: text/plain; charset=windows-1252; format=flowed
>
> Both yes and no :-), although I think it is fine to add both.
>
> 1140 .. there are no more than 2 cachedirs (1 user + 1 system) so I 
> didn't examine JNI refs in this loop
>
> 1166 .. I was not realistically expecting a "NULL" here so ignored it.
>
> Unlike some of the JNI bugs "call while pending exception" type bugs we've fixed these were actual, not hypothetical.
>
> Updated webrev:
>
> http://cr.openjdk.java.net/~prr/8172813.1
>
>
> -phil.
>
>
> On 01/16/2017 09:07 AM, Sergey Bylokhov wrote:
>> Hi, Phil.
>> Should we add DeleteLocalRef in fontpath.c at lines 1140 and 1166, or both were skipped intentionally?
>>
>>> Bug : https://bugs.openjdk.java.net/browse/JDK-8172813
>>> Webrev : http://cr.openjdk.java.net/~prr/8172813/
>>>
>>> - Clear local refs once we are done with them.
>>> - Use grep to filter out from the log unrelated warnings as separately reported here :
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8131136
>>>
>>> -phil.