RFR: 8328611: Thread safety issue in com.sun.tools.jdi.ReferenceTypeImpl::classObject

[Chris Plummer]

On Tue, 28 May 2024 23:20:48 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

> Fixed "double-checked-locking" bug in `ReferenceImplType.classObject()`. Details in the first comment. Tested with the following:
> - com/sun/jdi
> - nsk/jdi
> - nsk/jdwp
> - nsk/jdb

The bug is the classic "double-checked-locking" flaw. In general "double-checked-locking" does not work, but in this case, based on how it is used and the java memory model, it can be made to work by making the field volatile. Although the issue could be fixed by making the classObject field volatile, I decided to get rid of the double-checked-locking instead. There is little benefit to using it here (it simply avoids fetching the info twice when there is a race, which is very rare), and leaving it out simplifies the code and reduces overhead for the usual case (when there is no race). Regarding the pre-existing comment:

            // Are classObjects unique for an Object, or
            // created each time? Is this spec'ed?

Yes, they are unique. No they are not created each time. Probably this lack of understanding is why double-checked-locking was used here. The ObjectReference spec is a bit lose on the wording, referring to an ObjectReference as "An object that currently exists in the target VM". However, the implementation is not. VirtualMachineImpl.objectMirror() is used to create all ObjectReference instances, and it only creates one ObjectReference per JDWP objectID. I tested this by making classObject() fetch the ObjectReference on every call and compare the result to the cached value, and they always match. Also, the JDWP spec calls out that each java Object instance has 1 unique objectID. The following is the JDWP spec description of the objectID type.

"Uniquely identifies an object in the target VM. A particular object will be identified by exactly one objectID in JDWP commands and replies throughout its lifetime (or until the objectID is explicitly disposed). An ObjectID is not reused to identify a different object unless it has been explicitly disposed, regardless of whether the referenced object has been garbage collected. An objectID of 0 represents a null object. Note that the existence of an object ID does not prevent the garbage collection of the object. Any attempt to access a a garbage collected object with its object ID will result in the INVALID_OBJECT error code. Garbage collection can be disabled with the DisableCollection command, but it is not usually necessary to do so."

While looking into this bug I also ran across the similar classLoader() API, and noticed that it did not use synchronized and explained why in a comment. That is where I got the motivation to remove synchronized from classObject(). Then I found a bunch more similar APIs. I cleaned up their comment to be consistent.

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

PR Comment: https://git.openjdk.org/jdk/pull/19439#issuecomment-2136257466