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

[Severin Gehwolf]

On Thu, 20 Jan 2022 04:22:28 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:
> 
>   Fix some error case bugs that were introduced.

@plummercj I haven't reviewed, this but the Eclipse-based reproducer from JDK-8269232.still passes with this change. Thumbs up.

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

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