The test is failing because it is detecting an extra instance of `TestClass1`. 
The test (the debugger side) first tells the debuggee to create 10 instances of 
`TestClass1`. The debugger then uses JDI `ClassType.newInstance()` to create 
100 more instances. It then resumes the debuggee and uses 
`RefrenceType.instances()` to find out how many instances of `TestClass1` are 
reachable. Since the 100 created by `ClassType.newInstance()` should not have 
any references keeping them live, the answer should be 10, but sometimes it 
ends up being 11, so there is an extra instance.

I determined that this extra instance is always the last of the 100 that are 
created with `ClassType.newInstance()`. It uses the JDI/JDWP invoker interface. 
I found the following code in the debug agent invoker.c to be the problem:


    if (!detached) {
        outStream_initReply(&out, id);
        (void)outStream_writeValue(env, &out, tag, returnValue);
        (void)outStream_writeObjectTag(env, &out, exc);
        (void)outStream_writeObjectRef(env, &out, exc);
        outStream_sendReply(&out);
    }
…
    if (mustReleaseReturnValue && returnValue.l != NULL) {
        tossGlobalRef(env, &returnValue.l);
    }


The first block is responsible for sending the reply to the debugger for the 
JDI `ClassType.newInstance()` call. `returnValue` is a JNI global ref to the 
object that was just created, and `tossGlobalRef()` frees it after the reply 
packet has been sent. The problem is that once the reply packet has been 
received by the debugger (for the 100th `TestClass1` allocation), it resumes 
the debuggee and issues the `ReferenceType.instances()` call. This might be 
handled by the debug agent before it ever gets to the `tossGlobalRef()` call. 
So there will still be a reference to the 100th `TestClass1` object.

The fix is to call `tossGlobalRef()` after we are done with `returnValue`, but 
before sending out the packet. We are done with `returnValue` once the 
`outStream_writeValue()` call has been made. I decided to handle `exc` (the 
exception object) in the same manner. Although no tests were failing as a 
result of releasing it after sending the reply, I think you could write a test 
that triggered an exception and verified that the exception was not still 
considered live after doing the resume.

Regarding any concerns  you might have for moving `tossGlobalRef()` code from 
outside the `if (!detached)` to inside, if you follow the logic of this 
function you'll see that `mustReleaseReturnValue` can only be set true if 
`detached` is false. You'll also see that `exc` can only be non-null if 
`detached` is false. Thus these two `tossGlobalRef()` calls were only ever made 
when `detached` was false, and that remains true after my changes.

Regarding any concerns  you might have for making the `tossGlobalRef()` calls 
outside of the locks, the locking is a remnant from when the `exception` and 
`returnValue` fields were referenced directly out of the `InvokeRequest` 
struct, which could be accessed by other threads. That is no longer the case 
after changes were made for 
[JDK-8181419](https://bugs.openjdk.java.net/browse/JDK-8181419), which copied 
the fields into local variables. This code actually has been subject to a 
pretty long bug tail. See the last couple of long comments by me in 
[JDK-8176567](https://bugs.openjdk.java.net/browse/JDK-8176567) for details.

-------------

Commit messages:
 - Free invoker result and exception refs before sending the reply packet.

Changes: https://git.openjdk.java.net/jdk/pull/6943/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6943&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8176567
  Stats: 30 lines in 1 file changed: 14 ins; 15 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/6943.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/6943/head:pull/6943

PR: https://git.openjdk.java.net/jdk/pull/6943

Reply via email to