On Thu, 20 Jan 2022 04:22:28 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-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. Change looks good to me. ------------- Marked as reviewed by pliden (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7134
