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@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@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@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@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@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.
