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

[serguei.spitsyn at oracle.com]

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.


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
>>>>>>
>>
>