8255987: JDI tests fail with com.sun.jdi.ObjectCollectedException

Reviewed-by: dholmes, cjplummer

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2020, 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
@@ -87,8 +87,9 @@ static RefNode *
 createNode(JNIEnv *env, jobject ref)
 {
     RefNode   *node;
-    jobject    weakRef;
+    jobject    strongOrWeakRef;
     jvmtiError error;
+    jboolean   pin = gdata->pinAllCount != 0;
 
     /* Could allocate RefNode's in blocks, not sure it would help much */
     node = (RefNode*)jvmtiAllocate((int)sizeof(RefNode));
@@ -96,29 +97,39 @@ createNode(JNIEnv *env, jobject ref)
         return NULL;
     }
 
-    /* Create weak reference to make sure we have a reference */
+    if (pin) {
-    weakRef = JNI_FUNC_PTR(env,NewWeakGlobalRef)(env, ref);
+        /* Create strong reference to make sure we have a reference */
-    // NewWeakGlobalRef can throw OOM, clear exception here.
+        strongOrWeakRef = JNI_FUNC_PTR(env,NewGlobalRef)(env, ref);
-    if ((*env)->ExceptionCheck(env)) {
+    } else {
-        (*env)->ExceptionClear(env);
+        /* Create weak reference to make sure we have a reference */
-        jvmtiDeallocate(node);
+        strongOrWeakRef = JNI_FUNC_PTR(env,NewWeakGlobalRef)(env, ref);
-        return NULL;
+
+        // NewWeakGlobalRef can throw OOM, clear exception here.
+        if ((*env)->ExceptionCheck(env)) {
+            (*env)->ExceptionClear(env);
+            jvmtiDeallocate(node);
+            return NULL;
+        }
     }
 
-    /* Set tag on weakRef */
+    /* Set tag on strongOrWeakRef */
     error = JVMTI_FUNC_PTR(gdata->jvmti, SetTag)
-                          (gdata->jvmti, weakRef, ptr_to_jlong(node));
+                          (gdata->jvmti, strongOrWeakRef, ptr_to_jlong(node));
     if ( error != JVMTI_ERROR_NONE ) {
-        JNI_FUNC_PTR(env,DeleteWeakGlobalRef)(env, weakRef);
+        if (pin) {
+            JNI_FUNC_PTR(env,DeleteGlobalRef)(env, strongOrWeakRef);
+        } else {
+            JNI_FUNC_PTR(env,DeleteWeakGlobalRef)(env, strongOrWeakRef);
+        }
         jvmtiDeallocate(node);
         return NULL;
     }
 
     /* Fill in RefNode */
-    node->ref      = weakRef;
+    node->ref         = strongOrWeakRef;
-    node->isStrong = JNI_FALSE;
+    node->count       = 1;
-    node->count    = 1;
+    node->strongCount = pin ? 1 : 0;
-    node->seqNum   = newSeqNum();
+    node->seqNum      = newSeqNum();
 
     /* Count RefNode's created */
     gdata->objectsByIDcount++;
@@ -135,7 +146,7 @@ deleteNode(JNIEnv *env, RefNode *node)
         /* Clear tag */
         (void)JVMTI_FUNC_PTR(gdata->jvmti,SetTag)
                             (gdata->jvmti, node->ref, NULL_OBJECT_ID);
-        if (node->isStrong) {
+        if (node->strongCount != 0) {
             JNI_FUNC_PTR(env,DeleteGlobalRef)(env, node->ref);
         } else {
             JNI_FUNC_PTR(env,DeleteWeakGlobalRef)(env, node->ref);
@@ -149,7 +160,7 @@ deleteNode(JNIEnv *env, RefNode *node)
 static jobject
 strengthenNode(JNIEnv *env, RefNode *node)
 {
-    if (!node->isStrong) {
+    if (node->strongCount == 0) {
         jobject strongRef;
 
         strongRef = JNI_FUNC_PTR(env,NewGlobalRef)(env, node->ref);
@@ -164,11 +175,12 @@ strengthenNode(JNIEnv *env, RefNode *node)
         }
         if (strongRef != NULL) {
             JNI_FUNC_PTR(env,DeleteWeakGlobalRef)(env, node->ref);
-            node->ref      = strongRef;
+            node->ref         = strongRef;
-            node->isStrong = JNI_TRUE;
+            node->strongCount = 1;
         }
         return strongRef;
     } else {
+        node->strongCount++;
         return node->ref;
     }
 }
@@ -177,7 +189,7 @@ strengthenNode(JNIEnv *env, RefNode *node)
 static jweak
 weakenNode(JNIEnv *env, RefNode *node)
 {
-    if (node->isStrong) {
+    if (node->strongCount == 1) {
         jweak weakRef;
 
         weakRef = JNI_FUNC_PTR(env,NewWeakGlobalRef)(env, node->ref);
@@ -188,11 +200,12 @@ weakenNode(JNIEnv *env, RefNode *node)
 
         if (weakRef != NULL) {
             JNI_FUNC_PTR(env,DeleteGlobalRef)(env, node->ref);
-            node->ref      = weakRef;
+            node->ref         = weakRef;
-            node->isStrong = JNI_FALSE;
+            node->strongCount = 0;
         }
         return weakRef;
     } else {
+        node->strongCount--;
         return node->ref;
     }
 }
@@ -372,7 +385,8 @@ void
 commonRef_initialize(void)
 {
     gdata->refLock = debugMonitorCreate("JDWP Reference Table Monitor");
-    gdata->nextSeqNum       = 1; /* 0 used for error indication */
+    gdata->nextSeqNum = 1; /* 0 used for error indication */
+    gdata->pinAllCount = 0;
     initializeObjectsByID(HASH_INIT_SIZE);
 }
 
@@ -454,7 +468,7 @@ commonRef_idToRef(JNIEnv *env, jlong id)
 
         node = findNodeByID(env, id);
         if (node != NULL) {
-            if (node->isStrong) {
+            if (node->strongCount != 0) {
                 saveGlobalRef(env, node->ref, &ref);
             } else {
                 jobject lref;
@@ -544,6 +558,84 @@ commonRef_unpin(jlong id)
     return error;
 }
 
+/* Prevent garbage collection of object */
+void
+commonRef_pinAll()
+{
+    debugMonitorEnter(gdata->refLock); {
+        gdata->pinAllCount++;
+
+        if (gdata->pinAllCount == 1) {
+            JNIEnv  *env;
+            RefNode *node;
+            RefNode *prev;
+            int     i;
+
+            env = getEnv();
+
+            /*
+             * Walk through the id-based hash table. Detach any nodes
+             * for which the ref has been collected.
+             */
+            for (i = 0; i < gdata->objectsByIDsize; i++) {
+                node = gdata->objectsByID[i];
+                prev = NULL;
+                while (node != NULL) {
+                    jobject strongRef;
+
+                    strongRef = strengthenNode(env, node);
+
+                    /* Has the object been collected? */
+                    if (strongRef == NULL) {
+                        RefNode *freed;
+
+                        /* Detach from the ID list */
+                        if (prev == NULL) {
+                            gdata->objectsByID[i] = node->next;
+                        } else {
+                            prev->next = node->next;
+                        }
+                        freed = node;
+                        node = node->next;
+                        deleteNode(env, freed);
+                    } else {
+                        prev = node;
+                        node = node->next;
+                    }
+                }
+            }
+        }
+    } debugMonitorExit(gdata->refLock);
+}
+
+/* Permit garbage collection of objects */
+void
+commonRef_unpinAll()
+{
+    debugMonitorEnter(gdata->refLock); {
+        gdata->pinAllCount--;
+
+        if (gdata->pinAllCount == 0) {
+            JNIEnv  *env;
+            RefNode *node;
+            int     i;
+
+            env = getEnv();
+
+            for (i = 0; i < gdata->objectsByIDsize; i++) {
+                for (node = gdata->objectsByID[i]; node != NULL; node = node->next) {
+                    jweak weakRef;
+
+                    weakRef = weakenNode(env, node);
+                    if (weakRef == NULL) {
+                        EXIT_ERROR(AGENT_ERROR_NULL_POINTER,"NewWeakGlobalRef");
+                    }
+                }
+            }
+        }
+    } debugMonitorExit(gdata->refLock);
+}
+
 /* Release tracking of an object by ID */
 void
 commonRef_release(JNIEnv *env, jlong id)
@@ -582,7 +674,7 @@ commonRef_compact(void)
                 prev = NULL;
                 while (node != NULL) {
                     /* Has the object been collected? */
-                    if ( (!node->isStrong) &&
+                    if ( (node->strongCount == 0) &&
                           isSameObject(env, node->ref, NULL)) {
                         RefNode *freed;
 

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2005, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2020, 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
@@ -34,6 +34,8 @@ jobject commonRef_idToRef(JNIEnv *env, jlong id);
 void commonRef_idToRef_delete(JNIEnv *env, jobject ref);
 jvmtiError commonRef_pin(jlong id);
 jvmtiError commonRef_unpin(jlong id);
+void commonRef_pinAll();
+void commonRef_unpinAll();
 void commonRef_releaseMultiple(JNIEnv *env, jlong id, jint refCount);
 void commonRef_release(JNIEnv *env, jlong id);
 void commonRef_compact(void);

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

@@ -1553,6 +1553,12 @@ threadControl_suspendAll(void)
         }
 
         if (error == JVMTI_ERROR_NONE) {
+            /*
+             * Pin all objects to prevent objects from being
+             * garbage collected while the VM is suspended.
+             */
+            commonRef_pinAll();
+
             suspendAllCount++;
         }
 
@@ -1604,6 +1610,11 @@ threadControl_resumeAll(void)
     }
 
     if (suspendAllCount > 0) {
+        /*
+         * Unpin all objects.
+         */
+        commonRef_unpinAll();
+
         suspendAllCount--;
     }
 

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

@@ -65,7 +65,7 @@ typedef struct RefNode {
     jobject      ref;           /* could be strong or weak */
     struct RefNode *next;       /* next RefNode* in bucket chain */
     jint         count;         /* count of references */
-    unsigned     isStrong : 1;  /* 1 means this is a string reference */
+    unsigned     strongCount;   /* count of strong reference */
 } RefNode;
 
 /* Value of a NULL ID */
@@ -128,6 +128,7 @@ typedef struct {
     /* Common References static data */
     jrawMonitorID refLock;
     jlong         nextSeqNum;
+    unsigned      pinAllCount;
     RefNode     **objectsByID;
     int           objectsByIDsize;
     int           objectsByIDcount;

test/hotspot/jtreg/vmTestbase/nsk/jdi/ArrayType/newInstance/newinstance004.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2001, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2001, 2020, 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
@@ -144,9 +144,23 @@ public class newinstance004 {
         log1("      TESTING BEGINS");
 
         for (int i = 0; ; i++) {
-        pipe.println("newcheck");
+            pipe.println("newcheck");
+
+            // There are potentially other non-test Java threads allocating objects and triggering
+            // GC's so we need to suspend the target VM to avoid the objects created in the test
+            // from being accidentally GC'ed. However, we need the target VM temporary resumed
+            // while reading its response. Below we resume the target VM (if required) and suspend
+            // it only after pipe.readln() returns.
+
+            // On the first iteration the target VM is not suspended yet.
+            if (i > 0) {
+                debuggee.resume();
+            }
             line = pipe.readln();
 
+            // Suspending target VM to prevent other non-test Java threads from triggering GCs.
+            debuggee.suspend();
+
             if (line.equals("checkend")) {
                 log2("     : returned string is 'checkend'");
                 break ;
@@ -230,6 +244,7 @@ public class newinstance004 {
     //--------------------------------------------------   test summary section
     //-------------------------------------------------    standard end section
 
+        debuggee.resume();
         pipe.println("quit");
         log2("waiting for the debuggee to finish ...");
         debuggee.waitFor();

test/hotspot/jtreg/vmTestbase/nsk/jdi/ReferenceType/instances/instances002/instances002.java

@@ -189,7 +189,9 @@ public class instances002 extends HeapwalkingDebugger {
                         objectReferences.add(classType.newInstance(breakpointEvent.thread(), method, new ArrayList<Value>(), 0));
                     }
 
+                    debuggee.resume();
                     checkDebugeeAnswer_instances(className, baseInstances);
+                    debuggee.suspend();
 
                     break;
                 }

test/hotspot/jtreg/vmTestbase/nsk/jdi/VMOutOfMemoryException/VMOutOfMemoryException001/VMOutOfMemoryException001.java

@@ -79,7 +79,14 @@ public class VMOutOfMemoryException001 extends TestDebuggerType2 {
             // create array in debuggee VM till VMOutOfMemoryException
             while (true) {
                 ArrayReference array = referenceType.newInstance(100000);
-                array.disableCollection();
+                try {
+                    // Since the VM is not suspended, the object may have been collected
+                    // before disableCollection() could be called on it. Just ignore and
+                    // continue doing allocations until we run out of memory.
+                    array.disableCollection();
+                } catch (ObjectCollectedException e) {
+                    continue;
+                }
                 objects.add(array);
             }
         } catch (VMOutOfMemoryException e) {

test/hotspot/jtreg/vmTestbase/nsk/share/jdi/sde/SDEDebuggee.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2007, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2007, 2020, 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
@@ -67,10 +67,15 @@ public class SDEDebuggee extends AbstractJDIDebuggee {
         return false;
     }
 
+    // Keep class loader alive to avoid ObjectCollectedException
+    // on the debugger side, in case the GC unloads the class and
+    // invalidates code locations.
+    private TestClassLoader classLoader;
+
     // create instance of given class and execute all methods which names start
     // with 'sde_testMethod'
     private void executeTestMethods(String className) {
-        TestClassLoader classLoader = new TestClassLoader();
+        classLoader = new TestClassLoader();
         classLoader.setClassPath(classpath);
 
         try {