8332738: Debug agent can deadlock on callbackLock when using StackFrame.PopFrames

Reviewed-by: sspitsyn, amenkov

src/jdk.jdwp.agent/share/native/libjdwp/eventHandler.c

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2024, 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
@@ -93,8 +93,8 @@ static jrawMonitorID callbackBlock;
  *   not blocking might mean that a return would continue execution of
  *   some java thread in the middle of VM_DEATH, this seems troubled.
  *
- *   WARNING: No not 'return' or 'goto' out of the BEGIN_CALLBACK/END_CALLBACK
- *            block, this will mess up the count.
+ *   WARNING: Do not 'return' or 'goto' out of the BEGIN_CALLBACK/END_CALLBACK
+ *            block. This will mess up the active_callbacks count.
  */
 
 #define BEGIN_CALLBACK()                                                \
@@ -1709,6 +1709,18 @@ eventHandler_unlock(void)
     debugMonitorExit(handlerLock);
 }
 
+void
+callback_lock(void)
+{
+    debugMonitorEnter(callbackLock);
+}
+
+void
+callback_unlock(void)
+{
+    debugMonitorExit(callbackLock);
+}
+
 /***** handler creation *****/
 
 HandlerNode *

src/jdk.jdwp.agent/share/native/libjdwp/eventHandler.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2024, 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
@@ -78,6 +78,9 @@ void eventHandler_waitForActiveCallbacks();
 void eventHandler_lock(void);
 void eventHandler_unlock(void);
 
+void callback_lock(void);
+void callback_unlock(void);
+
 jboolean eventHandler_synthesizeUnloadEvent(char *signature, JNIEnv *env);
 
 jclass getMethodClass(jvmtiEnv *jvmti_env, jmethodID method);

src/jdk.jdwp.agent/share/native/libjdwp/invoker.c

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2024, 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
@@ -719,7 +719,9 @@ invoker_completeInvokeRequest(jthread thread)
     exc = NULL;
     id  = 0;
 
-    eventHandler_lock(); /* for proper lock order */
+    callback_lock();     /* for proper lock order in threadControl getLocks() */
+    eventHandler_lock(); /* for proper lock order in threadControl getLocks() */
+    stepControl_lock();  /* for proper lock order in threadControl getLocks() */
     debugMonitorEnter(invokerLock);
 
     request = threadControl_getInvokeRequest(thread);
@@ -772,7 +774,7 @@ invoker_completeInvokeRequest(jthread thread)
      * We cannot delete saved exception or return value references
      * since otherwise a deleted handle would escape when writing
      * the response to the stream. Instead, we clean those refs up
-     * after writing the respone.
+     * after writing the response.
      */
     deleteGlobalArgumentRefs(env, request);
 
@@ -790,7 +792,9 @@ invoker_completeInvokeRequest(jthread thread)
      * Give up the lock before I/O operation
      */
     debugMonitorExit(invokerLock);
+    stepControl_unlock();
     eventHandler_unlock();
+    callback_unlock();
 
     if (!detached) {
         outStream_initReply(&out, id);

src/jdk.jdwp.agent/share/native/libjdwp/stepControl.c

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2024, 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
@@ -805,7 +805,8 @@ stepControl_beginStep(JNIEnv *env, jthread thread, jint size, jint depth,
     LOG_STEP(("stepControl_beginStep: thread=%p,size=%d,depth=%d",
               thread, size, depth));
 
-    eventHandler_lock(); /* for proper lock order */
+    callback_lock();     /* for proper lock order in threadControl getLocks() */
+    eventHandler_lock(); /* for proper lock order in threadControl getLocks() */
     stepControl_lock();
 
     step = threadControl_getStepRequest(thread);
@@ -852,6 +853,7 @@ stepControl_beginStep(JNIEnv *env, jthread thread, jint size, jint depth,
 
     stepControl_unlock();
     eventHandler_unlock();
+    callback_unlock();
 
     return error;
 }

src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c

@@ -642,26 +642,31 @@ getLocks(void)
      * thread) needs to be grabbed here. This allows thread control
      * code to safely suspend and resume the application threads
      * while ensuring they don't hold a critical lock.
+     *
+     * stepControl_beginStep() grabs the eventHandler lock and stepControl lock
+     * before eventually ending up here, so we need to maintain that order here.
+     * Similarly, invoker_completeInvokeRequest() grabs the eventHandler lock
+     * and invoker lock.
      */
-
+    callback_lock();
     eventHandler_lock();
+    stepControl_lock();
     invoker_lock();
     eventHelper_lock();
-    stepControl_lock();
-    commonRef_lock();
     debugMonitorEnter(threadLock);
-
+    commonRef_lock();
 }
 
 static void
 releaseLocks(void)
 {
-    debugMonitorExit(threadLock);
     commonRef_unlock();
-    stepControl_unlock();
+    debugMonitorExit(threadLock);
     eventHelper_unlock();
     invoker_unlock();
+    stepControl_unlock();
     eventHandler_unlock();
+    callback_unlock();
 }
 
 void