On Tue, 18 Jan 2022 20:25:41 GMT, Chris Plummer <cjplum...@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-8255987), 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-8255987). @pliden can you have a look at this? ------------- PR: https://git.openjdk.java.net/jdk/pull/7134
