8350649: Class unloading accesses/resurrects dead Java mirror after JDK-8346567

[shipilev]

See bug for description of the bug. Shenandoah seems to be the only GC that runs into this problem so far.

Before JDK-8346567, we pulled class modifiers from the native Klass*, and so we bypassed this trouble. But now we take modifiers out of Java mirror, and this happens during unloading, which accesses/resurrects potentially dead mirror.

Additional testing:

• Linux x86_64 server fastdebug, original reproducer now passes
• Linux x86_64 server fastdebug, hotspot_gc_shenandoah
• Linux x86_64 server fastdebug, jdk_jfr
• Linux x86_64 server fastdebug, jdk_jfr with -XX:+UseShenandoahGC now passes

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8350649: Class unloading accesses/resurrects dead Java mirror after JDK-8346567 (Bug - P3)

Reviewers

• Coleen Phillimore (@coleenp - Reviewer)
• Erik Gahlin (@egahlin - Reviewer) Review applies to 8a77e589

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23775/head:pull/23775
$ git checkout pull/23775

Update a local copy of the PR:
$ git checkout pull/23775
$ git pull https://git.openjdk.org/jdk.git pull/23775/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23775

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23775.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back shade! 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.

[openjdk]

@shipilev 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:

8350649: Class unloading accesses/resurrects dead Java mirror after JDK-8346567

Reviewed-by: coleenp, egahlin

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 38 new commits pushed to the master branch:

• 0f82268: 8345598: Upgrade NSS binaries for interop tests
• ea2c923: 8323807: Async UL: Add a stalling mode to async UL
• e7d4b36: 8350667: Remove startThread_lock() and _startThread_lock on AIX
• 1e18fff: 8328473: StringTable and SymbolTable statistics delay time to safepoint
• a0dd565: 8350643: G1: Make loop iteration variable type correspond to limit in G1SurvRateGroup
• aac9cb4: 8349906: G1: Improve initial survivor rate for newly used young regions
• a431046: 8350518: org.openjdk.bench.java.util.TreeMapUpdate.compute fails with "java.lang.IllegalArgumentException: key out of range"
• a70eba8: 8330174: Protection zone for easier detection of accidental zero-nKlass use
• f529bf7: 8350483: AArch64: turn on signum intrinsics by default on Ampere CPUs
• 037e471: 8350666: cmp-baseline builds fail after JDK-8280682
• ... and 28 more: https://git.openjdk.org/jdk/compare/65f79c145b7b1b32ed064a37ad4d2b6aca935a4c...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.

[openjdk]

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

• hotspot

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.

[shipilev]

@coleenp, JDK-8346567 is yours, want to take a look?

[mlbridge]

Webrevs

• 04: Full - Incremental (0275ed1f)
• 03: Full - Incremental (8a77e589)
• 02: Full - Incremental (f49438c8)
• 01: Full - Incremental (c302a4a2)
• 00: Full (7269b441)

[coleenp]

No, we don't really want a duplicate of this field. We should just recompute them from the access flags since they don't change. If you're dubious about the value being the same make cached_modifier_flags be DEBUG_ONLY() and compare against compute_modifier_flags() result.

[coleenp]

It's only JFR that looks at modifier_flags after the mirror is dead, if I understand correctly.

[shipilev]

It's only JFR that looks at modifier_flags after the mirror is dead, if I understand correctly.

ciEnv looks at modifier_flags as well. I can introduce modifier_flags_slow to use on JFR path only. Let's see...

[coleenp]

ciEnv shouldn't be looking at modifier_flags from a dead mirror, though? If it's possible, then you can add modifier_flags to ciKlass to cache the value there. I wouldn't be unhappy with that.

[shipilev]

So I think JFR can just call compute_modifier_flags() directly, without relying on Java mirror. I added the blurb around the method to point out it is safer to do from unloading paths. See new version.

[coleenp]

Yes, this looks good. computer_modifier_flags() should get the same answer whenever it's called. Your comment looks good. JFR do_write_class during unloading is something that's been tricky in the past, but I hope there is nothing else that accesses the mirror when it's dead and the class should be unloaded.
Was there a reproducer that can be added with this change? I assume ZGC could have the same sort of problem.

[shipilev]

Was there a reproducer that can be added with this change? I assume ZGC could have the same sort of problem.

Existing jdk/jfr tests fail with Shenandoah reliably (see bug for example invocation), so I felt no need for a new regression test. JDK-8337978 also helps us to verify we touch the valid Java mirror. There is a larger reproducer in JDK-8350580, but I am still not 100% sure there are no other bugs it triggers. Once I am sure, I'll add some form of that as regression test.

[coleenp]

If existing tests fail reliably, then there's no need to add a new test. Looks like JDK-8337978 has proven its value!

[shipilev]

Thanks for reviews! Local testing passes fine, so I am integrating.

/integrate

[openjdk]

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

• 9477c70: 8024695: new File("").exists() returns false whereas it is the current working directory
• 3e46480: 8350770: [BACKOUT] Protection zone for easier detection of accidental zero-nKlass use
• bd112c4: 8350443: GHA: Split static-libs-bundles into a separate job
• 2731712: 8287749: Re-enable javadoc -serialwarn option
• 0f82268: 8345598: Upgrade NSS binaries for interop tests
• ea2c923: 8323807: Async UL: Add a stalling mode to async UL
• e7d4b36: 8350667: Remove startThread_lock() and _startThread_lock on AIX
• 1e18fff: 8328473: StringTable and SymbolTable statistics delay time to safepoint
• a0dd565: 8350643: G1: Make loop iteration variable type correspond to limit in G1SurvRateGroup
• aac9cb4: 8349906: G1: Improve initial survivor rate for newly used young regions
• ... and 32 more: https://git.openjdk.org/jdk/compare/65f79c145b7b1b32ed064a37ad4d2b6aca935a4c...master

Your commit was automatically rebased without conflicts.

[openjdk]

@shipilev Pushed as commit ec6624b.

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