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

[serguei.spitsyn at oracle.com]

Hi Severin,

Thank you for taking care about this issue!

The fix looks pretty good.
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 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20160321/7677f233/attachment.html>