[OpenJDK 2D-Dev] [9] Review Request- JDK-8152971- JNI Warning with -Xcheck:jni

Prasanta Sadhukhan prasanta.sadhukhan at oracle.com
Fri Jul 15 05:49:32 UTC 2016


+1.

Regards
Prasanta
>
> On 07/14/2016 01:34 AM, Prahalad Kumar Narayanan wrote:
>> Hello Everyone
>>
>> Good day to you.
>> A quick follow up on the bug fix for : JDK-8152971
>>
>> First, Thanks to Phil for his time in review.
>>
>> I ‘ve incorporated Phil ‘s suggestions and the webrev with 
>> modifications are available under:
>>         Link: http://cr.openjdk.java.net/~pnarayanan/8152971/webrev.05/
>>
>> Quick Summary on the Changes:
>>         . Explicity deletion of local refs in 
>> Java_sun_awt_Win32FontManager_populateFontFileNameMap0 have been 
>> removed.
>>                  . Since the references are not many, they will be 
>> reclaimed by JVM once JNI function returns
>>         . The variable fontKeyName has been removed and the constant 
>> string FONTKEY_NT has been used directly.
>>
>> Kindly review at your convenient time and share your feedback.
>>
>> Thanks for your time
>> Have a good day
>>
>> Prahalad N.
>>
>>
>> -----Original Message-----
>> From: Philip Race
>> Sent: Wednesday, July 13, 2016 10:26 PM
>> To: Prahalad Kumar Narayanan
>> Cc: Prasanta Sadhukhan; 2d-dev at openjdk.java.net; Praveen Srivastava
>> Subject: Re: [9] Review Request- JDK-8152971- JNI Warning with 
>> -Xcheck:jni
>>
>> Much cleaner .. although I also do not think we need to explicitly
>> delete the local refs in
>> Java_sun_awt_Win32FontManager_populateFontFileNameMap0
>>
>> since the return will do that.
>>
>> And a little clean up .. the variable fontKeyName does not
>> seem to be needed any more. Just directly use FONTKEY_NT.
>>
>> -phil.
>>
>> On 7/11/16, 12:02 PM, Prahalad Kumar Narayanan wrote:
>> Hello Everyone
>>
>> Good day to you.
>>
>> A quick follow-up on the bug fix for : JDK-8152971     JNI Warning 
>> with -Xcheck:jni
>>
>> First, Thanks to Phil for the time in review and detailed explanation.
>>
>> To answer the question -
>>       "The issue with local refs is that you want to avoid too many 
>> so there is no need to do that release before return explicitly since 
>> the VM will do it for us. same for the direct calls to 
>> deletelocalref. Do you get any JNI warnings if you remove these ? "
>> Answer :
>>         Yes., I see the warnings if DeleteLocalRef is not invoked on 
>> objects created in the callback functions.
>>                   It is in these callback functions that many objects 
>> get created without a corresponding release api.
>>         No warnings were seen when releaseGdiFontMapReferences() was 
>> taken out.
>>                   This function is removed in the latest change.
>>                   The behavior could be attributed to two points
>>                          a. FontMap having most of its references to 
>> jObjects that are on stack (as arguments to function).
>>                          b. Very few references to local objects are 
>> created in the parent JNI function ( 2 - 3 classIDs )
>>
>> To Summarize the Changes
>>        . In all callback functions, the DeleteLocalRef calls have 
>> been reorganized to avoid cluttering before return statements.
>>        . releaseGdiFontMapReferences() function has been removed.
>>                  The only font map reference that is released in 
>> latest code - is the reference to jClass object along with other 
>> classID references. This could be avoided but has been retained as a 
>> safe measure.
>>
>> The changes are available under webrev link:
>>        http://cr.openjdk.java.net/~pnarayanan/8152971/webrev.04/
>>
>> Kindly review at your convenient time and provide your suggestions.
>>
>> Thanks for your review & feedback
>> Have a good day
>>
>> Prahalad N.
>>
>>
>> -----Original Message-----
>> From: Phil Race
>> Sent: Friday, July 08, 2016 1:52 AM
>> To: Prahalad Kumar Narayanan
>> Cc: Prasanta Sadhukhan; 2d-dev at openjdk.java.net; Jayathirth D V; 
>> Praveen Srivastava
>> Subject: Re: [9] Review Request- JDK-8152971- JNI Warning with 
>> -Xcheck:jni
>>
>>
>> So far as I can see we can eliminate at least some of the clutter of 
>> the calls to deletelocalreference by simply doing it as soon as we 
>> are done with the reference
>>
>> eg
>>
>> 309     familyLC = (*env)->CallObjectMethod(env, fmi->family,
>>    310 fmi->toLowerCaseMID, fmi->locale);
>>
>>
>> can be immediately followed by :
>>      DeleteLocalReference(env, familyLC);
>>
>>
>> .. whether or not there is a pending exception.
>>
>> and
>>
>> 343     (*env)->CallObjectMethod(env, fmi->familyToFontListMap,
>>    344                              fmi->putMID, familyLC, fmi->list);
>>
>> can be immediately followed by :
>>
>>      DeleteLocalReference(env, fmi->family);
>>      DeleteLocalReference(env, fmi->list);
>>
>> .. whether or not there is a pending exception.
>>
>> and so on.
>>
>> releaseGdiFontMapReferences() seems to be used right before returning 
>> to Java.
>> The issue with local refs is that you want to avoid too many so there 
>> is no need to do that release before return explicitly since the VM 
>> will do it for us.
>> same for the direct calls to deletelocalref.
>> Do you get any JNI warnings if you remove these ?
>>
>>
>> I could even argue that some of the ones before return (0) from the 
>> enum functions which will cause us to then exit up the call stack 
>> fall into the same bucket but if you can simplify the change as I 
>> suggest it'll be easier to sort that out as there'll be less noise.
>>
>>
>> -phil.
>>
>>
>> On 07/04/2016 02:43 AM, Prahalad Kumar Narayanan wrote:
>> Hello Everyone
>>
>> Good day to you.
>>
>> A quick follow-up on the fix for
>>              Bug ID : JDK-8152971
>>              Title     : -Xcheck:jni - WARNING in native method
>>
>> Thanks to Phil for his feedback.
>> The feedback was not only detailed.. but contained important links 
>> for reference.
>>
>> I 've incorporated the review suggestions and have updated the webrev.
>> Brief on changes from previous revision:
>>
>>         1. Once an exception has been found in an Up-call, the 
>> exception is ' not ' consumed or cleared now.
>>                  . As Phil rightly said, this will pave way for the 
>> Java methods to know of the exceptions and handle them.
>>
>>         2. In addition to point No.1, The GDI's callback functions 
>> return zero '0' during an exception.
>>                  . This will stop the subsequent enumeration of fonts &
>> calls to registered callbacks through the APIs
>>
>>         3. Since exceptions are not consumed, all GDI 's callback 
>> functions are checked for pending exceptions at the entry.
>>                 . On pending exceptions, the GDI's callback functions 
>> return immediately.
>>                          . This prevents the functions from invoking 
>> any other JNI calls that could be un-safe during a pending exception
>>                          . The callback functions merely release any 
>> local reference held before returning to caller.
>>
>>         4. Since we no longer support Win98, the obsolete ANSI 
>> versions of functions (~A functions) have been removed.
>>                . Functions that work with Unicode strings invoking 
>> Unicode versions of Windows APIs (~W functions) are retained.
>>            5. The jtreg shell script has been modified to execute on 
>> all platforms after testing its execution on Windows, Linux, Mac and 
>> SunOS.
>>               . Just to re-iterate: The jtreg script fails on windows 
>> and solaris but in java.lang codebase.
>>               . A bug is already Open indicating JNI warnings in 
>> java.lang package. Hence this change does not introduce any new issues.
>>
>> As with any proposed webrev:
>>        . The changes were run through internal build system to ensure 
>> safe build on all supported OS versions.
>>        . JCK and JTREG test cases were run to ensure no conformance 
>> or regression failures occur.
>>
>> Kindly review the changes at your convenience and provide your feedback.
>>          . Updated webrev link:
>> http://cr.openjdk.java.net/~pnarayanan/8152971/webrev.03/
>>
>> Thank you for your time & effort in review Have a good day
>>
>> Prahalad N.
>>
>> ----------------------------------------------------------------------
>> ------
>> From: Philip Race
>> Sent: Wednesday, June 29, 2016 10:48 PM
>> To: Prasanta Sadhukhan
>> Cc: Prahalad Kumar Narayanan; 2d-dev at openjdk.java.net; Jayathirth D V;
>> Praveen Srivastava
>> Subject: Re: [9] Review Request- JDK-8152971- JNI Warning with
>> -Xcheck:jni
>>
>> https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/design
>> .html#exception_handling
>>
>> lists DeleteLocalRef as safe to call with a pending exception.
>>
>> So here ...
>> 74     fullnameLC = (*env)->CallObjectMethod(env, fullname,
>>    175 fmi->toLowerCaseMID, fmi->locale);
>>    176     if ((*env)->ExceptionCheck(env)) {
>>    177         (*env)->ExceptionClear(env);
>>    178         /* Delete the created references */
>>    179         DeleteLocalReference(env, fullname);
>>    180         return 1;
>>    181     }
>>
>> ... perhaps what we should do is not clear the exception and with the
>> goal that after returning from this function we can check in the
>> caller for a pending exception and return from JNI to Java *without
>> clearing it* - so that Java code gets that exception propagated. I
>> suggest this because I think we would have a correctness issue which
>> should be reported - not swallowed - if there ever really was one.
>>
>> I think this means that the GDI callbacks need to check for a pending
>> exception on entry and bail since once we hand off to windows it may
>> be called repeatedly.
>>
>> But also we should stop enumeration if we detect an error hence we
>> should return 0 in this case, not 1 per the docs for EnumFontFamExProc
>>
>> ---
>> https://msdn.microsoft.com/en-us/library/windows/desktop/dd162618(v=vs
>> .85).aspx
>>
>> Return value
>>
>> The return value must be a nonzero value to continue enumeration;
>>    to stop enumeration, the return value must be zero.
>>
>> ---
>>
>> Also the "A" functions are now obsolete. No win 98 support any more
>> :-) We should just delete them instead of updating them.
>>
>> And I'd prefer the test be verified on Solaris rather than excluding
>> it
>>
>> -phil
>>
>>
>> On 6/27/16, 10:24 PM, Prasanta Sadhukhan wrote:
>> Looks good to me. Only thing is in test script, you need to add 
>> copyright 2015, 2016 as the script is originally written in 2015 and 
>> you cannot omit the created year from the copyright.
>>
>> Regards
>> Prasanta
>> On 6/27/2016 4:17 PM, Prahalad Kumar Narayanan wrote:
>>
>> Hello Everyone
>>
>> Good day to you.
>>
>> Quick follow up to fix for the Bug
>>          Bug ID     : JDK-8152971
>>          Bug Link : https://bugs.openjdk.java.net/browse/JDK-8152971
>>
>> First, Thanks to Prasanta for his review & suggestions.
>>
>> I 've incorporated some of the review suggestions & updated the webrev.
>>         Webrev Link :
>> http://cr.openjdk.java.net/~pnarayanan/8152971/webrev.02/
>>
>> Overview on changes from previous webrev:
>>
>> 1. Code Changes: As Prasanta mentioned,
>>              The variable fontStrLC should be used in place of 
>> fontStr - The correction has been taken up.
>>              And releaseGdiFontMapReferences call is not required 
>> when GdiFontMapInfo object isn't initialized - The particular 
>> invocation has been removed.
>>
>> 2. With regard to deleting references held within GdiFontMapInfo 
>> struture
>>             JNI creates local references only for objects that are 
>> created at the Java side - jobject, jstring and jclass
>>             The other FMI variables - method IDs are not references 
>> to objects. Hence invoking deleteLocalRef.. is not valid (may not 
>> compile too).
>>
>> 3. The Jtreg shell script - LoadFontsJNICheck.sh was initially 
>> written to run only on MacOS.
>>             Since it addresses the bug at hand (which is on windows), 
>> I tested the script 's execution on Win and Linux.
>>                   The Jtreg script passes on linux and fails on 
>> windows with warnings in java.lang codebase.
>>                   As I understand, there is a JBS bug filed already 
>> pertaining to JNI warnings in java.lang package.
>>                   Hence enabling the script on windows, doesn't 
>> introduce new bug.
>>             The only OS that the script doesn't run - Solaris.
>>                   Presently, we don't have a Solaris environment at 
>> our facility to test the script.
>>                   Hence the original functionality on Solaris is 
>> retained.
>>
>> Kindly take a look at the changes at your convenience & provide the 
>> suggestions.
>>
>> Thank you for your review
>> Have a good day
>>
>> Prahalad N.
>>
>> ----------------------------------------------------------------------
>> ------
>> From: Prasanta Sadhukhan
>> Sent: Monday, June 27, 2016 11:51 AM
>> To: Prahalad Kumar Narayanan; 2d-dev at openjdk.java.net
>> Cc: Philip Race; Jayathirth D V; Praveen Srivastava
>> Subject: Re: [9] Review Request- JDK-8152971- JNI Warning with
>> -Xcheck:jni
>>
>> I guess this method(s) should take "fontStrLC" instead of "fontStr"
>> 650             (*env)->CallObjectMethod(env, fontToFileMap, 
>> fmi->putMID,
>>     651                                      fontStr, fileStr);
>>
>>     692         (*env)->CallObjectMethod(env, fontToFileMap, 
>> fmi->putMID,
>>     693                                  fontStr, fileStr);
>>
>> 762             (*env)->CallObjectMethod(env, fontToFileMap, 
>> fmi->putMID,
>>     763                                      fontStr, fileStr);
>>
>> 805         (*env)->CallObjectMethod(env, fontToFileMap, fmi->putMID,
>>     806                                  fontStr, fileStr);
>> It seems this line is not needed as we have not populated fmi 
>> structure yet.
>> 882         releaseGdiFontMapReferences(env, &fmi);
>>
>> Why aren't we deleting 
>> fmi->env,fmi.arrayListCtr,fmi.addMID,fmi.putMID  in 
>> releaseGdiFontMapReferences()?
>>
>> Also, it seems earlier the testscript was supposed to execute only on 
>> Mac but now you are extending the execution platform to windows and 
>> linux as well excluding only solaris.
>> Is there any particular need to restrict solaris too?
>>
>> Regards
>> Prasanta
>>
>>
>> On 6/24/2016 7:34 PM, Prahalad Kumar Narayanan wrote:
>> Hello Everyone on Java2D Forum
>> Good day to you.
>> A Quick follow-up on webrev to fix the following issue
>>          Bug ID     : JDK-8152971  / -Xcheck:jni - warning in native 
>> method
>>          Bug Link :
>> https://bugs.openjdk.java.net/browse/JDK-8152971?filter=-1
>> Updated webrev link:
>> http://cr.openjdk.java.net/~pnarayanan/8152971/webrev.01/
>> Description on Changes
>>          . The previous webrev contained changes to additional files 
>> which are not related to the current bug in concern.
>>          . Henceforth, the updated webrev limits the changes to only 
>> fontpath.c and a Jtreg test script to verify the change.
>> Regarding Build & Test
>>          . Though the changes pertain to windows specific code, 
>> internal build system was triggered to ensure safe build on all 
>> supported platforms.
>>          . In addition, no new Jtreg failures were found with the 
>> proposed changes.
>> Kindly review the changes at your convenience & provide your feedback.
>> Thank you for your time in review
>> Have a good day
>> Prahalad N.
>>
>> -----Original Message-----
>> From: Prahalad Kumar Narayanan
>> Sent: Wednesday, June 22, 2016 3:20 PM
>> To: 2d-dev at openjdk.java.net
>> Cc: Philip Race; Prasanta Sadhukhan; Jayathirth D V; Praveen
>> Srivastava
>> Subject: [9] Review Request- JDK-8152971- JNI Warning with -Xcheck:jni
>> Hello Everyone, on Java2D Group Good day to you.
>> Please find herewith, webrev changes to fix the bug-
>>          Bug ID     : JDK-8152971  / -Xcheck:jni - warning in native 
>> method
>>          Bug Link : 
>> https://bugs.openjdk.java.net/browse/JDK-8152971?filter=-1
>>          Webrev :
>> http://cr.openjdk.java.net/~pnarayanan/8152971/webrev.00/
>> Description on Bug:
>>         1.  Too many JNI warnings are reported in the native 
>> functions when test-code is run with VM Option:  -Xcheck:jni
>>         2.  The warnings can be classified into 2 categories
>>                     a.  JNI warnings that are thrown due to the 
>> missing exception checks after an Up call ( JNI function invoking 
>> Java method )
>>                     b.  JNI warnings that are thrown due to increased 
>> usage of Local Reference objects.
>> Description on the Fix:
>>         1.  File : FontPath.c
>>                       Added JNIEnv->ExceptionCheck() and 
>> ExceptionClear() where Up call is invoked.
>>                               The checks are added only to the valid 
>> Up calls as per JNI spec.
>>                       Added JNIEnv->DeleteLocalRef where the native 
>> functions created Java side objects but did not delete their local 
>> reference after usage.
>>                               A few of the native functions get 
>> invoked as Callbacks to Windows APIs following the font enumeration.
>>                               Therefore at each callback, the count 
>> of active local references would increase.
>>                                        Over time, the active local 
>> references would exceed the planned number of local references set by 
>> JVM.
>>         2.  File : awt_Component.cpp
>>                       Added JNIEnv->ExceptionCheck() and 
>> ExceptionClear() where an Up call displayed a JNI warning while 
>> running the Jtreg test script.
>>                       Information on Jtreg test script is given below.
>>         3.  File : LoadFontsJNICheck.sh
>>                       The shell script is already a part of JTreg 
>> test case & invokes LoadFontsJNICheck with -Xcheck:jni VM option
>>                       However, the script was configured to run only 
>> on Mac OS. Now, the script is modified to run on windows, linux and 
>> mac systems.
>>                                    This way, the code changes can be 
>> checked for proper execution with test-case.
>> Kindly review the changes at your convenience and share your feedback.
>> Thank you for your time in review
>> Have a good day
>> Prahalad N.
>




More information about the 2d-dev mailing list