8218755: Refix Symbol leak in prepend_host_package_name

Fix Symbol refcounting again, add comment and a test.

Reviewed-by: kbarrett, dholmes

src/hotspot/share/classfile/classFileParser.cpp

@@ -5720,9 +5720,8 @@ void ClassFileParser::fill_instance_klass(InstanceKlass* ik, bool changed_by_loa
 
 void ClassFileParser::update_class_name(Symbol* new_class_name) {
   // Decrement the refcount in the old name, since we're clobbering it.
-  if (_class_name != NULL) {
+  _class_name->decrement_refcount();
-    _class_name->decrement_refcount();
+
-  }
   _class_name = new_class_name;
   // Increment the refcount of the new name.
   // Now the ClassFileParser owns this name and will decrement in
@@ -5753,11 +5752,16 @@ void ClassFileParser::prepend_host_package_name(const InstanceKlass* unsafe_anon
     // characters.  So, do a strncpy instead of using sprintf("%s...").
     strncpy(new_anon_name + host_pkg_len + 1, (char *)_class_name->base(), class_name_len);
 
+    // Decrement old _class_name to avoid leaking.
+    _class_name->decrement_refcount();
+
     // Create a symbol and update the anonymous class name.
-    Symbol* new_name = SymbolTable::new_symbol(new_anon_name,
+    // The new class name is created with a refcount of one. When installed into the InstanceKlass,
+    // it'll be two and when the ClassFileParser destructor runs, it'll go back to one and get deleted
+    // when the class is unloaded.
+    _class_name = SymbolTable::new_symbol(new_anon_name,
                                           (int)host_pkg_len + 1 + class_name_len,
                                           CHECK);
-    update_class_name(new_name);
   }
 }
 
@@ -5861,7 +5865,8 @@ ClassFileParser::ClassFileParser(ClassFileStream* stream,
   _has_vanilla_constructor(false),
   _max_bootstrap_specifier_index(-1) {
 
-  update_class_name(name != NULL ? name : vmSymbols::unknown_class_name());
+  _class_name = name != NULL ? name : vmSymbols::unknown_class_name();
+  _class_name->increment_refcount();
 
   assert(THREAD->is_Java_thread(), "invariant");
   assert(_loader_data != NULL, "invariant");
@@ -6086,8 +6091,7 @@ void ClassFileParser::parse_stream(const ClassFileStream* const stream,
   Symbol* const class_name_in_cp = cp->klass_name_at(_this_class_index);
   assert(class_name_in_cp != NULL, "class_name can't be null");
 
-  // Update _class_name which could be null previously
+  // Update _class_name to reflect the name in the constant pool
-  // to reflect the name in the constant pool
   update_class_name(class_name_in_cp);
 
   // Don't need to check whether this class name is legal or not.

test/hotspot/jtreg/runtime/defineAnonClass/AnonSymbolLeak.java

@@ -0,0 +1,75 @@
+/*
+ * Copyright (c) 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
+ * 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.
+ */
+
+// This is copied from DefineAnon to test Symbol Refcounting for the package prepended name.
+
+package p1;
+
+import jdk.internal.org.objectweb.asm.ClassWriter;
+import jdk.internal.org.objectweb.asm.MethodVisitor;
+import jdk.internal.org.objectweb.asm.Opcodes;
+import jdk.internal.misc.Unsafe;
+
+
+class T {
+    static           protected void test0() { System.out.println("test0 (public)"); }
+    static           protected void test1() { System.out.println("test1 (protected)"); }
+    static /*package-private*/ void test2() { System.out.println("test2 (package)"); }
+    static             private void test3() { System.out.println("test3 (private)"); }
+}
+
+public class AnonSymbolLeak {
+
+    private static final Unsafe UNSAFE = Unsafe.getUnsafe();
+
+    static Class<?> getAnonClass(Class<?> hostClass, final String className) {
+        final String superName = "java/lang/Object";
+        ClassWriter cw = new ClassWriter(ClassWriter.COMPUTE_MAXS + ClassWriter.COMPUTE_FRAMES);
+        cw.visit(Opcodes.V1_8, Opcodes.ACC_PUBLIC + Opcodes.ACC_FINAL + Opcodes.ACC_SUPER, className, null, superName, null);
+
+        MethodVisitor mv = cw.visitMethod(Opcodes.ACC_STATIC | Opcodes.ACC_PUBLIC, "test", "()V", null, null);
+        mv.visitMethodInsn(Opcodes.INVOKESTATIC, "p1/T", "test0", "()V", false);
+        mv.visitMethodInsn(Opcodes.INVOKESTATIC, "p1/T", "test1", "()V", false);
+        mv.visitMethodInsn(Opcodes.INVOKESTATIC, "p1/T", "test2", "()V", false);
+        mv.visitMethodInsn(Opcodes.INVOKESTATIC, "p1/T", "test3", "()V", false);
+        mv.visitInsn(Opcodes.RETURN);
+        mv.visitMaxs(0, 0);
+        mv.visitEnd();
+
+        final byte[] classBytes = cw.toByteArray();
+        Class<?> invokerClass = UNSAFE.defineAnonymousClass(hostClass, classBytes, new Object[0]);
+        UNSAFE.ensureClassInitialized(invokerClass);
+        return invokerClass;
+    }
+
+    public static void test() throws Throwable {
+        // AnonClass is injected into package p1.
+        System.out.println("Injecting from the same package (p1):");
+        Class<?> p1cls = getAnonClass(T.class, "AnonClass");
+        p1cls.getMethod("test").invoke(null);
+    }
+
+    public static void main(java.lang.String[] unused) throws Throwable {
+        test();
+    }
+}

test/hotspot/jtreg/runtime/defineAnonClass/DefineAnon.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 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
@@ -27,6 +27,7 @@
  * @library /testlibrary
  * @modules java.base/jdk.internal.org.objectweb.asm
  *          java.management
+ *          java.base/jdk.internal.misc
  * @compile -XDignore.symbol.file=true DefineAnon.java
  * @run main/othervm p1.DefineAnon
  */
@@ -36,7 +37,7 @@ package p1;
 import jdk.internal.org.objectweb.asm.ClassWriter;
 import jdk.internal.org.objectweb.asm.MethodVisitor;
 import jdk.internal.org.objectweb.asm.Opcodes;
-import sun.misc.Unsafe;
+import jdk.internal.misc.Unsafe;
 
 
 class T {
@@ -48,17 +49,7 @@ class T {
 
 public class DefineAnon {
 
-    private static Unsafe getUnsafe() {
+    private static final Unsafe UNSAFE = Unsafe.getUnsafe();
-        try {
-            java.lang.reflect.Field singleoneInstanceField = Unsafe.class.getDeclaredField("theUnsafe");
-            singleoneInstanceField.setAccessible(true);
-            return (Unsafe) singleoneInstanceField.get(null);
-        } catch (Throwable ex) {
-            throw new RuntimeException("Was unable to get Unsafe instance.");
-        }
-    }
-
-    static Unsafe UNSAFE = DefineAnon.getUnsafe();
 
     static Class<?> getAnonClass(Class<?> hostClass, final String className) {
         final String superName = "java/lang/Object";

test/hotspot/jtreg/runtime/defineAnonClass/TestAnonSymbolLeak.java

@@ -0,0 +1,77 @@
+/*
+ * Copyright (c) 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
+ * 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.
+ */
+
+
+/*
+ * @test TestAnonSymbolLeak
+ * @bug 8218755
+ * @requires vm.opt.final.ClassUnloading
+ * @modules java.base/jdk.internal.misc
+ *          java.base/jdk.internal.org.objectweb.asm
+ *          java.management
+ * @library /test/lib /runtime/testlibrary
+ * @build p1.AnonSymbolLeak
+ * @build sun.hotspot.WhiteBox
+ * @run main ClassFileInstaller sun.hotspot.WhiteBox
+ *                              sun.hotspot.WhiteBox$WhiteBoxPermission
+ * @run main/othervm -Xbootclasspath/a:. -Xmn8m -XX:+UnlockDiagnosticVMOptions -Xlog:class+unload -XX:+WhiteBoxAPI TestAnonSymbolLeak
+ */
+
+import java.lang.reflect.Method;
+import sun.hotspot.WhiteBox;
+import jdk.test.lib.Asserts;
+
+public class TestAnonSymbolLeak {
+    static String className = "p1.AnonSymbolLeak";
+
+    private static class ClassUnloadTestMain {
+        public static void test() throws Exception {
+            // Load the AnonSymbolLeak class in a new class loader, run it, which loads
+            // an unsafe anonymous class in the same package p1.  Then unload it.
+            // Then test that the refcount of the symbol created for the prepended name doesn't leak.
+            ClassLoader cl = ClassUnloadCommon.newClassLoader();
+            Class<?> c = cl.loadClass(className);
+            c.getMethod("test").invoke(null);
+            cl = null; c = null;
+            ClassUnloadCommon.triggerUnloading();
+        }
+    }
+
+    public static WhiteBox wb = WhiteBox.getWhiteBox();
+
+    public static void main(String... args) throws Exception {
+        String oldName = "AnonClass";
+        String prependedName = "p1/AnonClass";
+        ClassUnloadCommon.failIf(wb.isClassAlive(className), "should not be loaded");
+        int countBeforeOld = wb.getSymbolRefcount(oldName);
+        int countBefore = wb.getSymbolRefcount(prependedName);
+        ClassUnloadTestMain.test();
+        ClassUnloadCommon.failIf(wb.isClassAlive(className), "should be unloaded");
+        int countAfterOld = wb.getSymbolRefcount(oldName);
+        int countAfter = wb.getSymbolRefcount(prependedName);
+        Asserts.assertEquals(countBeforeOld, countAfterOld); // no leaks to the old name
+        System.out.println("count before and after " + countBeforeOld + " " + countAfterOld);
+        Asserts.assertEquals(countBefore, countAfter);       // no leaks to the prepended name
+        System.out.println("count before and after " + countBefore + " " + countAfter);
+    }
+}

test/hotspot/jtreg/runtime/defineAnonClass/UnsafeDefMeths.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2018, 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
@@ -28,6 +28,7 @@
  * @library /testlibrary
  * @modules java.base/jdk.internal.org.objectweb.asm
  *          java.management
+ *          java.base/jdk.internal.misc
  * @compile -XDignore.symbol.file=true UnsafeDefMeths.java
  * @run main UnsafeDefMeths
  */
@@ -35,7 +36,7 @@
 import jdk.internal.org.objectweb.asm.ClassWriter;
 import jdk.internal.org.objectweb.asm.MethodVisitor;
 import jdk.internal.org.objectweb.asm.Type;
-import sun.misc.Unsafe;
+import jdk.internal.misc.Unsafe;
 
 import java.lang.invoke.MethodType;
 import java.lang.reflect.Field;
@@ -57,18 +58,7 @@ import static jdk.internal.org.objectweb.asm.Opcodes.V1_8;
 
 public class UnsafeDefMeths {
 
-    static final Unsafe UNSAFE;
+    private static final Unsafe UNSAFE = Unsafe.getUnsafe();
-
-    static {
-        try {
-            Field unsafeField = Unsafe.class.getDeclaredField("theUnsafe");
-            unsafeField.setAccessible(true);
-            UNSAFE = (Unsafe) unsafeField.get(null);
-        }
-        catch (Exception e) {
-            throw new InternalError(e);
-        }
-    }
 
     interface Resource {
         Pointer ptr();