RFR: 8297286: runtime/vthread tests crashing after JDK-8296324 [v9]

[Serguei Spitsyn]

On Thu, 30 Mar 2023 01:32:28 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1554:
>> 
>>> 1552:       }
>>> 1553:       // Correct jt->jvmti_thread_state() and jt->jvmti_vthread() if necessary.
>>> 1554:       // It was not maintained while notifyJvmti was disabled.
>> 
>> While trying to understand which exact situation we are trying to guard against with this code, I run the test without the sleeps and without this restore code and I got a crash when deleting a JvmtiThreadState (null dereference of _thread in the ~()). Probably the same crash you mentioned you had. But when debugging the crash I see that the problem is that the assumption that disabling the flag is done when no virtual threads are running is not guaranteed (see my comment there). So I think we are trying to address a case that shouldn't happen in the first place. Also not sure if applying this restore in all cases will be correct, since we might be somewhere at a transition. For example, a thread could have blocked right in the return from notifyJvmtiUnmount() in yieldContinuation(). It will looked like virtual because unmount() was not executed yet, and the jvmti_thread_state should be that of the platform thread because we never changed it when mounting. We should leave the stat
 e as is but in here we would change it to the virtual thread's jvmti state. The only case I think it makes sense to do this restore steps when enabling the flag is for those threads that are outside a transition with a mounted virtual thread, since we want to adjust the jvmti_thread_state so that it looks right on the next unmount.
>> But in any case this is also only needed when using the WhiteBox methods right? In the intended case (no WhiteBox method used), after we execute this operation to enable the events, we will create the JvmtiThreadStates later in JvmtiExport::get_jvmti_interface() and the correct jvmti_thread_state and jvmti_vthread will be already set for each JavaThread. In that case can we only execute this restore code when using the WhiteBox API?
>
> I've just pushed my update with refactoring of these corrections in `VM_SetNotifyJvmtiEventsMode` after discussion of this code with Leonid. I hope it resolved at least part of your concerns.
> This kind of problem exists only when we disable+enable notifyJvmti events multiple times for testing purposes.
> This code with corrections of jt->jvmti_thread_state() is not needed when we enable notifyJvmti events just once.
> Your are correct that the ability to disable notifyJvmti events is implemented with using WhiteBox API.
> My last update has a comment explaining this.
> 
>> In that case can we only execute this restore code when using the WhiteBox API?
> 
> This is a good suggestion which we already discussed privately with Leonid.
> I've got an idea how to implement this check.

> Also not sure if applying this restore in all cases will be correct, since we might be somewhere at a transition. For example, a thread could have blocked right in the return from notifyJvmtiUnmount() in yieldContinuation(). It will looked like virtual because unmount() was not executed yet, and the jvmti_thread_state should be that of the platform thread because we never changed it when mounting. We should leave the state as is but in here we would change it to the virtual thread's jvmti state. The only case I think it makes sense to do this restore steps when enabling the flag is for those threads that are outside a transition with a mounted virtual thread, since we want to adjust the jvmti_thread_state so that it looks right on the next unmount.

Agreed, thanks. In fact, I've already experimented with it.
As you noted, this correction is wrong for unmount transition. Also, we don't need to correct it for mount transition because it will be corrected by the `JvmtiVTMSTransitionDisabler::VTMS_mount_end()` as it makes a call `thread->rebind_to_jvmti_thread_state_of(vt)`.
The only thing that bothers me here is that we fight non-real problems as this code is needed only to support artificial disabling for testing purposes to be able to repeat enable again. :-)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13133#discussion_r1153513204