Alexandr Miloslavskiy <amiloslavskiy@apache.org> (amiloslavskiy)

r1882115 | amiloslavskiy | 2020-09-29 12:12:59 +0000 (Tue, 29 Sep 2020)

JavaHL: Fix incorrect cache in SVNBase::createCppBoundObject

The problem here is that 'SVNBase::createCppBoundObject' can work
with different classes (see argument), but it cached methodID of
'<init>' for the first class processed. When invoked with a
different class later, it will call wrong '<init>' method.

The error is seen when running JavaHL tests with JDK14.

Error message is:
FATAL ERROR in native method: Wrong object class or methodID passed to JNI call
  at <...>.javahl.util.SubstLib.translateOutputStream(Native Method)
  at <...>.javahl.SVNUtil.translateStream(SVNUtil.java:1046)
  at <...>.javahl.UtilTests.testTranslateStream(UtilTests.java:521)
  <...>

[in subversion/bindings/javahl]
* native/SVNBase.cpp
  (createCppBoundObject): Do not cache methodID.

Review by: hartmannathan

Approved by: hartmannathan

r1882518 | amiloslavskiy | 2020-10-15 10:12:52 +0000 (Thu, 15 Oct 2020)

JavaHL: Introduce tests showing JVM crashes with TunnelAgent

The crashes will be addressed in subsequent commits.

[in subversion/bindings/javahl]
* tests/org/apache/subversion/javahl/BasicTests.java
  Add helper class 'TestTunnelAgent' to emulate server replies in test
  environment.
  Add new tests which currently causes JVM to crash:
  * testCrash_RemoteSession_nativeDispose
  * testCrash_RequestChannel_nativeRead_AfterException
  * testCrash_RequestChannel_nativeRead_AfterSvnError

r1882519 | amiloslavskiy | 2020-10-15 10:13:54 +0000 (Thu, 15 Oct 2020)

JavaHL: Introduce new helper class to temporarily stash Java exceptions

This will be used in subsequent commits. Committed separately to
facilitate code review and cherry-picks.

[in subversion/bindings/javahl]
* native/JNIUtil.cpp
* native/JNIUtil.h
  New class 'StashException', please refer to code comments.

r1882520 | amiloslavskiy | 2020-10-15 10:14:30 +0000 (Thu, 15 Oct 2020)

JavaHL: Fix 'JNI call made with exception pending' error

It is incorrect to call Java method with 'CallObjectMethod' when there
is an unhandled Java exception already pending. The fix is to
temporarily move exception out of the way.

This error is easily seen when running JavaHL tests.

[in subversion/bindings/javahl]
* native/JNIUtil.cpp
  Use 'StashException' helper class to temporarily move exception out
  of the way in two functions which are designed to run when there is
  an exception pending.

r1882521 | amiloslavskiy | 2020-10-15 10:15:14 +0000 (Thu, 15 Oct 2020)

JavaHL: Fix 'JNI call made without checking exceptions when required to'

Java JNI documentation says:
  The JNI functions that invoke a Java method return the result of
  the Java method. The programmer must call ExceptionOccurred() to
  check for possible exceptions that occurred during the execution
  of the Java method.

These warnings are seen in JavaHL tests due to '-Xcheck:jni' option.

[in subversion/bindings/javahl]
* native/JNIUtil.cpp
  Properly check for exceptions after every 'CallObjectMethod()'. In
  case of exception, return NULL, which is valid, because the callers
  are prepared for it:
  * exception_to_cstring()
    JNIUtil::thrownExceptionToCString()
      ClientContext::resolve()
        Passes result to 'svn_error_create()'
          Documented to be able to deal with NULL.
  * JNIUtil::checkJavaException()
      Explicitly handles returned NULL.

r1882522 | amiloslavskiy | 2020-10-15 10:15:47 +0000 (Thu, 15 Oct 2020)

JavaHL: Fix crash in TunnelAgent.CloseTunnelCallback after GC

When jobject reference is kept across different JNI calls, a new global
reference must be requested with NewGlobalRef(). Otherwise, GC is free
to remove the object. Even if Java code keeps a reference to the object,
GC can still move the object around, invalidating the kept jobject,
which results in a native crash when trying to access it.

This crash is demonstrated by the following JavaHL test:
'testCrash_RemoteSession_nativeDispose'

[in subversion/bindings/javahl]
* native/OperationContext.cpp
  (callCloseTunnelCallback): Extract function to facilitate changes in
    further commits.
  (openTunnel): Add NewGlobalRef() for kept jobject.
  (callCloseTunnelCallback): Add a matching DeleteGlobalRef().
  (closeTunnel): Remove early return. This becomes necessary in next commit
    to make sure that every object is cleaned up regardless of errors in other
	objects. Removing early return in this commit reduces patch sizes. Extra
	JNIUtil calls are merely a very small performance change in an unlikely
	scenario.

r1882523 | amiloslavskiy | 2020-10-15 10:16:39 +0000 (Thu, 15 Oct 2020)

JavaHL: Fix crash when TunnelAgent continues reading after being closed

The problem here is that when 'RemoteSession' is destroyed, it also
does 'apr_pool_destroy()' which frees memory behind 'apr_file_t', which
is represented by 'TunnelChannel.nativeChannel' in Java.
'TunnelChannel' runs on a different thread and is unaware that
'apr_file_t' pointer is now invalid.

Fix this by informing 'TunnelChannel' before 'apr_file_t' is freed.

This crash is demonstrated by the following JavaHL tests:
* testCrash_RequestChannel_nativeRead_AfterException
* testCrash_RequestChannel_nativeRead_AfterSvnError

This commit alone does not fix all problems in these tests, see
subsequent commits as well.

[in subversion/bindings/javahl]
* native/OperationContext.cpp
  (jrequest,jresponse): Keep references to tunnels to inform them later
  (create_Channel): Keeping a reference requires NewGlobalRef()
  (close_TunnelChannel): New function to inform Java side.
  (openTunnel): Keep references to Java tunnel objects.
  (closeTunnel): Inform Java side when tunnel is closed.
* src/org/apache/subversion/javahl/util/RequestChannel.java
* src/org/apache/subversion/javahl/util/ResponseChannel.java
  Add 'synchronized' to avoid error described in 'syncClose()'
* src/org/apache/subversion/javahl/util/Tunnel.java
  A new function to clean up. I decided not to change old '.close()'
  because the new function can lead to a deadlock if used incorrectly.

r1882524 | amiloslavskiy | 2020-10-15 10:17:19 +0000 (Thu, 15 Oct 2020)

JavaHL: Make sure that TunnelAgent is cleaned up on pending exception

See the previous commit for explanation why cleanup is important.

Before this commit, if there was a pending Java exception (such as SVN
error), 'OperationContext::closeTunnel()' early-returned on
'isJavaExceptionThrown()'.

At the same time, calling Java methods in 'close_TunnelChannel()'
requires that there are no pending Java exceptions. Use
'StashException' to temporarily move it out of the way.

This crash is demonstrated by the following JavaHL tests:
* testCrash_RequestChannel_nativeRead_AfterException
* testCrash_RequestChannel_nativeRead_AfterSvnError

This commit alone does not fix all problems in these tests, see
previous and next commits as well.

[in subversion/bindings/javahl]
* native/OperationContext.cpp
  Use 'StashException' to move temporarily exception out of the way.

r1882525 | amiloslavskiy | 2020-10-15 10:18:28 +0000 (Thu, 15 Oct 2020)

JavaHL: Fix crash when TunnelAgent.openTunnel() throws exception

See the previous commit for explanation why cleanup is important.

The problem occurs because when 'OperationContext::openTunnel()'
returns an error, SVN considers it as not constructed and will not
clean it up.

This crash is demonstrated by the following JavaHL test:
* testCrash_RequestChannel_nativeRead_AfterException

[in subversion/bindings/javahl]
* native/OperationContext.cpp
  Clean up after exception in 'TunnelAgent.openTunnel()'

Found	Patch
r1896012	r1886035, r1885955, r1882525, r1882524, r1882523, r1882522, r1882521, r1882520, r1882519, r1882518, r1882126, r1882115, r1882113