You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ph...@apache.org on 2017/07/06 20:56:14 UTC
svn commit: r1801108 - in
/subversion/trunk/subversion/bindings/javahl/native: Array.cpp CreateJ.cpp
Iterator.cpp OperationContext.cpp RemoteSession.cpp RevisionRangeList.cpp
Author: philip
Date: Thu Jul 6 20:56:14 2017
New Revision: 1801108
URL: http://svn.apache.org/viewvc?rev=1801108&view=rev
Log:
Add exception checks to some of the JavaHL native code to avoid JVM
warnings about JNI problems of the form:
WARNING in native method: JNI call made without checking exceptions when required to
* subversion/bindings/javahl/native/Array.cpp
(Array::init): Add exception check.
* subversion/bindings/javahl/native/CreateJ.cpp
(CreateJ::Mergeinfo): Add exception check.
* subversion/bindings/javahl/native/Iterator.cpp
(init_iterator, Iterator::next): Add exception check.
* subversion/bindings/javahl/native/OperationContext.cpp
(OperationContext::closeTunnel): Add exception check.
* subversion/bindings/javahl/native/RemoteSession.cpp
(build_string_array, long_iterable_to_revnum_array,
location_hash_to_map): Add exception check.
* subversion/bindings/javahl/native/RevisionRangeList.cpp
(RevisionRangeList::RevisionRangeList): Add exception check.
Modified:
subversion/trunk/subversion/bindings/javahl/native/Array.cpp
subversion/trunk/subversion/bindings/javahl/native/CreateJ.cpp
subversion/trunk/subversion/bindings/javahl/native/Iterator.cpp
subversion/trunk/subversion/bindings/javahl/native/OperationContext.cpp
subversion/trunk/subversion/bindings/javahl/native/RemoteSession.cpp
subversion/trunk/subversion/bindings/javahl/native/RevisionRangeList.cpp
Modified: subversion/trunk/subversion/bindings/javahl/native/Array.cpp
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl/native/Array.cpp?rev=1801108&r1=1801107&r2=1801108&view=diff
==============================================================================
--- subversion/trunk/subversion/bindings/javahl/native/Array.cpp (original)
+++ subversion/trunk/subversion/bindings/javahl/native/Array.cpp Thu Jul 6 20:56:14 2017
@@ -55,6 +55,8 @@ Array::init(jobjectArray jobjects)
if (jobjects != NULL)
{
JNIEnv *env = JNIUtil::getEnv();
+ if (JNIUtil::isExceptionThrown())
+ return;
jint arraySize = env->GetArrayLength(jobjects);
if (JNIUtil::isExceptionThrown())
return;
Modified: subversion/trunk/subversion/bindings/javahl/native/CreateJ.cpp
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl/native/CreateJ.cpp?rev=1801108&r1=1801107&r2=1801108&view=diff
==============================================================================
--- subversion/trunk/subversion/bindings/javahl/native/CreateJ.cpp (original)
+++ subversion/trunk/subversion/bindings/javahl/native/CreateJ.cpp Thu Jul 6 20:56:14 2017
@@ -1420,6 +1420,8 @@ jobject CreateJ::Mergeinfo(svn_mergeinfo
RevisionRangeList(static_cast<svn_rangelist_t*>(val)).toList();
env->CallVoidMethod(jmergeinfo, addRevisions, jpath, jranges);
+ if (JNIUtil::isJavaExceptionThrown())
+ return NULL;
env->DeleteLocalRef(jranges);
env->DeleteLocalRef(jpath);
Modified: subversion/trunk/subversion/bindings/javahl/native/Iterator.cpp
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl/native/Iterator.cpp?rev=1801108&r1=1801107&r2=1801108&view=diff
==============================================================================
--- subversion/trunk/subversion/bindings/javahl/native/Iterator.cpp (original)
+++ subversion/trunk/subversion/bindings/javahl/native/Iterator.cpp Thu Jul 6 20:56:14 2017
@@ -35,6 +35,8 @@ jobject init_iterator(jobject jiterable,
return NULL;
JNIEnv* env = JNIUtil::getEnv();
+ if (JNIUtil::isJavaExceptionThrown())
+ return NULL;
static jmethodID iterator_mid = 0;
if (0 == iterator_mid)
@@ -99,6 +101,8 @@ jobject Iterator::next() const
return NULL;
JNIEnv* env = JNIUtil::getEnv();
+ if (JNIUtil::isJavaExceptionThrown())
+ return NULL;
static jmethodID next_mid = 0;
if (0 == next_mid)
Modified: subversion/trunk/subversion/bindings/javahl/native/OperationContext.cpp
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl/native/OperationContext.cpp?rev=1801108&r1=1801107&r2=1801108&view=diff
==============================================================================
--- subversion/trunk/subversion/bindings/javahl/native/OperationContext.cpp (original)
+++ subversion/trunk/subversion/bindings/javahl/native/OperationContext.cpp Thu Jul 6 20:56:14 2017
@@ -643,6 +643,8 @@ OperationContext::closeTunnel(void *tunn
return;
JNIEnv *env = JNIUtil::getEnv();
+ if (JNIUtil::isJavaExceptionThrown())
+ return;
static jmethodID mid = 0;
if (0 == mid)
Modified: subversion/trunk/subversion/bindings/javahl/native/RemoteSession.cpp
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl/native/RemoteSession.cpp?rev=1801108&r1=1801107&r2=1801108&view=diff
==============================================================================
--- subversion/trunk/subversion/bindings/javahl/native/RemoteSession.cpp (original)
+++ subversion/trunk/subversion/bindings/javahl/native/RemoteSession.cpp Thu Jul 6 20:56:14 2017
@@ -646,6 +646,8 @@ build_string_array(const Iterator& iter,
{
const char* element;
jstring jitem = (jstring)iter.next();
+ if (JNIUtil::isExceptionThrown())
+ return NULL;
if (contains_relpaths)
{
Relpath item(jitem, pool);
@@ -959,7 +961,10 @@ long_iterable_to_revnum_array(jobject jl
Iterator iter(jlong_iterable);
while (iter.hasNext())
{
- const jlong entry = env->CallLongMethod(iter.next(), mid);
+ jobject next = iter.next();
+ if (JNIUtil::isExceptionThrown())
+ return NULL;
+ const jlong entry = env->CallLongMethod(next, mid);
if (JNIUtil::isExceptionThrown())
return NULL;
APR_ARRAY_PUSH(array, svn_revnum_t) = svn_revnum_t(entry);
@@ -971,6 +976,8 @@ jobject
location_hash_to_map(apr_hash_t* locations, apr_pool_t* scratch_pool)
{
JNIEnv* env = JNIUtil::getEnv();
+ if (JNIUtil::isExceptionThrown())
+ return NULL;
jclass long_cls = env->FindClass("java/lang/Long");
if (JNIUtil::isExceptionThrown())
Modified: subversion/trunk/subversion/bindings/javahl/native/RevisionRangeList.cpp
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/javahl/native/RevisionRangeList.cpp?rev=1801108&r1=1801107&r2=1801108&view=diff
==============================================================================
--- subversion/trunk/subversion/bindings/javahl/native/RevisionRangeList.cpp (original)
+++ subversion/trunk/subversion/bindings/javahl/native/RevisionRangeList.cpp Thu Jul 6 20:56:14 2017
@@ -42,7 +42,10 @@ RevisionRangeList::RevisionRangeList(job
m_rangelist = apr_array_make(pool.getPool(), 0, sizeof(svn_merge_range_t*));
while (iter.hasNext())
{
- svn_merge_range_t* range = RevisionRange(iter.next()).toMergeRange(pool);
+ jobject next = iter.next();
+ if (JNIUtil::isExceptionThrown())
+ return;
+ svn_merge_range_t* range = RevisionRange(next).toMergeRange(pool);
if (JNIUtil::isExceptionThrown())
return;
APR_ARRAY_PUSH(m_rangelist, svn_merge_range_t*) = range;
Re: svn commit: r1801108 - in
/subversion/trunk/subversion/bindings/javahl/native: Array.cpp CreateJ.cpp
Iterator.cpp OperationContext.cpp RemoteSession.cpp RevisionRangeList.cpp
Posted by Stefan Fuhrmann <st...@apache.org>.
On 06.07.2017 23:11, Philip Martin wrote:
> Philip Martin <ph...@codematters.co.uk> writes:
>
>> I've upgraded my JDK and it produced all these warnings.
Excellent to see these warnings being addressed!
> There is a second class of warnings of the form:
>
> WARNING: JNI local refs: 57, exceeds capacity: 56
They all seem to be off by one. But that could well be
an artefact of the first one too many getting reported.
> These are generated in two places: JNIUtil::putErrorsInTrace() and
> Java_org_apache_subversion_javahl_util_PropLib_unparseExternals().
>
> I'm not sure how best to fix these so I have a local hack in
> JNIUtil::wrappedCreateClientException to remove most of them:
>
> // Create a local frame for our references
> - env->PushLocalFrame(LOCAL_FRAME_SIZE);
> + env->PushLocalFrame(LOCAL_FRAME_SIZE + 100);
Maybe, it is the SVN_ERR__TRACING section in that method
that causes the overflow?
-- Stefan^2.
Re: svn commit: r1801108 - in /subversion/trunk/subversion/bindings/javahl/native: Array.cpp CreateJ.cpp Iterator.cpp OperationContext.cpp RemoteSession.cpp RevisionRangeList.cpp
Posted by Philip Martin <ph...@codematters.co.uk>.
Branko Čibej <br...@apache.org> writes:
> On 06.07.2017 23:11, Philip Martin wrote:
>> // Create a local frame for our references
>> - env->PushLocalFrame(LOCAL_FRAME_SIZE);
>> + env->PushLocalFrame(LOCAL_FRAME_SIZE + 100);
>
> Mmph ... it kinda hurts to see how limited the JNI API is in this
> respect. We could either increase the LOCAL_FRAME_SIZE variable, or
> create a new frame within each iteration of the loop.
We might be able to fix the unparseExternals case by passing an env into
the functor, then we would be creating fewer refs.
putErrorsInTrace is harder because it is recursive and stores a ref at
each level. Would pushing/poping frames invalidate those refs? Perhaps
converting the recursion to iteration might be the best solution.
--
Philip
Re: svn commit: r1801108 - in
/subversion/trunk/subversion/bindings/javahl/native: Array.cpp CreateJ.cpp
Iterator.cpp OperationContext.cpp RemoteSession.cpp RevisionRangeList.cpp
Posted by Branko Čibej <br...@apache.org>.
On 06.07.2017 23:11, Philip Martin wrote:
> Philip Martin <ph...@codematters.co.uk> writes:
>
>> I've upgraded my JDK and it produced all these warnings.
Adding the exception checks is correct. Rewriting everything to use the
jniwrapper abstraction would be even better, but that's a huge-ish heap
of work.
> There is a second class of warnings of the form:
>
> WARNING: JNI local refs: 57, exceeds capacity: 56
>
> These are generated in two places: JNIUtil::putErrorsInTrace() and
> Java_org_apache_subversion_javahl_util_PropLib_unparseExternals().
>
> I'm not sure how best to fix these so I have a local hack in
> JNIUtil::wrappedCreateClientException to remove most of them:
>
> // Create a local frame for our references
> - env->PushLocalFrame(LOCAL_FRAME_SIZE);
> + env->PushLocalFrame(LOCAL_FRAME_SIZE + 100);
Mmph ... it kinda hurts to see how limited the JNI API is in this
respect. We could either increase the LOCAL_FRAME_SIZE variable, or
create a new frame within each iteration of the loop.
-- Brane
Re: svn commit: r1801108 - in /subversion/trunk/subversion/bindings/javahl/native: Array.cpp CreateJ.cpp Iterator.cpp OperationContext.cpp RemoteSession.cpp RevisionRangeList.cpp
Posted by Philip Martin <ph...@codematters.co.uk>.
Philip Martin <ph...@codematters.co.uk> writes:
> I've upgraded my JDK and it produced all these warnings.
There is a second class of warnings of the form:
WARNING: JNI local refs: 57, exceeds capacity: 56
These are generated in two places: JNIUtil::putErrorsInTrace() and
Java_org_apache_subversion_javahl_util_PropLib_unparseExternals().
I'm not sure how best to fix these so I have a local hack in
JNIUtil::wrappedCreateClientException to remove most of them:
// Create a local frame for our references
- env->PushLocalFrame(LOCAL_FRAME_SIZE);
+ env->PushLocalFrame(LOCAL_FRAME_SIZE + 100);
--
Philip
Re: svn commit: r1801108 - in /subversion/trunk/subversion/bindings/javahl/native: Array.cpp CreateJ.cpp Iterator.cpp OperationContext.cpp RemoteSession.cpp RevisionRangeList.cpp
Posted by Philip Martin <ph...@codematters.co.uk>.
philip@apache.org writes:
> Author: philip
> Date: Thu Jul 6 20:56:14 2017
> New Revision: 1801108
>
> URL: http://svn.apache.org/viewvc?rev=1801108&view=rev
> Log:
> Add exception checks to some of the JavaHL native code to avoid JVM
> warnings about JNI problems of the form:
>
> WARNING in native method: JNI call made without checking exceptions when required to
>
I've upgraded my JDK and it produced all these warnings. I'm not sure
of the rules for JNI exception checking, take Iterator.cpp for example.
The call to JNIUtil::getInv() in Iterator::next() needs to be checked or
the warning is generated:
jobject Iterator::next() const
{
if (!m_jiterator)
return NULL;
JNIEnv* env = JNIUtil::getEnv();
if (JNIUtil::isJavaExceptionThrown())
return NULL;
static jmethodID next_mid = 0;
however similar code in Iterator::hasNext() doesn't need the check, at
least as far as the warning is concerned:
bool Iterator::hasNext() const
{
if (!m_jiterator)
return false;
JNIEnv* env = JNIUtil::getEnv();
static jmethodID hasNext_mid = 0;
Both functions are used by the testsuite but only Iterator::next()
causes the warning. Is Iterator::hasNext() correct to omit the check or
is this a limitation of the warning system? Should all calls to
JNIUtil::getEnv() be checked?
--
Philip
Re: svn commit: r1801108 - in /subversion/trunk/subversion/bindings/javahl/native: Array.cpp CreateJ.cpp Iterator.cpp OperationContext.cpp RemoteSession.cpp RevisionRangeList.cpp
Posted by Philip Martin <ph...@codematters.co.uk>.
"Bert Huijben" <be...@qqmail.nl> writes:
> Should we backport these to 1.9.x?
I don't know, possibly. See my other mail to dev for the questions I
have about these checks.
--
Philip
RE: svn commit: r1801108 - in /subversion/trunk/subversion/bindings/javahl/native: Array.cpp CreateJ.cpp Iterator.cpp OperationContext.cpp RemoteSession.cpp RevisionRangeList.cpp
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: philip@apache.org [mailto:philip@apache.org]
> Sent: donderdag 6 juli 2017 22:56
> To: commits@subversion.apache.org
> Subject: svn commit: r1801108 - in
> /subversion/trunk/subversion/bindings/javahl/native: Array.cpp CreateJ.cpp
> Iterator.cpp OperationContext.cpp RemoteSession.cpp
> RevisionRangeList.cpp
>
> Author: philip
> Date: Thu Jul 6 20:56:14 2017
> New Revision: 1801108
>
> URL: http://svn.apache.org/viewvc?rev=1801108&view=rev
> Log:
> Add exception checks to some of the JavaHL native code to avoid JVM
> warnings about JNI problems of the form:
>
> WARNING in native method: JNI call made without checking exceptions
> when required to
>
> * subversion/bindings/javahl/native/Array.cpp
> (Array::init): Add exception check.
>
> * subversion/bindings/javahl/native/CreateJ.cpp
> (CreateJ::Mergeinfo): Add exception check.
>
> * subversion/bindings/javahl/native/Iterator.cpp
> (init_iterator, Iterator::next): Add exception check.
>
> * subversion/bindings/javahl/native/OperationContext.cpp
> (OperationContext::closeTunnel): Add exception check.
>
> * subversion/bindings/javahl/native/RemoteSession.cpp
> (build_string_array, long_iterable_to_revnum_array,
> location_hash_to_map): Add exception check.
>
> * subversion/bindings/javahl/native/RevisionRangeList.cpp
> (RevisionRangeList::RevisionRangeList): Add exception check.
Should we backport these to 1.9.x?
Bert
RE: svn commit: r1801108 - in /subversion/trunk/subversion/bindings/javahl/native: Array.cpp CreateJ.cpp Iterator.cpp OperationContext.cpp RemoteSession.cpp RevisionRangeList.cpp
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: philip@apache.org [mailto:philip@apache.org]
> Sent: donderdag 6 juli 2017 22:56
> To: commits@subversion.apache.org
> Subject: svn commit: r1801108 - in
> /subversion/trunk/subversion/bindings/javahl/native: Array.cpp CreateJ.cpp
> Iterator.cpp OperationContext.cpp RemoteSession.cpp
> RevisionRangeList.cpp
>
> Author: philip
> Date: Thu Jul 6 20:56:14 2017
> New Revision: 1801108
>
> URL: http://svn.apache.org/viewvc?rev=1801108&view=rev
> Log:
> Add exception checks to some of the JavaHL native code to avoid JVM
> warnings about JNI problems of the form:
>
> WARNING in native method: JNI call made without checking exceptions
> when required to
>
> * subversion/bindings/javahl/native/Array.cpp
> (Array::init): Add exception check.
>
> * subversion/bindings/javahl/native/CreateJ.cpp
> (CreateJ::Mergeinfo): Add exception check.
>
> * subversion/bindings/javahl/native/Iterator.cpp
> (init_iterator, Iterator::next): Add exception check.
>
> * subversion/bindings/javahl/native/OperationContext.cpp
> (OperationContext::closeTunnel): Add exception check.
>
> * subversion/bindings/javahl/native/RemoteSession.cpp
> (build_string_array, long_iterable_to_revnum_array,
> location_hash_to_map): Add exception check.
>
> * subversion/bindings/javahl/native/RevisionRangeList.cpp
> (RevisionRangeList::RevisionRangeList): Add exception check.
Should we backport these to 1.9.x?
Bert