[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