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

[serguei.spitsyn at oracle.com]

Hi Severin,

Please, find my comments below.


On 4/15/16 13:52, serguei.spitsyn at oracle.com wrote:
> On 4/15/16 11:59, Dmitry Samersoff wrote:
>> Severin,
>>
>> Looks good for me.
>>
>> But I'm a little afraid of the fact that now we are holding
>> eventHandler_lock while doing invoke*.
>
> It seems, I have this concern too.
> Please, let me take a look closer at this part if it is done in a 
> right way.

Ok, my question was not about the eventHandler_lock, but about the 
invokerLock.
Please, see below.

>
>
>>
>> So please hold on with backports until the fix bakes in jdk9 for some 
>> time.
> +1
>
> Thanks,
> Serguei
>
>>
>> -Dmitry
>>
>> On 2016-04-15 19:53, Severin Gehwolf wrote:
>>> Hi,
>>>
>>> Here is a patch which is a redo of the fix for JDK-4858370 which got
>>> backed out due to it causing test regressions. Specifically problems
>>> were reported for com/sun/jdi/InvokeTest.java
>>> and com/sun/jdi/InterfaceMethodsTest.java with the fix for JDK-4858370
>>> applied.
>>>
>>> Those test regressions were caused because:
>>> a.) The fix for JDK-4858370 deleted refs from the request object
>>>      while *not* holding the invoker lock.

>>> b.) Invokes done via invoker_doInvoke() save global references, but
>>>      don't hold the invoker lock either.

It seems, the invokerLock is to protect any uses of the 'request' 
pointer that points
to the field ThreadNode.currentInvoke, not to protect the 
saveGlobalRef() call itself.
So that, we have a hole in synchronization that you nicely discovered.

Here is a couple of more places where the 'request' pointer is not 
protected with
the invokerLock:
     invoker_enableInvokeRequests(), invoker_isPending(), 
invoker_isEnabled().

I've filed a new bug:
   https://bugs.openjdk.java.net/browse/JDK-8154529

BTW, these are pretty simple or rare cases that do not harm much.
The function invoker_isPending() is not used at all.

Please, consider your fix reviewed.

Thanks!
Serguei


>>>
>>> This new fix grabs relevant locks for both cases above.
>>>
>>> Testing done:
>>>   - com/sun/jdi test set. No regressions + added regression test.
>>>   - com/sun/jdi/InterfaceMethodsTest.java did not fail in 1300
>>>     invocations. Same for com/sun/jdi/InvokeTest.java.
>>>   - I haven't seen any crashes in new OomDebugTest.java either.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8153711
>>> webrev: 
>>> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev.01/
>>>
>>> Once reviewed, I'd need somebody to sponsor this patch.
>>>
>>> Thanks,
>>> Severin
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20160419/879a2153/attachment.html>