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

[serguei.spitsyn at oracle.com]

On 4/19/16 02:16, Severin Gehwolf wrote:
> Hi,
>
> On Tue, 2016-04-19 at 01:33 -0700, serguei.spitsyn at oracle.com wrote:
>> 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.
> Yes, that's right. saveGlobalRef() saves references by using the request pointer.
>
>> 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.
> OK.
>
>> Please, consider your fix reviewed.
> Thanks!
>
> The hg exported changeset is here:
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/JDK-8153711-jdk9-jdk.export.patch
>
> I'd need somebody to sponsor this for me.

I will sponsor the fix for you.

Thanks,
Serguei

>
> Cheers,
> Severin
>
>> 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
>>>>>
>>>>   
>>>   
>>   
>