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

[serguei.spitsyn at oracle.com]

On 4/19/16 19:06, serguei.spitsyn at oracle.com wrote:
> On 4/19/16 02:22, serguei.spitsyn at oracle.com wrote:
>> 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.
>
> Hi Severin,
>
> I postpone a push for this fix.
>
> There are two nsk.jdi test failures (they look like deadlocks):
>   nsk/jdi/ObjectReference/invokeMethod/invokemethod012 FAIL(TIMEOUT)
>   nsk/jdi/Scenarios/invokeMethod/popframes001 FAIL(TIMEOUT)
>
> and one jtreg sun/com/jdi failure (it looks like a deadlock too):
>   com/sun/jdi/InvokeHangTes.java
>
>
> I'll double check if these failures are regressions caused by your fix 
> or not.

I confirm, the failures above are new regressions introduced by the fix.
The tests fail consistently with the fix and do not fail without the fix.

Thanks,
Serguei

>
>
> Thanks,
> Serguei
>
>
>
>>
>> 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
>>>>>>>
>>>
>>
>