[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Integrated: 8269542: JDWP: EnableCollection support is no longer spec compliant after JDK-8255987
From:       Chris Plummer <cjplummer () openjdk ! java ! net>
Date:       2022-01-25 19:29:41
Message-ID: mJ7-Eu6A5HyD7IGQRwgYxZPL7dzUBO_ZYd6hJW2cELo=.ef64b286-0d46-4468-9836-907704b58723 () github ! com
[Download RAW message or body]

On Tue, 18 Jan 2022 20:25:41 GMT, Chris Plummer <cjplummer@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).

This pull request has now been integrated.

Changeset: 841eae6f
Author:    Chris Plummer <cjplummer@openjdk.org>
URL:       https://git.openjdk.java.net/jdk/commit/841eae6f527c00115e0455c4e04f042c28a014bb
                
Stats:     53 lines in 2 files changed: 23 ins; 5 del; 25 mod

8269542: JDWP: EnableCollection support is no longer spec compliant after JDK-8255987
8258071: Fix for JDK-8255987 can be subverted with ObjectReference.EnableCollection

Reviewed-by: dholmes, pliden

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

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


[prev in list] [next in list] [prev in thread] [next in thread]