On Wed, 25 Jun 2025 22:08:17 GMT, Daniel D. Daugherty <dcu...@openjdk.org>
wrote:
>> Kevin Walls has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - comment update
>> - comment update
>
> src/hotspot/share/services/threadService.cpp line 1477:
>
>> 1475: java_thread = java_lang_Thread::thread(thread_h());
>> 1476: if (java_thread == nullptr) {
>> 1477: return nullptr; // thread terminated
>
> This is not the right way to determine if you have a valid JavaThread
> when you have created a ThreadsListHandle. This code near the top
> of `ThreadSnapshotFactory::get_thread_snapshot` is not right:
>
>
> ThreadsListHandle tlh(THREAD);
> ResourceMark rm(THREAD);
> HandleMark hm(THREAD);
> Handle thread_h(THREAD, JNIHandles::resolve(jthread));
>
>
> The above code was added by:
> [JDK-8357650](https://bugs.openjdk.org/browse/JDK-8357650) ThreadSnapshot to
> take snapshot of thread for thread dumps
>
> Here's the example code from src/hotspot/share/runtime/threadSMR.hpp:
>
> // JNI jobject example:
> // jobject jthread = ...;
> // :
> // ThreadsListHandle tlh;
> // JavaThread* jt = nullptr;
> // bool is_alive = tlh.cv_internal_thread_to_JavaThread(jthread, &jt,
> nullptr);
> // if (is_alive) {
> // : // do stuff with 'jt'...
> // }
>
>
> So instead of this line:
>
> Handle thread_h(THREAD, JNIHandles::resolve(jthread));
>
> which does not guarantee you a valid JavaThread handle, you should
> use `tlh.cv_internal_thread_to_JavaThread` to get a `JavaThread*`.
Great catch Dan! I totally missed the TLH at the start of
`get_thread_snapshot`. I knew something was off here but couldn't quite put my
finger on it.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25958#discussion_r2167822932
