Committed in r947006. I tweaked the patch just a bit to ensure we catch any exceptions generated. Please confirm that it still functions as expected.
Thanks! -Hyrum On Thu, May 20, 2010 at 5:13 PM, Byeongcheol Lee <[email protected]>wrote: > Dear Subversion Developers: > > I attach a patch for a local reference leak in Outter.cpp, and the log > message follows here: > > [[[ > * subversion/bindings/javahl/native/Outputer.cpp > (write): add a call to "DeleteLocalRef" where a leaking local > reference is unreachable. > ]]] > > To reproduce this bug, run your javaHL regression test under our > dynamic bug detector, JInn > [http://userweb.cs.utexas.edu/~bclee/jinn<http://userweb.cs.utexas.edu/%7Ebclee/jinn> > ]: > > $env JAVA_TOOL_OPTIONS=-agentlib:jinn make check-javahl > ... > .Exception in thread "main" xtc.lang.blink.agent.JNIAssertionFailure: > The number of local references exceeds the current capacity 16 in > NewByteArray. > at > xtc.lang.blink.agent.JNIAssertionFailure.assertFail(JNIAssertionFailure.java:16) > at org.apache.subversion.javahl.SVNAdmin.dump(Native Method) > at org.apache.subversion.javahl.SVNAdmin.dump(SVNAdmin.java:128) > at org.apache.subversion.javahl.SVNTests.setUp(SVNTests.java:241) > at junit.framework.TestCase.runBare(TestCase.java:128) > ... > at org.apache.subversion.javahl.RunTests.main(RunTests.java:116) > ... > > The local reference leak happens when a Java native method creates > more than 16 local references without deleting unreachable references > (DeleteLocalRef), adjusting capacity (EnsureLocalCapacity) or > allocating a local frame (PushLocalFrame). The Subversion r946780 > violates this JNI programming rule: > > "The JNI specification dictates that the virtual machine automatically > ensures that each native method can create at least 16 local > references. Experience shows that this provides enough capacity for > the majority of native methods that do not contain complex > interactions with objects in the Java virtual machine. If, however, > there is a need to create additional local references, a native method > may issue an EnsureLocalCapacity call to make sure that space for a > sufficient number of local references is available. For example, a > slight variation of a previous example above reserves enough capacity > for all local references created during the loop execution if > sufficient memory is available:" > [http://java.sun.com/docs/books/jni/html/refs.html#27592] > > I attached gdb to the JVM, and examined every JNI call that allocates > a local reference, and I ended up the leaking allocation site at Line > 99 in subversion/bindings/javahl/native/Outputer.cpp: > > ... > 73 svn_error_t *Outputer::write(void *baton, const char *buffer, > apr_size_t *len) > 74 { > ... > 99 jbyteArray data = JNIUtil::makeJByteArray((const signed > char*)buffer, > 100 *len); > 101 if (JNIUtil::isJavaExceptionThrown()) > 102 return SVN_NO_ERROR; > 103 > 104 // write the data > 105 jint written = env->CallIntMethod(that->m_jthis, mid, data); > 106 if (JNIUtil::isJavaExceptionThrown()) > 107 return SVN_NO_ERROR; > 108 > 109 // return the number of bytes written > 110 *len = written; > 111 > 112 return SVN_NO_ERROR; > 113 } > > This "write" method does not delete the local reference in the > variable "data" at Line 99 when it returns. In the test run, this > "write" method was activated more than 16 times. The simplest fix is > adding "env->DeleteLocalRef(data)" after Line 105 where the "data" > variable is not used in the future. Even if Line 105 throws a Java > exception, it's fine because "DeleteLocalRef" is one of 20 > exception-oblivious function: > > "After an exception has been raised, the native code must first clear > the exception before making other JNI calls. When there is a pending > exception, the JNI functions that are safe to call are: > .... > DeleteLocalRef() > ...." [ > http://java.sun.com/javase/6/docs/technotes/guides/jni/spec/design.html#wp770 > ] > > For your information, here are calling contexts that allocate 16 > unreachable local references: > > 1 > Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101 > svn_stream_write at subversion/libsvn_subr/stream.c:153 > svn_stream_printf at subversion/libsvn_subr/stream.c:209 > svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1072 > svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480 > SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210 > Java_org_apache_subversion_javahl_SVNAdmin_dump at > org_apache_subversion_javahl_SVNAdmin.cpp:165 > > 2 > Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101 > svn_stream_write at subversion/libsvn_subr/stream.c:153 > svn_stream_printf at subversion/libsvn_subr/stream.c:209 > svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1075 > svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480 > SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210 > Java_org_apache_subversion_javahl_SVNAdmin_dump at > org_apache_subversion_javahl_SVNAdmin.cpp:165 > > 3 > Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101 > svn_stream_write at subversion/libsvn_subr/stream.c:153 > svn_stream_printf at subversion/libsvn_subr/stream.c:209 > write_revision_record at subversion/libsvn_repos/dump.c:985 > svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1101 > svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480 > SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210 > Java_org_apache_subversion_javahl_SVNAdmin_dump at > org_apache_subversion_javahl_SVNAdmin.cpp:165 > > 4 > Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101 > svn_stream_write at subversion/libsvn_subr/stream.c:153 > svn_stream_printf at subversion/libsvn_subr/stream.c:209 > write_revision_record at subversion/libsvn_repos/dump.c:988 > svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1101 > svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480 > SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210 > Java_org_apache_subversion_javahl_SVNAdmin_dump at > org_apache_subversion_javahl_SVNAdmin.cpp:165 > > 5 > Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101 > svn_stream_write at subversion/libsvn_subr/stream.c:153 > svn_stream_printf at subversion/libsvn_subr/stream.c:209 > write_revision_record at subversion/libsvn_repos/dump.c:995 > svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1101 > svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480 > SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210 > Java_org_apache_subversion_javahl_SVNAdmin_dump at > org_apache_subversion_javahl_SVNAdmin.cpp:165 > > 6 > Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101 > svn_stream_write at subversion/libsvn_subr/stream.c:153 > write_revision_record at subversion/libsvn_repos/dump.c:1001 > svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1101 > svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480 > SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210 > Java_org_apache_subversion_javahl_SVNAdmin_dump at > org_apache_subversion_javahl_SVNAdmin.cpp:165 > > 7 > Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101 > svn_stream_write at subversion/libsvn_subr/stream.c:153 > write_revision_record at subversion/libsvn_repos/dump.c:1004 > svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1101 > svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480 > SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210 > Java_org_apache_subversion_javahl_SVNAdmin_dump at > org_apache_subversion_javahl_SVNAdmin.cpp:165 > > 8 > Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101 > svn_stream_write at subversion/libsvn_subr/stream.c:153 > svn_stream_printf at subversion/libsvn_subr/stream.c:209 > repos_progress_handler at subversion/libsvn_repos/deprecated.c:454 > svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1159 > svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480 > SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210 > Java_org_apache_subversion_javahl_SVNAdmin_dump at > org_apache_subversion_javahl_SVNAdmin.cpp:165 > > 9 > Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101 > svn_stream_write at subversion/libsvn_subr/stream.c:153 > svn_stream_printf at subversion/libsvn_subr/stream.c:209 > write_revision_record at subversion/libsvn_repos/dump.c:985 > svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1119 > svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480 > SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210 > Java_org_apache_subversion_javahl_SVNAdmin_dump at > org_apache_subversion_javahl_SVNAdmin.cpp:165 > > 10 > Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101 > svn_stream_write at subversion/libsvn_subr/stream.c:153 > svn_stream_printf at subversion/libsvn_subr/stream.c:209 > write_revision_record at subversion/libsvn_repos/dump.c:988 > svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1119 > svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480 > SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210 > Java_org_apache_subversion_javahl_SVNAdmin_dump at > org_apache_subversion_javahl_SVNAdmin.cpp:165 > > 11 > Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101 > svn_stream_write at subversion/libsvn_subr/stream.c:153 > svn_stream_printf at subversion/libsvn_subr/stream.c:209 > write_revision_record at subversion/libsvn_repos/dump.c:995 > svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1119 > svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480 > SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210 > Java_org_apache_subversion_javahl_SVNAdmin_dump at > org_apache_subversion_javahl_SVNAdmin.cpp:165 > > 12 > Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101 > svn_stream_write at subversion/libsvn_subr/stream.c:153 > write_revision_record at subversion/libsvn_repos/dump.c:1001 > svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1119 > svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480 > SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210 > Java_org_apache_subversion_javahl_SVNAdmin_dump at > org_apache_subversion_javahl_SVNAdmin.cpp:165 > > 13 > Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101 > svn_stream_write at subversion/libsvn_subr/stream.c:153 > write_revision_record at subversion/libsvn_repos/dump.c:1004 > svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1119 > svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480 > SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210 > Java_org_apache_subversion_javahl_SVNAdmin_dump at > org_apache_subversion_javahl_SVNAdmin.cpp:165 > > 14 > Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101 > svn_stream_write at subversion/libsvn_subr/stream.c:153 > svn_stream_printf at subversion/libsvn_subr/stream.c:209 > dump_node at subversion/libsvn_repos/dump.c:330 > add_directory at subversion/libsvn_repos/dump.c:723 > path_driver_cb_func at subversion/libsvn_repos/replay.c:450 > svn_delta_path_driver at subversion/libsvn_delta/path_driver.c:254 > svn_repos_replay2 at subversion/libsvn_repos/replay.c:777 > svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1152 > svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480 > SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210 > Java_org_apache_subversion_javahl_SVNAdmin_dump at > org_apache_subversion_javahl_SVNAdmin.cpp:165 > > 15 > Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101 > svn_stream_write at subversion/libsvn_subr/stream.c:153 > svn_stream_printf at subversion/libsvn_subr/stream.c:209 > dump_node at subversion/libsvn_repos/dump.c:337 > add_directory at subversion/libsvn_repos/dump.c:723 > path_driver_cb_func at subversion/libsvn_repos/replay.c:450 > svn_delta_path_driver at subversion/libsvn_delta/path_driver.c:254 > svn_repos_replay2 at subversion/libsvn_repos/replay.c:777 > svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1152 > svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480 > SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210 > Java_org_apache_subversion_javahl_SVNAdmin_dump at > org_apache_subversion_javahl_SVNAdmin.cpp:165 > > 16 > Outputer::write at subversion/bindings/javahl/native/Outputer.cpp:101 > svn_stream_write at subversion/libsvn_subr/stream.c:153 > svn_stream_printf at subversion/libsvn_subr/stream.c:209 > dump_node at subversion/libsvn_repos/dump.c:417 > add_directory at subversion/libsvn_repos/dump.c:723 > path_driver_cb_func at subversion/libsvn_repos/replay.c:450 > svn_delta_path_driver at subversion/libsvn_delta/path_driver.c:254 > svn_repos_replay2 at subversion/libsvn_repos/replay.c:777 > svn_repos_dump_fs3 at subversion/libsvn_repos/dump.c:1152 > svn_repos_dump_fs2 at subversion/libsvn_repos/deprecated.c:480 > SVNAdmin::dump at subversion/bindings/javahl/native/SVNAdmin.cpp:210 > Java_org_apache_subversion_javahl_SVNAdmin_dump at > org_apache_subversion_javahl_SVNAdmin.cpp:165 > > Regards, > Byeong >
