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

[Severin Gehwolf]

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.

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