8341020: Error handler crashes when Metaspace is not fully initialized #21206

shipilev · 2024-09-26T11:44:44Z

When debugging CDS, I asked for os::print_location when Metaspace was not completely initialized. I suspect the same might happen if error handler fires very early in startup sequence.

os::print_location got to Metaspace::contains, which ended up in Metaspace::is_in_nonclass_metaspace, which crashed, since VirtualSpaceList was null. Other checks in Metaspace::contains seem to only check the address against the boundaries, and are not affected.

Additional testing:

The same debugging session did not crash the VM after the patch

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-8341020: Error handler crashes when Metaspace is not fully initialized (Bug - P4)

Reviewers

David Holmes (@dholmes-ora - Reviewer) Review applies to 7efa13b9
Kim Barrett (@kimbarrett - Reviewer)
Johan Sjölen (@jdksjolen - Reviewer)
Stefan Karlsson (@stefank - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21206

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-09-26T11:45:18Z

👋 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 · 2024-09-26T11:46:50Z

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

8341020: Error handler crashes when Metaspace is not fully initialized

Reviewed-by: kbarrett, jsjolen, stefank, dholmes

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

40a44e1: 8340851: Open some TextArea awt tests
b993227: 8342119: Shenandoah: Remove extra ShenandoahUpdateRefsClosure
83e9e48: 8325937: runtime/handshake/HandshakeDirectTest.java causes "monitor end should be strictly below the frame pointer" assertion failure on AArch64
236c71c: 8341376: Open some TextArea awt tests 4
979895d: 8342079: Shenandoah: Remove extra ShenandoahInitMarkRootsClosure
9a94884: 8341055: Open some TextArea awt tests 2
7e98f59: 8340987: Open some TextArea awt tests 1
d915ac2: 8339871: serviceability/sa/TestDebugInfoDecode.java should be driver
363327e: 8341452: Test runtime/cds/appcds/DumpRuntimeClassesTest.java from JDK-8324259 is failing

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 · 2024-09-26T11:47:32Z

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

hotspot-runtime

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 · 2024-09-26T11:51:06Z

Webrevs

01: Full - Incremental (3769ca28)
00: Full (7efa13b9)

dholmes-ora

LGTM!

Thanks

kimbarrett · 2024-09-30T06:20:37Z

src/hotspot/share/memory/metaspace.cpp

@@ -992,6 +992,7 @@ bool Metaspace::is_in_shared_metaspace(const void* ptr) {

 // Returns true if pointer points into one of the non-class-space metaspace regions.
 bool Metaspace::is_in_nonclass_metaspace(const void* ptr) {
-  return VirtualSpaceList::vslist_nonclass()->contains((MetaWord*)ptr);
+  VirtualSpaceList* list = VirtualSpaceList::vslist_nonclass();
+  return (list != nullptr) && list->contains((MetaWord*) ptr);


I'm not sure about this. This, and a number of other related functions nearby,
all seem like they should have Metaspace::initialized() as a precondition. And
the call to Metaspace::contains in os::print_location should be conditional on
Metaspace::initialized(). (There may be other "is initialized" checks that
should be made by (and offered to) os::print_location, but that's beyond the
scope of this change.)

FWIW, we had a similar discussion in #19435.

All of these methods are public. I think that it seems sane to assume that they are the boundary for checking whether Metaspace is initialized before doing something.

All right, I think checking for Metaspace::initialized() in os::print_location is more in line with other changes, notably JDK-8333290. This is Kim's suggestion, I think. So I did just that.

jdksjolen

Thanks, this looks good to me.

stefank

Looks good to me.

kimbarrett

Looks good.

shipilev · 2024-10-18T05:53:53Z

Thank you!

/integrate

openjdk · 2024-10-18T05:55:09Z

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

f50bd0d: 8341513: Remove the unused thread_type field from OSThread
7a16906: 8341134: Deprecate for removal the jrunscript tool
ffe6091: 8173970: jar tool should have a way to extract to a directory
2b03dbd: 8311530: Deprecate jdk.jsobject module for removal
f7e8f50: 8341292: Open some TextArea awt tests 3
fe83b7d: 8339341: SurfaceManager cacheMap retains strong references
12551ae: 8339975: Open some dialog awt tests 2
60af907: 8340494: Open some dialog awt tests 4
40a44e1: 8340851: Open some TextArea awt tests
b993227: 8342119: Shenandoah: Remove extra ShenandoahUpdateRefsClosure
... and 7 more: https://git.openjdk.org/jdk/compare/d6f8b465e47d40220bdba6bf7502de90ee9fa7f7...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-18T05:55:20Z

@shipilev Pushed as commit 7f4ed50.

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

Fix

7efa13b

openjdk bot added the rfr Pull request is ready for review label Sep 26, 2024

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Sep 26, 2024

dholmes-ora approved these changes Sep 29, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 29, 2024

kimbarrett suggested changes Sep 30, 2024

View reviewed changes

shipilev changed the title ~~8341020: Metaspace::contains crashes when Metaspace is not fully initialized~~ 8341020: Error handler crashes when Metaspace is not fully initialized Oct 17, 2024

shipilev added 2 commits October 17, 2024 16:26

Merge branch 'master' into JDK-8341020-metaspace-crash

053d72a

Just check in error handler

3769ca2

openjdk bot removed the ready Pull request is ready to be integrated label Oct 17, 2024

jdksjolen approved these changes Oct 17, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 17, 2024

stefank approved these changes Oct 17, 2024

View reviewed changes

kimbarrett approved these changes Oct 17, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Oct 18, 2024

openjdk bot closed this Oct 18, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 18, 2024

shipilev deleted the JDK-8341020-metaspace-crash branch January 8, 2025 12:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

8341020: Error handler crashes when Metaspace is not fully initialized #21206

8341020: Error handler crashes when Metaspace is not fully initialized #21206

Uh oh!

shipilev commented Sep 26, 2024 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented Sep 26, 2024

Uh oh!

openjdk bot commented Sep 26, 2024 •

edited

Loading

Uh oh!

openjdk bot commented Sep 26, 2024

Uh oh!

mlbridge bot commented Sep 26, 2024 •

edited

Loading

Uh oh!

dholmes-ora left a comment

Uh oh!

kimbarrett Sep 30, 2024

Uh oh!

stefank Sep 30, 2024

Uh oh!

jdksjolen Oct 3, 2024

Uh oh!

shipilev Oct 17, 2024

Uh oh!

jdksjolen left a comment

Uh oh!

stefank left a comment

Uh oh!

kimbarrett left a comment

Uh oh!

shipilev commented Oct 18, 2024

Uh oh!

openjdk bot commented Oct 18, 2024

Uh oh!

openjdk bot commented Oct 18, 2024

Uh oh!

Uh oh!

8341020: Error handler crashes when Metaspace is not fully initialized #21206

8341020: Error handler crashes when Metaspace is not fully initialized #21206

Uh oh!

Conversation

shipilev commented Sep 26, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Sep 26, 2024

Uh oh!

openjdk bot commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Sep 26, 2024

Uh oh!

mlbridge bot commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

kimbarrett Sep 30, 2024

Choose a reason for hiding this comment

Uh oh!

stefank Sep 30, 2024

Choose a reason for hiding this comment

Uh oh!

jdksjolen Oct 3, 2024

Choose a reason for hiding this comment

Uh oh!

shipilev Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

jdksjolen left a comment

Choose a reason for hiding this comment

Uh oh!

stefank left a comment

Choose a reason for hiding this comment

Uh oh!

kimbarrett left a comment

Choose a reason for hiding this comment

Uh oh!

shipilev commented Oct 18, 2024

Uh oh!

openjdk bot commented Oct 18, 2024

Uh oh!

openjdk bot commented Oct 18, 2024

Uh oh!

Uh oh!

shipilev commented Sep 26, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Sep 26, 2024 •

edited

Loading

mlbridge bot commented Sep 26, 2024 •

edited

Loading