[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9] Review Request- JDK-8152971- JNI Warning with -Xcheck:jni
From: Philip Race <philip.race () oracle ! com>
Date: 2016-06-29 17:17:59
Message-ID: 577402C7.6040505 () oracle ! com
[Download RAW message or body]
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 forEnumFontFamExProc
---
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.
>
[Attachment #3 (text/html)]
<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<pre><span class="new"><a class="moz-txt-link-freetext" \
href="https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/design.html#exce \
ption_handling">https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/design.html#exception_handling</a>
lists DeleteLocalRef as safe to call with a pending exception.
So here ...
</span></pre>
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<pre>74 fullnameLC = (*env)->CallObjectMethod(env, fullname,
175 fmi->toLowerCaseMID, \
fmi->locale); <span class="new"> 176 if ((*env)->ExceptionCheck(env)) \
{</span> <span class="new"> 177 (*env)->ExceptionClear(env);</span>
<span class="new"> 178 /* Delete the created references */</span>
<span class="new"> 179 DeleteLocalReference(env, fullname);</span>
<span class="new"> 180 return 1;</span>
<span class="new"> 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 </span><meta http-equiv="content-type" content="text/html; \
charset=UTF-8">EnumFontFamExProc <span class="new">
---
<a class="moz-txt-link-freetext" \
href="https://msdn.microsoft.com/en-us/library/windows/desktop/dd162618(v=vs.85).aspx" \
>https://msdn.microsoft.com/en-us/library/windows/desktop/dd162618(v=vs.85).aspx</a> \
>
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
</span></pre>
<br>
<br>
On 6/27/16, 10:24 PM, Prasanta Sadhukhan wrote:
<blockquote cite="mid:577209F0.9080309@oracle.com" type="cite">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.
<br>
<br>
Regards
<br>
Prasanta
<br>
On 6/27/2016 4:17 PM, Prahalad Kumar Narayanan wrote:
<br>
<blockquote type="cite">Hello Everyone
<br>
<br>
Good day to you.
<br>
<br>
Quick follow up to fix for the Bug
<br>
Bug ID : JDK-8152971
<br>
Bug Link :
<a class="moz-txt-link-freetext" \
href="https://bugs.openjdk.java.net/browse/JDK-8152971">https://bugs.openjdk.java.net/browse/JDK-8152971</a>
<br>
<br>
First, Thanks to Prasanta for his review & suggestions.
<br>
<br>
I 've incorporated some of the review suggestions & updated
the webrev.
<br>
Webrev Link :
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~pnarayanan/8152971/webrev.02/">http://cr.openjdk.java.net/~pnarayanan/8152971/webrev.02/</a>
<br>
<br>
Overview on changes from previous webrev:
<br>
<br>
1. Code Changes: As Prasanta mentioned,
<br>
The variable fontStrLC should be used in place of
fontStr - The correction has been taken up.
<br>
And releaseGdiFontMapReferences call is not required
when GdiFontMapInfo object isn't initialized - The particular
invocation has been removed.
<br>
<br>
2. With regard to deleting references held within GdiFontMapInfo
struture
<br>
JNI creates local references only for objects that are
created at the Java side - jobject, jstring and jclass
<br>
The other FMI variables - method IDs are not
references to objects. Hence invoking deleteLocalRef.. is not
valid (may not compile too).
<br>
<br>
3. The Jtreg shell script - LoadFontsJNICheck.sh was initially
written to run only on MacOS.
<br>
Since it addresses the bug at hand (which is on
windows), I tested the script 's execution on Win and Linux.
<br>
The Jtreg script passes on linux and fails on
windows with warnings in java.lang codebase.
<br>
As I understand, there is a JBS bug filed
already pertaining to JNI warnings in java.lang package.
<br>
Hence enabling the script on windows, doesn't
introduce new bug.
<br>
The only OS that the script doesn't run - Solaris.
<br>
Presently, we don't have a Solaris environment
at our facility to test the script.
<br>
Hence the original functionality on Solaris is
retained.
<br>
<br>
Kindly take a look at the changes at your convenience &
provide the suggestions.
<br>
<br>
Thank you for your review
<br>
Have a good day
<br>
<br>
Prahalad N.
<br>
<br>
<br>
<br>
From: Prasanta Sadhukhan
<br>
Sent: Monday, June 27, 2016 11:51 AM
<br>
To: Prahalad Kumar Narayanan; <a class="moz-txt-link-abbreviated" \
href="mailto:2d-dev@openjdk.java.net">2d-dev@openjdk.java.net</a> <br>
Cc: Philip Race; Jayathirth D V; Praveen Srivastava
<br>
Subject: Re: [9] Review Request- JDK-8152971- JNI Warning with
-Xcheck:jni
<br>
<br>
I guess this method(s) should take "fontStrLC" instead of
"fontStr"
<br>
650 (*env)->CallObjectMethod(env, fontToFileMap,
fmi->putMID,
<br>
651 \
fontStr, fileStr); <br>
<br>
692 (*env)->CallObjectMethod(env, fontToFileMap,
fmi->putMID,
<br>
693 \
fontStr, fileStr); <br>
<br>
762 (*env)->CallObjectMethod(env, fontToFileMap,
fmi->putMID,
<br>
763 \
fontStr, fileStr); <br>
<br>
805 (*env)->CallObjectMethod(env, fontToFileMap,
fmi->putMID,
<br>
806 \
fontStr, fileStr); <br>
It seems this line is not needed as we have not populated fmi
structure yet.
<br>
882 releaseGdiFontMapReferences(env, &fmi);
<br>
<br>
Why aren't we deleting
fmi->env,fmi.arrayListCtr,fmi.addMID,fmi.putMID in
releaseGdiFontMapReferences()?
<br>
<br>
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.
<br>
Is there any particular need to restrict solaris too?
<br>
<br>
Regards
<br>
Prasanta
<br>
<br>
<br>
On 6/24/2016 7:34 PM, Prahalad Kumar Narayanan wrote:
<br>
Hello Everyone on Java2D Forum
<br>
Good day to you.
<br>
A Quick follow-up on webrev to fix the following issue
<br>
Bug ID : JDK-8152971 / -Xcheck:jni - warning in
native method
<br>
Bug Link :
<a class="moz-txt-link-freetext" \
href="https://bugs.openjdk.java.net/browse/JDK-8152971?filter=-1">https://bugs.openjdk.java.net/browse/JDK-8152971?filter=-1</a>
<br>
Updated webrev link:
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~pnarayanan/8152971/webrev.01/">http://cr.openjdk.java.net/~pnarayanan/8152971/webrev.01/</a>
<br>
Description on Changes
<br>
. The previous webrev contained changes to additional
files which are not related to the current bug in concern.
<br>
. Henceforth, the updated webrev limits the changes to
only fontpath.c and a Jtreg test script to verify the change.
<br>
Regarding Build & Test
<br>
. Though the changes pertain to windows specific code,
internal build system was triggered to ensure safe build on all
supported platforms.
<br>
. In addition, no new Jtreg failures were found with the
proposed changes.
<br>
Kindly review the changes at your convenience & provide your
feedback.
<br>
Thank you for your time in review
<br>
Have a good day
<br>
Prahalad N.
<br>
-----Original Message-----
<br>
From: Prahalad Kumar Narayanan
<br>
Sent: Wednesday, June 22, 2016 3:20 PM
<br>
To: <a class="moz-txt-link-abbreviated" \
href="mailto:2d-dev@openjdk.java.net">2d-dev@openjdk.java.net</a> <br>
Cc: Philip Race; Prasanta Sadhukhan; Jayathirth D V; Praveen
Srivastava
<br>
Subject: [9] Review Request- JDK-8152971- JNI Warning with
-Xcheck:jni
<br>
Hello Everyone, on Java2D Group
<br>
Good day to you.
<br>
Please find herewith, webrev changes to fix the bug-
<br>
Bug ID : JDK-8152971 / -Xcheck:jni - warning in
native method
<br>
Bug Link :
<a class="moz-txt-link-freetext" \
href="https://bugs.openjdk.java.net/browse/JDK-8152971?filter=-1">https://bugs.openjdk.java.net/browse/JDK-8152971?filter=-1</a>
<br>
Webrev :
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~pnarayanan/8152971/webrev.00/">http://cr.openjdk.java.net/~pnarayanan/8152971/webrev.00/</a>
<br>
Description on Bug:
<br>
1. Too many JNI warnings are reported in the native
functions when test-code is run with VM Option: -Xcheck:jni
<br>
2. The warnings can be classified into 2 categories
<br>
a. JNI warnings that are thrown due to \
the missing exception checks after an Up call ( JNI function
invoking Java method )
<br>
b. JNI warnings that are thrown due to
increased usage of Local Reference objects.
<br>
Description on the Fix:
<br>
1. File : FontPath.c
<br>
Added JNIEnv->ExceptionCheck() and
ExceptionClear() where Up call is invoked.
<br>
The checks are added \
only to the valid Up calls as per JNI spec.
<br>
Added JNIEnv->DeleteLocalRef where \
the native functions created Java side objects but did not delete
their local reference after usage.
<br>
A few of the native \
functions get invoked as Callbacks to Windows APIs following the font
enumeration.
<br>
Therefore at each \
callback, the count of active local references would increase.
<br>
Over \
time, the active local references would exceed the planned number of local \
references set by JVM.
<br>
2. File : awt_Component.cpp
<br>
Added JNIEnv->ExceptionCheck() and
ExceptionClear() where an Up call displayed a JNI warning while
running the Jtreg test script.
<br>
Information on Jtreg test script is \
given below.
<br>
3. File : LoadFontsJNICheck.sh
<br>
The shell script is already a part of \
JTreg test case & invokes LoadFontsJNICheck with -Xcheck:jni VM
option
<br>
However, the script was configured to \
run only on Mac OS. Now, the script is modified to run on windows,
linux and mac systems.
<br>
This way, \
the code changes can be checked for proper execution with test-case.
<br>
Kindly review the changes at your convenience and share your
feedback.
<br>
Thank you for your time in review
<br>
Have a good day
<br>
Prahalad N.
<br>
</blockquote>
<br>
</blockquote>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic