RFR: 8345543: Test serviceability/jvmti/vthread/StopThreadTest/StopThreadTest.java failed: expected JVMTI_ERROR_OPAQUE_FRAME instead of: 0 [v2]

[David Holmes]

On Wed, 15 Jan 2025 19:29:07 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> The problem is that the state of the target `vthread` is changed to `BLOCKED` too early. It is done in a constructor of helper object `JavaThreadBlockedOnMonitorEnterState` which sets the state of `vthread` to `JavaThreadStatus::BLOCKED_ON_MONITOR_ENTER`. The `vthread` is getting suspended in `try_preempt()` while calling `JvmtiVTMSTransitionDisabler::start_VTMS_transition()`, so does not become unmounted as it is expected by the failing test. Also, it seems that the base class of the `JavaThreadBlockedOnMonitorEnterState` is supporting just HotSpot  `JavaThread`'s and so, it is changing the carrier thread state only.
>> 
>> The fix is to move the definition of the `JavaThreadBlockedOnMonitorEnterState` object after block where `try_preempt()` is called.
>> 
>> Testing:
>>  - Checked the failed test does not fail with the fix: `serviceability/jvmti/vthread/StopThreadTest/StopThreadTest.java`
>>  - Ran mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   review: nit: move curly bracket down

Sorry but I do not think this change is correct. While it may fix the issue with the virtual thread it now changes the behaviour for regular threads. For a very long time we have done the following in order:

JavaThreadBlockedOnMonitorEnterState jtbmes(current, this);`
current->set_current_pending_monitor(this);`
DTRACE_MONITOR_PROBE(contended__enter, this, object(), current);
if (JvmtiExport::should_post_monitor_contended_enter()) {
  JvmtiExport::post_monitor_contended_enter(current, this);

and it all works correctly for regular threads. If you delay setting the thread state that may cause problems elsewhere.

This also makes me wonder whether the rest of the above actions are necessarily correct in the virtual thread case.

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

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23125#pullrequestreview-2554551929