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

[Daniel D. Daugherty]

Dmitry,

This fix needs to be run through the entire JPDA test stack
before it is pushed. Don't know if we still have test definitions
to support that style of run anymore so it might be easier to
run it through the equivalent of a JDK9-hs nightly.

Dan

On 9/14/16 11:50 AM, Dmitry Samersoff wrote:
> Severin,
>
> The fix looks good for me.
>
> I'll sponsor the push, but please wait for Serguei.
>
> -Dmitry
>
>
> On 2016-09-09 19:27, 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/
>>
>