[PING] RFR(s) 8153711: [REDO] JDWP: Memory Leak: GlobalRefs never deleted when processing invokeMethod command

[serguei.spitsyn at oracle.com]

Hi Severin,

The fix looks good.
Thank you for persistence in fixing the issue!

The only suggestion is to refactor the lines 800-815 into a method call.
Something like deletePoentiallySavedGlobalRefs, similar to 
deleteGlobalArgumetRefs.

Thanks,
Serguei



On 9/14/16 09:34, Severin Gehwolf wrote:
> Anyone?
>
> On Fri, 2016-09-09 at 18:27 +0200, Severin Gehwolf wrote:
>> Hi,
>>
>> Could I please get a review of the this 4th version of this fix:
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8153711
>> webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev.03/
>>
>> It fixes a memory leak problem in the debugger as shown by the new
>> regression test.
>>
>> A bit of history to this new patch. The first version[1] of this patch,
>> pushed as fix for JDK-4858370, caused regressions in
>> InterfaceMethodsTest, InvokeTest and OomDebugTest (part of the fix).
>> The cause was not holding the invoker lock when clearing the
>> references. A subsequent version[2] caused deadlocks, because we were
>> holding the invoker lock while invoking in invoker_doInvoke().
>>
>> Finally, a third version[3] caused NPE's and asserts on Solaris. The
>> reason for them seems to be clearing the request->clazz and request-
>>> instance members *after* sending back the reply to the client. My
>> hypothesis is that it maybe related to the sequence of monitor_exit-
>>> perform IO->monitor_enter->toss references. Perhaps there is a
>> schedule where the response has been sent back, the next invoke starts
>> for the same app thread and it is just far enough along so that the
>> tossing of the references becomes observable by the next request.
>> Unfortunately, I don't have proof for this.
>>
>> However, testing showed that tossing request->clazz and request-
>>> instance members before the IO operation prevents this assertion from
>> being triggered. Thus, I'm now clearing global references - the ones we
>> can clear before sending back the response to the client - in the same
>> block while still holding the invoker and event handler locks as the
>> rest of the operations being done in completeInvokeRequest. Once the
>> response has been sent to the client there are still two global
>> references to clear: The one for the return value and a possible
>> exception which might have occurred. Since they are required for
>> sending the response to the client we do this after it's been sent.
>>
>> I think not holding the invoker lock while invoking is still a problem,
>> but that's being tracked in JDK-8156498.
>>
>> Testing done:
>>
>> - com/sun/jdi test-set. No regressions.
>>    New OomDebugTest passes. Fails before.
>> - I haven't observed hangs or regressions in 1000 runs on
>>    com/sun/jdi/InvokeTest.java
>>    com/sun/jdi/InvokeHangTest.java
>>    com/sun/jdi/OomDebugTest.java on Linux x86_64 release/fastdebug
>> - I haven't seen asserts being triggered on Solaris x86_64
>>    fastdebug of 100 iterations of:
>>    com/sun/jdi/InvokeTest.java
>>    com/sun/jdi/InvokeHangTest.java
>>    com/sun/jdi/OomDebugTest.java
>>
>> I believe I need a sponsor who can push this fix through JPRT once
>> reviewed.
>>
>> Thoughts?
>>
>> Thanks,
>> Severin
>>
>> [1] http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/277d7584fa03
>> [2] http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev.01/
>> [3] http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev.02/