RR(S): 8020943: Memory leak when GCNotifier uses create_from_platform_dependent_str()

[Kevin Walls]

On 31/07/13 14:44, Daniel D. Daugherty wrote:
> On 7/31/13 3:45 AM, Kevin Walls wrote:
>> Hi,
>>
>> I'd like to get a review on this small change:
>>
>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8020943
>> https://jbs.oracle.com/bugs/browse/JDK-8020943
>>
>> webrev
>> http://cr.openjdk.java.net/~kevinw/8020943/webrev.00/
>
> src/share/vm/services/gcNotifier.cpp
>     No comments.
>
> The fix is fine in that you changed the code to call a function that
> doesn't leak a JNI handle, but the original JNI handle leak that you
> found in java_lang_String::create_from_platform_dependent_str()
> remains right?
>
> I think create_from_platform_dependent_str() still needs the call to
> "JNIHandles::destroy_local(js)". The next function down in the same
> file, java_lang_String::as_platform_dependent_str(), takes care to
> call "JNIHandles::destroy_local(js)" for its locally created JNI
> handle so there's good precedent for cleaning up such things.
>
> Dan
>


Hi Dan,

Yes.  It was also suggested to me that the JNIHandle would be cleared on 
a transition back to Java (which never happens in this case), so 
although I don't see many other callers, they may not all see a problem.

In another thread my initial thought was in 
create_from_platform_dependent_str itself, to clear the JNIHandle.  I 
just suggested the simple function-name-change here as all the jni work 
is unnecessary for what the gcNotifier needs.

Maybe I should make both changes, including:

$ hg diff  ../src/share/vm/classfile/javaClasses.cpp
diff --git a/src/share/vm/classfile/javaClasses.cpp
b/src/share/vm/classfile/javaClasses.cpp
--- a/src/share/vm/classfile/javaClasses.cpp
+++ b/src/share/vm/classfile/javaClasses.cpp
@@ -244,7 +244,10 @@
       ThreadToNativeFromVM ttn(thread);
       js = (_to_java_string_fn)(thread->jni_environment(), str);
     }
-  return Handle(THREAD, JNIHandles::resolve(js));
+  // XXX KJW we get a jstring back, but leak it...
+  Handle result = Handle(THREAD, JNIHandles::resolve(js));
+  JNIHandles::destroy_local(js);
+  return result;
   }


Thanks
Kevin





>
>>
>> It turns out there's a leak in the gc notifier: reproduce by 
>> attaching e.g. JConsole and watching, if there is frequent GC the 
>> number of apparently unowned String objects that can't get collect 
>> continually increases.
>>
>> In the notifier, the method it calls to create String objects 
>> involves a JNI call that leaves a Handle behind and doesn't get 
>> cleared.   There is a simpler method to call, there is no need for 
>> all that work, as we are dealing with a small set of simple strings 
>> in the JVM being converted, to describe the collection type, cause, ...
>>
>> Thanks
>> Kevin
>