RFR: 8359870: JVM crashes in AccessInternal::PostRuntimeDispatch [v7]

[David Holmes]

On Mon, 30 Jun 2025 11:24:28 GMT, Kevin Walls <kevinw at openjdk.org> wrote:

>> ThreadDumper/ThreadSnapshot need to handle a failure to resolve the native VM JavaThread from a java.lang.Thread.  This is hard to reproduce but a thread that has since terminated can provoke a crash.  Recognise this and return a null ThreadSnapshot.
>
> Kevin Walls has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 13 additional commits since the last revision:
> 
>  - remove test definition changes
>  - TLH: use cv_internal_thread_to_JavaThread()
>  - Merge remote-tracking branch 'upstream/master' into 8359870_threadexited
>  - Test requires: permit linux debug testing
>  - comment update
>  - comment update
>  - newline
>  - Test fails on minimal VM: require jvmti feature
>  - Correct THROW macro
>  - ThreadDumper thread count
>  - ... and 3 more: https://git.openjdk.org/jdk/compare/393efb22...e2043438

Some minor nits/suggestions but generally looks good.

src/hotspot/share/services/threadService.cpp line 1445:

> 1443:   JavaThread* java_thread = nullptr;
> 1444:   oop thread_oop;
> 1445:   bool has_javathread = tlh.cv_internal_thread_to_JavaThread(jthread, &java_thread, &thread_oop);

Suggestion:

  bool has_javathread = tlh.cv_internal_thread_to_JavaThread(jthread, &java_thread, &thread_oop);
  assert((has_javathread && thread_oop != nullptr) || !has_javathread, "Missing Thread oop");

src/hotspot/share/services/threadService.cpp line 1447:

> 1445:   bool has_javathread = tlh.cv_internal_thread_to_JavaThread(jthread, &java_thread, &thread_oop);
> 1446:   Handle thread_h(THREAD, thread_oop);
> 1447:   bool is_virtual = java_lang_VirtualThread::is_instance(thread_h());

Suggestion:

  bool is_virtual = java_lang_VirtualThread::is_instance(thread_h());  // Deals with null

Just to make it clear we don't need an external null check here.

src/hotspot/share/services/threadService.cpp line 1450:

> 1448: 
> 1449:   if (!has_javathread && !is_virtual) {
> 1450:     return nullptr; // thread terminated

Suggestion:

    return nullptr; // thread terminated so not of interest

It took me a lot of backtracking in the Java code to find ThreadContainer which finally stated that only live threads are of interest, so an expanded comment here would help me. ( I was ready to suggest we should be creating a `ThreadSnapshot` that represents the terminated thread.)

src/java.base/share/classes/jdk/internal/vm/ThreadDumper.java line 313:

> 311:      * @throws UncheckedIOException if an I/O error occurs
> 312:      */
> 313:     private static boolean dumpThread(Thread thread, JsonWriter jsonWriter) {

Please document the return value in the javadoc comment.

src/java.base/share/classes/jdk/internal/vm/ThreadSnapshot.java line 56:

> 54:      * Take a snapshot of a Thread to get all information about the thread.
> 55:      * Return null if a ThreadSnapshot cannot be created, for example if the
> 56:      * thread has terminated.

Strictly speaking we can create a snapshot for a terminated thread, but our only client doesn't care about them so we don't.

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

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25958#pullrequestreview-2973312048
PR Review Comment: https://git.openjdk.org/jdk/pull/25958#discussion_r2176304440
PR Review Comment: https://git.openjdk.org/jdk/pull/25958#discussion_r2176303308
PR Review Comment: https://git.openjdk.org/jdk/pull/25958#discussion_r2176318110
PR Review Comment: https://git.openjdk.org/jdk/pull/25958#discussion_r2176319448
PR Review Comment: https://git.openjdk.org/jdk/pull/25958#discussion_r2176320553