RFR(s) 4858370: JDWP: Memory Leak: GlobalRefs never deleted when processing invokeMethod command

[Severin Gehwolf]

Hi Serguei,

On Mon, 2016-03-21 at 14:38 -0700, serguei.spitsyn at oracle.com wrote:
> Hi Severin,
> 
> Thank you for taking care about this issue!
> 
> The fix looks pretty good.

Thanks for the quick review!

> A couple of minor comments though.
> 
> src/jdk.jdwp.agent/share/native/libjdwp/invoker.c
>  222     jint argIndex;
>  223     jbyte argumentTag;
>  224     jvalue *argument;
>  225     void *cursor;
>  . . .
>  234     argIndex = 0;
>  235     argumentTag = firstArgumentTypeTag(request->methodSignature, &cursor);
>  236     argument = request->arguments;
> 
>   Could you make a small simplification and re-arrange the initialization of the locals:
>     void *cursor;
>     jint argIndex = 0;
>     jvalue *argument = request->arguments;
>     jbyte argumentTag = firstArgumentTypeTag(request->methodSignature, &cursor);
>  
>   I'd also suggest to get rid of the extra spaces at the lines: 227, 230, 237, 240, 252.   It will make the code style to be more consistent.   Could you, correct the comment at line 216 (invoker_invoke*() => invoker invoke*()):
>    216  * invoke request was carried out. See fillInvokeRequest() and invoker_invoke*()
> 
> 
> test/com/sun/jdi/OomDebugTest.java   Some lines are too long: 142, 162, 197, 212, 228, 264, 267, 289   It would be nice to make them shorter. I can push the fix once it has been reviewed. Thanks, Serguei

Updated webrev with the changes above is here:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-4858370/webrev.02/

For convenience, I've uploaded the HG exported patch too:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-4858370/JDK-4858370-jdk9-jdk.export.patch

Cheers,
Severin

> On 3/21/16 10:47, Severin Gehwolf wrote:
> > Hi,
> > 
> > Could somebody please review this fix for bug 4858370?
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-4858370
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-4858370/webrev.01/
> > 
> > Testing done: jdk_jdi test group passes. Added regression test.
> > 
> > There is a memory leak in the JDWP implementation where method
> > parameters, method return values and the "this" object reference for
> > constructor invocations are kept in memory via a global reference and
> > get, thus, never GC'ed.
> > 
> > The proposed fix deletes global references again in
> > invoke_completeInvokeRequest(). The references got previously created
> > in fillInvokeRequest() and invoker_invoke*() implementations.
> > 
> > Thoughts?
> > 
> > Note that I'd need somebody to sponsor the push to JDK 9 tree for me
> > (once approved). Thanks.
> > 
> > Cheers,
> > Severin
> >