8217618: JVM TI SuspendThread doesn't suspend the current thread before returning

Reviewed-by: dcubed, sspitsyn, dlong

src/hotspot/share/runtime/thread.cpp

@@ -2392,9 +2392,20 @@ void JavaThread::java_suspend() {
     }
   }
 
+  if (Thread::current() == this) {
+    // Safely self-suspend.
+    // If we don't do this explicitly it will implicitly happen
+    // before we transition back to Java, and on some other thread-state
+    // transition paths, but not as we exit a JVM TI SuspendThread call.
+    // As SuspendThread(current) must not return (until resumed) we must
+    // self-suspend here.
+    ThreadBlockInVM tbivm(this);
+    java_suspend_self();
+  } else {
     VM_ThreadSuspend vm_suspend;
     VMThread::execute(&vm_suspend);
   }
+}
 
 // Part II of external suspension.
 // A JavaThread self suspends when it detects a pending external suspend

test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS202/hs202t002/hs202t002.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2007, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2007, 2019, 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
@@ -43,13 +43,7 @@ extern "C" {
 static jint redefineNumber = 0;
 static jvmtiEnv * jvmti = NULL;
 
-typedef enum {
+static volatile bool thread_suspend_error = false;
-  suspend_error = -1,
-  not_suspended,
-  suspended
-} thread_suspend_status_t;
-
-static volatile thread_suspend_status_t thread_suspend_status = not_suspended;
 
 void JNICALL callbackMethodExit(jvmtiEnv *jvmti_env,
                                 JNIEnv* jni_env,
@@ -75,10 +69,10 @@ void JNICALL callbackMethodExit(jvmtiEnv *jvmti_env,
                 nsk_printf("Agent::SUSPENDING>> \n");
                 err=jvmti_env->SuspendThread(thread);
                 if (err == JVMTI_ERROR_NONE) {
-                    thread_suspend_status = suspended;
+                  // we don't get here until we are resumed
-                    nsk_printf("Agent:: Thread successfully suspended..\n");
+                    nsk_printf("Agent:: Thread successfully suspended and was resumed\n");
-                } else if (err == JVMTI_ERROR_THREAD_SUSPENDED) {
+                } else {
-                    thread_suspend_status = suspend_error;
+                    thread_suspend_error = true;
                     nsk_printf(" ## Error occured %s \n",TranslateError(err));
                 }
             }
@@ -175,7 +169,6 @@ Java_nsk_jvmti_scenarios_hotswap_HS202_hs202t002_hs202t002_resumeThread(JNIEnv *
 
     err = jvmti->ResumeThread(thread);
     if (err == JVMTI_ERROR_NONE) {
-        thread_suspend_status = not_suspended;
         retvalue = JNI_TRUE;
         nsk_printf(" Agent:: Thread Resumed.. \n");
     } else {
@@ -189,13 +182,21 @@ JNIEXPORT jboolean JNICALL
 Java_nsk_jvmti_scenarios_hotswap_HS202_hs202t002_hs202t002_isThreadSuspended(JNIEnv* jni,
                                                                              jclass clas,
                                                                              jthread thread) {
-    if (suspend_error == thread_suspend_status) {
+    if (thread_suspend_error) {
         jclass ex_class = jni->FindClass("java/lang/IllegalThreadStateException");
         jni->ThrowNew(ex_class, "Thread has failed to self suspend");
         return JNI_FALSE;
     }
 
-    return suspended == thread_suspend_status;
+    // There is an inherent race here if the suspend fails for some reason but
+    // thread_suspend_error is not yet set. But as long as we report the suspend
+    // state correctly there is no problem as the Java code will simply loop and call
+    // this again until we see thread_suspend_error is true.
+
+    jint state = 0;
+    // No errors possible here: thread is valid, and state is not NULL
+    jvmti->GetThreadState(thread, &state);
+    return (state & JVMTI_THREAD_STATE_SUSPENDED) != 0;
 }
 
-}
+} // extern C

test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/sampling/SP05/sp05t003/sp05t003.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2019, 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
@@ -65,7 +65,7 @@ static const char* threadsName[THREADS_COUNT] = {
 /* references to tested threads */
 static jthread threadsList[THREADS_COUNT];
 
-/* events conunts */
+/* events counts */
 static volatile int eventsStart = 0;
 static volatile int eventsEnd   = 0;
 
@@ -376,12 +376,13 @@ callbackThreadStart(jvmtiEnv* jvmti, JNIEnv* jni, jthread thread) {
             /* suspend thread */
             NSK_DISPLAY3("  suspend starting thread #%d (%s): %p\n",
                                 i, threadsName[i], (void*)thread);
+            /* must bump the count before we suspend */
+            eventsStart++;
 
             if (!NSK_JVMTI_VERIFY(jvmti->SuspendThread(thread))) {
                 nsk_jvmti_setFailStatus();
                 return;
             }
-            eventsStart++;
 
             break;
         }
@@ -406,6 +407,8 @@ callbackThreadEnd(jvmtiEnv* jvmti, JNIEnv* jni, jthread thread) {
     for (i = 0; i < THREADS_COUNT; i++) {
         if (jni->IsSameObject(threadsList[i], thread)) {
                 NSK_DISPLAY0("SUCCESS: expected THREAD_END event\n");
+            /* must bump the count before we suspend */
+            eventsEnd++;
 
             /* suspend thread */
             NSK_DISPLAY3("  suspend finishing thread #%d (%s): %p\n",
@@ -415,7 +418,6 @@ callbackThreadEnd(jvmtiEnv* jvmti, JNIEnv* jni, jthread thread) {
                 nsk_jvmti_setFailStatus();
                 return;
             }
-            eventsEnd++;
 
             break;
         }