On Sun, 2 Jul 2023 22:39:46 GMT, David Holmes <[email protected]> wrote:
>> The JVMTI function `SetEventNotificationMode` can set notification mode
>> globally (`event_thread == nullptr`) for all threads or for a specific
>> thread (`event_thread != nullptr`). To get a stable mount/unmount vision of
>> virtual threads a JvmtiVTMSTransitionDisabler helper object is created :
>> `JvmtiVTMSTransitionDisabler disabler(event_thread);`
>>
>> In a case if `event_thread == nullptr` the VTMS transitions are disabled for
>> all virtual thread,
>> otherwise they are disabled for a specific thread if it is virtual.
>> The call to `JvmtiEventController::set_user_enabled()` makes a call to
>> `recompute_enabled()` at the end of its work to do a required bookkeeping.
>> As part of this work, the `recompute_thread_enabled(state)` is called for
>> each thread from the `ThreadsListHandle`, not only for the given
>> `event_thread`:
>>
>> ThreadsListHandle tlh;
>> for (; state != nullptr; state = state->next()) {
>> any_env_thread_enabled |= recompute_thread_enabled(state);
>> }
>>
>> This can cause crashes as VTMS transitions for other virtual threads are
>> allowed.
>> Crashes are observed in this small function:
>>
>> bool is_interp_only_mode() {
>> return _thread == nullptr ? _saved_interp_only_mode != 0 :
>> _thread->is_interp_only_mode();
>> }
>>
>> In a case `_thread != nullptr` then the call needs to be executed:
>> `_thread->is_interp_only_mode()`.
>> But the filed `_thread` can be already changed to `nullptr` by a VTMS
>> transition.
>>
>> The fix is to always disable all transitions.
>> Thanks to Dan and Patricio for great analysis of this crash!
>>
>> Testing:
>> - In progress: mach5 tiers 1-6
>
> src/hotspot/share/prims/jvmtiEnv.cpp line 578:
>
>> 576: record_class_file_load_hook_enabled();
>> 577: }
>> 578: JvmtiVTMSTransitionDisabler disabler;
>
> An explanatory comment would have been good.
The fix has been already integrated.
I'll add a comment when there is a chance.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14728#discussion_r1253537846
