RFR: 8269542: JDWP: EnableCollection support is no longer spec compliant after JDK-8255987 [v2]

[David Holmes]

On Wed, 19 Jan 2022 16:40:08 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

>> The JDWP spec mentions nothing about `DisableCollection` and `EnableCollection` tracking the depth or nesting of the commands. This means that `EnableCollection` should enable collection no matter how many `DisableCollection` commands were done before it. This is how the debug agent used to work before [JDK-8255987](https://bugs.openjdk.java.net/browse/JDK-8255987). Now the commands' nesting level is tracked, meaning that if there are two `DisableCollection` commands, you need two `EnableCollection` commands to re-enable collection. This is contrary to what the spec says. Only one `EnableCollection` command should be needed to undo any number of `DisableCollection` commands.
>> 
>> Note, JDWP differs from JDI here. The JDI spec explicitly says you need an enable for each disable in order to enable collection again. JDI tracks the disable count. Also, JDI only does the JDWP `DisableCollection` for the first disable, and only does the JDWP `EnableCollection` when the count goes back to 0. For this reason this JDWP bug cannot be reproduced by using JDI. You have to use JDWP directly. Unfortunately we have very limited direct testing of JDWP. JDWP testing mostly is done via JDI. What little JDWP testing we do have pretty much just verifies that reply packets are as expected. They don't verify VM or application behavior. For example, we have no JDWP test that tests that `DisableCollection` actually disables the collecting of the object. For this reason I have not added a test for this CR.
>> 
>> Another issue is that support for `DisableCollection/EnableCollection` nesting was intermixed with the support for having `VM.Suspend` disable collection on all objects (which was the main purpose of [JDK-8255987](https://bugs.openjdk.java.net/browse/JDK-8255987)). As a result, if you do a `VM.Suspend` and then do a `DisableCollection` on an `ObjectReference`, that object can now be collected, even though the spec says it should not be during a `VM.Suspend`. This issues is documented in [JDK-8258071](https://bugs.openjdk.java.net/browse/JDK-8258071), and is also fixed by this PR.
>> 
>> To fix both of these issues, `node->strongCount` should go back to being a boolean (`node->isStrong`) like it was before [JDK-8255987](https://bugs.openjdk.java.net/browse/JDK-8255987). Also, separate flags should be maintained to indicate if the reference is strong due to a `DisableCollection` and/or due to `VM.Suspend`. We need a flag for each, because it's possible that both can be true at the same time. When `node->isStrong` is true, if there is an `EnableCollection` it only gets set false if there is no current `VM.Suspend`. Likewise was if `VM.Resume` is done, it only gets set false if there is no outstanding `DisableCollection`.
>> 
>> There should be no need for maintaining a count anymore since we aren't suppose to for `DisableCollection/EnableCollection`, and there is no need to for `VM.Suspend/Resume`, since it only calls `commonRef_pinAll()` code when the `suspendAllCount` is changed to/from 0.
>> 
>> Please also note [JDK-8269232](https://bugs.openjdk.java.net/browse/JDK-8269232), which was also introduced by this nesting level counting, but is being fixed in more direct manner to keep the changes simple. The fix for CR replaces the changes for [JDK-8269232](https://bugs.openjdk.java.net/browse/JDK-8269232).
>
> Chris Plummer has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Get rid of isStrong flag and instead just compute it based on the value of isPinAll and isCommonPin. Add comments for some boolean args.

Hi Chris,

Update looks good. Just one query on something that doesn't quite look right.

Thanks,
David

src/jdk.jdwp.agent/share/native/libjdwp/commonRef.c line 181:

> 179:             node->strongCount = 1;
> 180:         }
> 181:         return strongRef;

Just to be clear about the removal of this line, at this point if `strongRef==NULL` then `node->ref==NULL` (otherwise we would have taken the EXIT_ERROR path) - right?

src/jdk.jdwp.agent/share/native/libjdwp/commonRef.c line 206:

> 204:             node->strongCount = 0;
> 205:         }
> 206:         return weakRef;

In this case I don't see that the removal of this return is correct if `weakRef==NULL` ?

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

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