RFR: 8334085: Test crash: assert(thread->held_monitor_count() == 0) failed: Must be [v3]

[David Holmes]

On Wed, 24 Jul 2024 18:59:44 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> The test `serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java` is failing with the assert in the `thaw_internal()` function. The assert is not fully correct as it does not account for an unexpected scenario.
>> 
>> Thanks to Patricio for reproducing this failure and identifying the root cause:
>>> The problem is that we can unmount a virtual thread, then mount it again, thaw a few frames, execute code that acquires a JNI monitor, and then call thaw again without releasing that monitor. In this test this will happen if the vthread is unmounted in System.out.println("Thread doing JNI call: " ...) because of contention with the main thread doing System.out.println("Main waiting for event.").
>> The issue can be reproduced by adding Thread.yield() before jniMonitorEnterAndLetObjectDie(). 
>> 
>> The fix corrects the assert to account for the `thread->jni_monitor_count()`.
>> Question: Is the same scenario possible for non-JNI monitors as well?
>> Also, the fix includes the test tweak described above which makes this failure always reproducible.
>> 
>> Testing:
>>  - Ran the test `GetOwnedMonitorInfoTest.java` locally
>>  - Mach5 tiers 1-6 are passed
>
> Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   added a comment explaining why extra yield is needed

test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java line 56:

> 54: 
> 55:     private static void jniMonitorEnterAndLetObjectDie() {
> 56:         // The monitor iterator used by GetOwnedMonitorInfo to

The original was correct "used to assert"

test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java line 90:

> 88: 
> 89:                 // Extra unmount helps to reproduce 8334085.
> 90:                 // Two sub-sequential thaws are needed in that sceanrio.

s/sceanrio/scenario

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20294#discussion_r1690432483
PR Review Comment: https://git.openjdk.org/jdk/pull/20294#discussion_r1690433117