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

Reviewed-by: dholmes, ihse, sspitsyn, dcubed

make/test/JtregNativeHotspot.gmk

@@ -861,7 +861,7 @@ BUILD_HOTSPOT_JTREG_EXECUTABLES_LIBS_exesigtest := -ljvm
 
 ifeq ($(call isTargetOs, windows), true)
   BUILD_HOTSPOT_JTREG_EXECUTABLES_CFLAGS_exeFPRegs := -MT
-  BUILD_HOTSPOT_JTREG_EXCLUDE += exesigtest.c libterminatedThread.c libTestJNI.c libCompleteExit.c libTestPsig.c exeGetCreatedJavaVMs.c
+  BUILD_HOTSPOT_JTREG_EXCLUDE += exesigtest.c libterminatedThread.c libTestJNI.c libCompleteExit.c libMonitorWithDeadObjectTest.c libTestPsig.c exeGetCreatedJavaVMs.c
   BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libatExit := jvm.lib
   BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libnativeStack := jvm.lib
   BUILD_HOTSPOT_JTREG_EXECUTABLES_LIBS_exedaemonDestroy := jvm.lib
@@ -1503,6 +1503,7 @@ else
   BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libterminatedThread += -lpthread
   BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libatExit += -ljvm
   BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libCompleteExit += -lpthread
+  BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libMonitorWithDeadObjectTest += -lpthread
   BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libnativeStack += -lpthread
   BUILD_HOTSPOT_JTREG_EXECUTABLES_LIBS_exeGetCreatedJavaVMs := -ljvm -lpthread
 

src/hotspot/share/prims/jvmtiEnvBase.cpp

@@ -2243,6 +2243,13 @@ JvmtiMonitorClosure::do_monitor(ObjectMonitor* mon) {
   }
   // Filter out on stack monitors collected during stack walk.
   oop obj = mon->object();
+
+  if (obj == nullptr) {
+    // This can happen if JNI code drops all references to the
+    // owning object.
+    return;
+  }
+
   bool found = false;
   for (int j = 0; j < _owned_monitors_list->length(); j++) {
     jobject jobj = ((jvmtiMonitorStackDepthInfo*)_owned_monitors_list->at(j))->monitor;

src/hotspot/share/runtime/synchronizer.cpp

@@ -1115,7 +1115,6 @@ void ObjectSynchronizer::owned_monitors_iterate_filtered(MonitorClosure* closure
     // ObjectMonitor cannot be async deflated.
     if (monitor->has_owner() && filter(monitor->owner_raw())) {
       assert(!monitor->is_being_async_deflated(), "Owned monitors should not be deflating");
-      assert(monitor->object_peek() != nullptr, "Owned monitors should not have a dead object");
 
       closure->do_monitor(monitor);
     }

src/hotspot/share/runtime/vmOperations.cpp

@@ -347,6 +347,12 @@ class ObjectMonitorsDump : public MonitorClosure, public ObjectMonitorsView {
       return;
     }
 
+    if (monitor->object_peek() == nullptr) {
+      // JNI code doesn't necessarily keep the monitor object
+      // alive. Filter out monitors with dead objects.
+      return;
+    }
+
     add(monitor);
   }
 

test/hotspot/jtreg/runtime/Monitor/MonitorWithDeadObjectTest.java

@@ -0,0 +1,99 @@
+/*
+ * Copyright (c) 2022, 2023, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ *
+ */
+
+/*
+ * @bug 8320515
+ * @summary This test checks that ObjectMonitors with dead objects don't
+ *          cause asserts, crashes, or failures when various sub-systems
+ *          in the JVM find them.
+ * @library /testlibrary /test/lib
+ * @modules jdk.management
+ */
+
+/*
+ * @requires os.family != "windows" & os.family != "aix"
+ * @test id=DetachThread
+ * @run main/othervm/native MonitorWithDeadObjectTest 0
+ */
+
+/*
+ * @requires os.family != "windows" & os.family != "aix"
+ * @test id=DumpThreadsBeforeDetach
+ * @run main/othervm/native MonitorWithDeadObjectTest 1
+ */
+
+/*
+ * @requires os.family != "windows" & os.family != "aix"
+ * @test id=DumpThreadsAfterDetach
+ * @run main/othervm/native MonitorWithDeadObjectTest 2
+ */
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.ThreadMXBean;
+
+public class MonitorWithDeadObjectTest {
+    public static native void createMonitorWithDeadObject();
+    public static native void createMonitorWithDeadObjectDumpThreadsBeforeDetach();
+
+    static {
+        System.loadLibrary("MonitorWithDeadObjectTest");
+    }
+
+    private static void dumpThreadsWithLockedMonitors() {
+        ThreadMXBean threadBean = ManagementFactory.getThreadMXBean();
+        threadBean.dumpAllThreads(true, false);
+    }
+
+    private static void testDetachThread() {
+        // Create an ObjectMonitor with a dead object from an attached thread.
+        // This used to provoke an assert in DetachCurrentThread.
+        createMonitorWithDeadObject();
+    }
+
+    private static void testDumpThreadsBeforeDetach() {
+        // Create an ObjectMonitor with a dead object from an attached thread
+        // and perform a thread dump before detaching the thread.
+        createMonitorWithDeadObjectDumpThreadsBeforeDetach();
+    }
+
+    private static void testDumpThreadsAfterDetach() {
+        createMonitorWithDeadObject();
+
+        // The thread dumping code used to not tolerate monitors with dead
+        // objects and the detach code used to not unlock these monitors, so
+        // test that we don't end up with a bug where these monitors are not
+        // unlocked and then passed to the thread dumping code.
+        dumpThreadsWithLockedMonitors();
+    }
+
+    public static void main(String[] args) throws Exception {
+        int test = Integer.parseInt(args[0]);
+        switch (test) {
+            case 0: testDetachThread(); break;
+            case 1: testDumpThreadsBeforeDetach(); break;
+            case 2: testDumpThreadsAfterDetach(); break;
+            default: throw new RuntimeException("Unknown test");
+        };
+    }
+}

test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c

@@ -0,0 +1,172 @@
+/*
+ * Copyright (c) 2022, 2023, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+#include <jni.h>
+#include <pthread.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+static JavaVM* jvm;
+static pthread_t attacher;
+
+#define die(x) do { printf("%s:%s\n",x , __func__); perror(x); exit(EXIT_FAILURE); } while (0)
+
+static void check_exception(JNIEnv* env, const char* msg) {
+  if ((*env)->ExceptionCheck(env)) {
+    fprintf(stderr, "Error: %s", msg);
+    exit(-1);
+  }
+}
+
+#define check(env, what, msg)                      \
+  check_exception((env), (msg));                   \
+  do {                                             \
+    if ((what) == 0) {                             \
+      fprintf(stderr, #what "is null: %s", (msg)); \
+      exit(-2);                                    \
+    }                                              \
+  } while (0)
+
+static jobject create_object(JNIEnv* env) {
+  jclass clazz = (*env)->FindClass(env, "java/lang/Object");
+  check(env, clazz, "No class");
+
+  jmethodID constructor = (*env)->GetMethodID(env, clazz, "<init>", "()V");
+  check(env, constructor, "No constructor");
+
+  jobject obj = (*env)->NewObject(env, clazz, constructor);
+  check(env, constructor, "No object");
+
+  return obj;
+}
+
+static void system_gc(JNIEnv* env) {
+  jclass clazz = (*env)->FindClass(env, "java/lang/System");
+  check(env, clazz, "No class");
+
+  jmethodID method = (*env)->GetStaticMethodID(env, clazz, "gc", "()V");
+  check(env, method, "No method");
+
+  (*env)->CallStaticVoidMethod(env, clazz, method);
+  check_exception(env, "Calling System.gc()");
+}
+
+static void thread_dump_with_locked_monitors(JNIEnv* env) {
+  jclass ManagementFactoryClass = (*env)->FindClass(env, "java/lang/management/ManagementFactory");
+  check(env, ManagementFactoryClass, "No ManagementFactory class");
+
+  jmethodID getThreadMXBeanMethod = (*env)->GetStaticMethodID(env, ManagementFactoryClass, "getThreadMXBean", "()Ljava/lang/management/ThreadMXBean;");
+  check(env, getThreadMXBeanMethod, "No getThreadMXBean method");
+
+  jobject threadBean = (*env)->CallStaticObjectMethod(env, ManagementFactoryClass, getThreadMXBeanMethod);
+  check(env, threadBean, "Calling getThreadMXBean()");
+
+  jclass ThreadMXBeanClass = (*env)->FindClass(env, "java/lang/management/ThreadMXBean");
+  check(env, ThreadMXBeanClass, "No ThreadMXBean class");
+
+  jmethodID dumpAllThreadsMethod = (*env)->GetMethodID(env, ThreadMXBeanClass, "dumpAllThreads", "(ZZ)[Ljava/lang/management/ThreadInfo;");
+  check(env, dumpAllThreadsMethod, "No dumpAllThreads method");
+
+  // The 'lockedMonitors == true' is what causes the monitor with a dead object to be examined.
+  jobject array = (*env)->CallObjectMethod(env, threadBean, dumpAllThreadsMethod, JNI_TRUE /* lockedMonitors */, JNI_FALSE /* lockedSynchronizers*/);
+  check(env, array, "Calling dumpAllThreads(true, false)");
+}
+
+static void create_monitor_with_dead_object(JNIEnv* env) {
+  jobject obj = create_object(env);
+
+  if ((*env)->MonitorEnter(env, obj) != 0) die("MonitorEnter");
+
+  // Drop the last strong reference to the object associated with the monitor.
+  // The monitor only keeps a weak reference to the object.
+  (*env)->DeleteLocalRef(env, obj);
+
+  // Let the GC clear the weak reference to the object.
+  system_gc(env);
+}
+
+static void* create_monitor_with_dead_object_in_thread(void* arg) {
+  JNIEnv* env;
+  int res = (*jvm)->AttachCurrentThread(jvm, (void**)&env, NULL);
+  if (res != JNI_OK) die("AttachCurrentThread");
+
+  // Make the correct incantation to create a monitor with a dead object.
+  create_monitor_with_dead_object(env);
+
+  // DetachCurrentThread will try to unlock held monitors. This has been a
+  // source of at least two bugs:
+  // - When the object reference in the monitor was cleared, the monitor
+  //   iterator code would skip it, preventing it from being unlocked when
+  //   the owner thread detached, leaving it lingering in the system.
+  // - When the monitor iterator API was rewritten the code was changed to
+  //   assert that we didn't have "owned" monitors with dead objects. This
+  //   test provokes that situation and that asserts.
+  if ((*jvm)->DetachCurrentThread(jvm) != JNI_OK) die("DetachCurrentThread");
+
+  return NULL;
+}
+
+static void* create_monitor_with_dead_object_and_dump_threads_in_thread(void* arg) {
+  JNIEnv* env;
+  int res = (*jvm)->AttachCurrentThread(jvm, (void**)&env, NULL);
+  if (res != JNI_OK) die("AttachCurrentThread");
+
+  // Make the correct incantation to create a monitor with a dead object.
+  create_monitor_with_dead_object(env);
+
+  // Perform a thread dump that checks for all thread's monitors.
+  // That code didn't expect the monitor iterators to return monitors
+  // with dead objects and therefore asserted/crashed.
+  thread_dump_with_locked_monitors(env);
+
+  if ((*jvm)->DetachCurrentThread(jvm) != JNI_OK) die("DetachCurrentThread");
+
+  return NULL;
+}
+
+JNIEXPORT void JNICALL Java_MonitorWithDeadObjectTest_createMonitorWithDeadObject(JNIEnv* env, jclass jc) {
+  void* ret;
+
+  (*env)->GetJavaVM(env, &jvm);
+
+  if (pthread_create(&attacher, NULL, create_monitor_with_dead_object_in_thread, NULL) != 0) die("pthread_create");
+  if (pthread_join(attacher, &ret) != 0) die("pthread_join");
+}
+
+JNIEXPORT void JNICALL Java_MonitorWithDeadObjectTest_createMonitorWithDeadObjectDumpThreadsBeforeDetach(JNIEnv* env, jclass jc) {
+  void* ret;
+
+  (*env)->GetJavaVM(env, &jvm);
+
+  if (pthread_create(&attacher, NULL, create_monitor_with_dead_object_and_dump_threads_in_thread, NULL) != 0) die("pthread_create");
+  if (pthread_join(attacher, &ret) != 0) die("pthread_join");
+}
+
+#ifdef __cplusplus
+}
+#endif

test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java

@@ -24,8 +24,10 @@
 
 /**
  * @test
- * @bug 8185164
+ * @bug 8185164 8320515
- * @summary Checks that a contended monitor does not show up in the list of owned monitors
+ * @summary Checks that a contended monitor does not show up in the list of owned monitors.
+ *          8320515 piggy-backs on this test and injects an owned monitor with a dead object,
+            and checks that that monitor isn't exposed to GetOwnedMonitorInfo.
  * @requires vm.jvmti
  * @compile GetOwnedMonitorInfoTest.java
  * @run main/othervm/native -agentlib:GetOwnedMonitorInfoTest GetOwnedMonitorInfoTest
@@ -46,19 +48,42 @@ public class GetOwnedMonitorInfoTest {
         }
     }
 
+    private static native void jniMonitorEnter(Object obj);
     private static native int check();
     private static native boolean hasEventPosted();
 
-    public static void main(String[] args) throws Exception {
+    private static void jniMonitorEnterAndLetObjectDie() {
-        runTest(true);
+        // The monitor iterator used by GetOwnedMonitorInfo used to
-        runTest(false);
+        // assert when an owned monitor with a dead object was found.
+        // Inject this situation into this test that performs other
+        // GetOwnedMonitorInfo testing.
+        Object obj = new Object() {};
+        jniMonitorEnter(obj);
+        if (!Thread.holdsLock(obj)) {
+            throw new RuntimeException("The object is not locked");
+        }
+        obj = null;
+        System.gc();
     }
 
-    public static void runTest(boolean isVirtual) throws Exception {
+    public static void main(String[] args) throws Exception {
+        runTest(true, true);
+        runTest(true, false);
+        runTest(false, true);
+        runTest(false, false);
+    }
+
+    public static void runTest(boolean isVirtual, boolean jni) throws Exception {
         var threadFactory = isVirtual ? Thread.ofVirtual().factory() : Thread.ofPlatform().factory();
         final GetOwnedMonitorInfoTest lock = new GetOwnedMonitorInfoTest();
 
         Thread t1 = threadFactory.newThread(() -> {
+            Thread.currentThread().setName("Worker-Thread");
+
+            if (jni) {
+                jniMonitorEnterAndLetObjectDie();
+            }
+
             synchronized (lock) {
                 System.out.println("Thread in sync section: "
                                    + Thread.currentThread().getName());

test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/libGetOwnedMonitorInfoTest.c

@@ -22,6 +22,7 @@
  */
 
 #include <stdio.h>
+#include <stdlib.h>
 #include <string.h>
 #include "jvmti.h"
 #include "jni.h"
@@ -264,6 +265,14 @@ jint Agent_Initialize(JavaVM *jvm, char *options, void *reserved) {
     return JNI_OK;
 }
 
+JNIEXPORT void JNICALL
+Java_GetOwnedMonitorInfoTest_jniMonitorEnter(JNIEnv* env, jclass cls, jobject obj) {
+    if ((*env)->MonitorEnter(env, obj) != 0) {
+        fprintf(stderr, "MonitorEnter failed");
+        exit(-1);
+    }
+}
+
 JNIEXPORT jint JNICALL
 Java_GetOwnedMonitorInfoTest_check(JNIEnv *env, jclass cls) {
     return status;