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

[Prahalad Kumar Narayanan]

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.