On Sun, 2 Jan 2022 04:06:05 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
> 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.
Marked as reviewed by amenkov (Reviewer).
src/jdk.jdwp.agent/share/native/libjdwp/invoker.c line 802:
> 800: */
> 801: if (mustReleaseReturnValue && returnValue.l != NULL) {
> 802: tossGlobalRef(env, &returnValue.l);
Please make indentation 4 spaces to be consistent with other code in the file.
The same for line 805
-------------
PR: https://git.openjdk.java.net/jdk/pull/6943
