8269542: JDWP: EnableCollection support is no longer spec compliant after JDK-8255987

[plummercj]

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. 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). 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, 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. 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, 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.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issues

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

Reviewers

• David Holmes (@dholmes-ora - Reviewer)
• Per Liden (@pliden - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7134/head:pull/7134
$ git checkout pull/7134

Update a local copy of the PR:
$ git checkout pull/7134
$ git pull https://git.openjdk.java.net/jdk pull/7134/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7134

View PR using the GUI difftool:
$ git pr show -t 7134

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7134.diff

[bridgekeeper]

👋 Welcome back cjplummer! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[plummercj]

/issue add 8269542,8258071

[openjdk]

@plummercj This issue is referenced in the PR title - it will now be updated.

Adding additional issue to issue list: 8258071: Fix for JDK-8255987 can be subverted with ObjectReference.EnableCollection.

[openjdk]

@plummercj The following label will be automatically applied to this pull request:

• serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 02: Full - Incremental (e57f1caa)
• 01: Full - Incremental (6046f5c0)
• 00: Full (f1a1177f)

[plummercj]

@pliden can you have a look at this?

[dholmes-ora]

Hi Chris,

Your links to JDK-8269232 actually go to the wrong bug.

[dholmes-ora]

Hi Chris,

I've read through what you've written and the related bug reports to refresh my memory on this. I understand the approach you've taken and it makes sense as described. I think the implementation can be simplified a little by not using three fields - see below. But overall this seems okay.

Thanks,
David

[plummercj]

Hi Chris,

Your links to JDK-8269232 actually go to the wrong bug.

Ok. Fixed.

[dholmes-ora]

Hi Chris,

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

Thanks,
David

[dholmes-ora]

Fixes for NULL case look good.

Thanks,
David

[openjdk]

@plummercj This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 91 new commits pushed to the master branch:

• d156911: 8280459: Suspicious integer division in Hashtable.readHashtable
• 9bf6ffa: 8279124: VM does not handle SIGQUIT during initialization
• 30cd47d: 8280499: runtime/cds/appcds/TestDumpClassListSource.java fails on platforms without AppCDS custom class loaders support
• 54c9de2: 8275918: Remove unused local variables in java.base security code
• b9ae779: 8279675: CDS cannot handle non-existent JAR file in bootclassapth
• c1e4f3d: 8279397: Update --release 18 symbol information for JDK 18 build 32
• 2920ce5: 8278036: Saving rscratch1 is optional in MacroAssembler::verify_heapbase
• 6287ae3: 8277531: Print actual default stacksize on Windows thread logging
• ab2c8d3: 8280393: Promote use of HtmlTree factory methods
• 47b1c51: 8277120: Use Optional.isEmpty instead of !Optional.isPresent in java.net.http
• ... and 81 more: https://git.openjdk.java.net/jdk/compare/46fd683820bb7149c0605a0ba03f59e76de69c16...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[plummercj]

Fixes for NULL case look good.

Thanks, David

Thanks for the review and noticing these issues.

[jerboaa]

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

[plummercj]

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

Thanks for verifying!

[pliden]

Change looks good to me.

[plummercj]

Thanks for the reviews Per and David!

[plummercj]

/integrate

[openjdk]

Going to push as commit 841eae6.
Since your change was applied there have been 130 commits pushed to the master branch:

• 76fe03f: 8280166: Extend java/lang/instrument/GetObjectSizeIntrinsicsTest.java test cases
• cebaad1: 8280041: Retry loop issues in java.io.ClassCache
• cbe8395: 8280168: Add Objects.toIdentityString
• f4575e4: 8279946: (ch) java.nio.channels.FileChannel tryLock and write methods are missing @throws NonWritableChannelException
• 674a97b: 8280396: G1: Full gc mark stack draining should prefer to make work available to other threads
• fe77250: 8280414: Memory leak in DefaultProxySelector
• 496baad: 8280030: [REDO] Parallel: More precise boundary in ObjectStartArray::object_starts_in_range
• 4503d04: 8280375: G1: Tighten mem region limit in G1RebuildRemSetHeapRegionClosure
• 36fbec7: 8280241: (aio) AsynchronousSocketChannel init fails in IPv6 only Windows env
• 28796cb: 8163921: HttpURLConnection default Accept header is malformed according to HTTP/1.1 RFC
• ... and 120 more: https://git.openjdk.java.net/jdk/compare/46fd683820bb7149c0605a0ba03f59e76de69c16...master

Your commit was automatically rebased without conflicts.

[openjdk]

@plummercj Pushed as commit 841eae6.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.