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

[Severin Gehwolf]

Hi Dmitry,

On Fri, 2016-04-15 at 21:59 +0300, Dmitry Samersoff wrote:
> Severin,
> 
> Looks good for me.

Thanks for the review!

> But I'm a little afraid of the fact that now we are holding
> eventHandler_lock while doing invoke*.

OK. FWIW, we need to hold this lock for proper lock ordering.
Otherwise com/sun/jdi/InvokeHangTest.java fails:

diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/invoker.c b/src/jdk.jdwp.agent/share/native/libjdwp/invoker.c
--- a/src/jdk.jdwp.agent/share/native/libjdwp/invoker.c
+++ b/src/jdk.jdwp.agent/share/native/libjdwp/invoker.c
@@ -670,7 +670,6 @@
         return JNI_FALSE;
     }
 
-    eventHandler_lock(); /* for proper lock order */
     debugMonitorEnter(invokerLock);
     env = getEnv();
 
@@ -706,7 +705,6 @@
 
     } END_WITH_LOCAL_REFS(env);
     debugMonitorExit(invokerLock);
-    eventHandler_unlock();


> So please hold on with backports until the fix bakes in jdk9 for some
> time.

Sounds good. Sorry for the trouble last time around.

Cheers,
Severin

> -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.
> > 
> > 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/we
> > brev.01/
> > 
> > Once reviewed, I'd need somebody to sponsor this patch.
> > 
> > Thanks,
> > Severin
> > 
>