RFR: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object [v8]

[Daniel D. Daugherty]

On Mon, 27 Nov 2023 20:20:01 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

>> In the rewrites made for:
>> [JDK-8318757](https://bugs.openjdk.org/browse/JDK-8318757) `VM_ThreadDump asserts in interleaved ObjectMonitor::deflate_monitor calls`
>> 
>> I removed the filtering of *owned ObjectMonitors with dead objects*. The reasoning was that you should never have an owned ObjectMonitor with a dead object. I added an assert to check this assumption. It turns out that the assumption was wrong *if* you use JNI to call MonitorEnter and then remove all references to the locked object.
>> 
>> The provided tests provoke this assert form:
>> * the JNI thread detach code
>> * thread dumping with locked monitors, and
>> * the JVMTI GetOwnedMonitorInfo API.
>> 
>> While investigating this we've found that the thread detach code becomes more correct when this filter was removed. Previously, the locked monitors never got unlocked because the ObjectMonitor iterator never exposed these monitors to the JNI detach code that unlocks the thread's monitors. That bug caused an ObjectMonitor leak. So, for this case I'm leaving these ObjectMonitors unfiltered so that we don't reintroduce the leak.
>> 
>> The thread dumping case doesn't tolerate ObjectMonitor with dead objects, so I'm filtering those in the closure that collects ObjectMonitor. Side note: We have discussions about ways to completely rewrite this by letting each thread have thread-local information about JNI held locks. If we have this we could probably throw away the entire ObjectMonitorDump hashtable, and its walk of the `_in_use_list.`.
>> 
>> For GetOwnedMonitorInfo it is unclear if we should expose these weird ObjectMonitor. If we do, then the users can detect that a thread holds a lock with a dead object, and the code will return NULL as one of the "owned monitors" returned. I don't think that's a good idea, so I'm filtering out these ObjectMonitor for those calls.
>> 
>> Test: the written tests with and without the fix. Tier1-Tier3, so far.
>
> Stefan Karlsson has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix indentation

I've made a first pass over this PR. I don't think have anything
that's a "must do". I'll make another pass tomorrow after I have
a chance to think about this fix.

test/hotspot/jtreg/runtime/Monitor/MonitorWithDeadObjectTest.java line 2:

> 1: /*
> 2:  * Copyright (c) 2022, 2023, Oracle and/or its affiliates. All rights reserved.

nit: why include 2022 in the copyright header?

test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 2:

> 1: /*
> 2:  * Copyright (c) 2022, 2023, Oracle and/or its affiliates. All rights reserved.

nit: why include 2022 in the copyright header?

test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 28:

> 26: #include <pthread.h>
> 27: #include <stdio.h>
> 28: #include <unistd.h>

Should these be in sort order?

test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 110:

> 108: 
> 109:   // Let the GC clear the weak reference to the object.
> 110:   system_gc(env);

A single GC may not be enough...

test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 121:

> 119:   create_monitor_with_dead_object(env);
> 120: 
> 121:   // DetachCurrenThread will try to unlock held monitors. This has been a

nit typo: s/DetachCurrenThread/DetachCurrentThread/

test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 131:

> 129:   if ((*jvm)->DetachCurrentThread(jvm) != JNI_OK) die("DetachCurrentThread");
> 130: 
> 131:   return NULL;

Why is this function return type "void*" when it only returns NULL?

test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c line 149:

> 147:   if ((*jvm)->DetachCurrentThread(jvm) != JNI_OK) die("DetachCurrentThread");
> 148: 
> 149:   return NULL;

Why is this function return type "void*" when it only returns NULL?

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

PR Review: https://git.openjdk.org/jdk/pull/16783#pullrequestreview-1751551279
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1406858803
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1406861430
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1406862006
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1406864625
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1406864964
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1406866330
PR Review Comment: https://git.openjdk.org/jdk/pull/16783#discussion_r1406866618