8062771: Core reflection should use final fields whenever possible

Reviewed-by: psandoz, darcy

jdk/src/java.base/share/classes/sun/reflect/BootstrapConstructorAccessorImpl.java

@@ -32,7 +32,7 @@ import java.lang.reflect.Constructor;
     bootstrapping. */
 
 class BootstrapConstructorAccessorImpl extends ConstructorAccessorImpl {
-    private Constructor<?> constructor;
+    private final Constructor<?> constructor;
 
     BootstrapConstructorAccessorImpl(Constructor<?> c) {
         this.constructor = c;

jdk/src/java.base/share/classes/sun/reflect/InstantiationExceptionConstructorAccessorImpl.java

@@ -33,7 +33,7 @@ import java.lang.reflect.InvocationTargetException;
 
 class InstantiationExceptionConstructorAccessorImpl
     extends ConstructorAccessorImpl {
-    private String message;
+    private final String message;
 
     InstantiationExceptionConstructorAccessorImpl(String message) {
         this.message = message;

jdk/src/java.base/share/classes/sun/reflect/Label.java

@@ -47,10 +47,10 @@ class Label {
         }
         // This won't work for more than one assembler anyway, so this is
         // unnecessary
-        ClassFileAssembler asm;
-        short instrBCI;
-        short patchBCI;
-        int   stackDepth;
+        final ClassFileAssembler asm;
+        final short instrBCI;
+        final short patchBCI;
+        final int   stackDepth;
     }
     private List<PatchInfo> patches = new ArrayList<>();
 

jdk/src/java.base/share/classes/sun/reflect/NativeConstructorAccessorImpl.java

@@ -32,7 +32,7 @@ import sun.reflect.misc.ReflectUtil;
     afterward, switches to bytecode-based implementation */
 
 class NativeConstructorAccessorImpl extends ConstructorAccessorImpl {
-    private Constructor<?> c;
+    private final Constructor<?> c;
     private DelegatingConstructorAccessorImpl parent;
     private int numInvocations;
 

jdk/src/java.base/share/classes/sun/reflect/NativeMethodAccessorImpl.java

@@ -32,7 +32,7 @@ import sun.reflect.misc.ReflectUtil;
     switches to bytecode-based implementation */
 
 class NativeMethodAccessorImpl extends MethodAccessorImpl {
-    private Method method;
+    private final Method method;
     private DelegatingMethodAccessorImpl parent;
     private int numInvocations;
 

jdk/src/java.base/share/classes/sun/reflect/ReflectionFactory.java

@@ -50,9 +50,9 @@ import sun.reflect.misc.ReflectUtil;
 public class ReflectionFactory {
 
     private static boolean initted = false;
-    private static Permission reflectionFactoryAccessPerm
+    private static final Permission reflectionFactoryAccessPerm
         = new RuntimePermission("reflectionFactoryAccess");
-    private static ReflectionFactory soleInstance = new ReflectionFactory();
+    private static final ReflectionFactory soleInstance = new ReflectionFactory();
     // Provides access to package-private mechanisms in java.lang.reflect
     private static volatile LangReflectAccess langReflectAccess;
 

jdk/src/java.base/share/classes/sun/reflect/SignatureIterator.java

@@ -28,7 +28,7 @@ package sun.reflect;
 /** Assists in iterating down a method's signature */
 
 public class SignatureIterator {
-    private String sig;
+    private final String sig;
     private int idx;
 
     public SignatureIterator(String sig) {

jdk/src/java.base/share/classes/sun/reflect/generics/factory/CoreReflectionFactory.java

@@ -45,8 +45,8 @@ import sun.reflect.generics.tree.FieldTypeSignature;
  * core reflection (java.lang.reflect).
  */
 public class CoreReflectionFactory implements GenericsFactory {
-    private GenericDeclaration decl;
-    private Scope scope;
+    private final GenericDeclaration decl;
+    private final Scope scope;
 
     private CoreReflectionFactory(GenericDeclaration d, Scope s) {
         decl = d;

jdk/src/java.base/share/classes/sun/reflect/generics/reflectiveObjects/GenericArrayTypeImpl.java

@@ -34,7 +34,7 @@ import java.util.Objects;
  */
 public class GenericArrayTypeImpl
     implements GenericArrayType {
-    private Type genericComponentType;
+    private final Type genericComponentType;
 
     // private constructor enforces use of static factory
     private GenericArrayTypeImpl(Type ct) {

jdk/src/java.base/share/classes/sun/reflect/generics/reflectiveObjects/LazyReflectiveObjectGenerator.java

@@ -40,7 +40,7 @@ import sun.reflect.generics.visitor.Reifier;
  *
 */
 public abstract class LazyReflectiveObjectGenerator {
-    private GenericsFactory factory; // cached factory
+    private final GenericsFactory factory; // cached factory
 
     protected LazyReflectiveObjectGenerator(GenericsFactory f) {
         factory = f;

jdk/src/java.base/share/classes/sun/reflect/generics/reflectiveObjects/ParameterizedTypeImpl.java

@@ -38,9 +38,9 @@ import java.util.Objects;
 /** Implementing class for ParameterizedType interface. */
 
 public class ParameterizedTypeImpl implements ParameterizedType {
-    private Type[] actualTypeArguments;
-    private Class<?>  rawType;
-    private Type   ownerType;
+    private final Type[] actualTypeArguments;
+    private final Class<?>  rawType;
+    private final Type   ownerType;
 
     private ParameterizedTypeImpl(Class<?> rawType,
                                   Type[] actualTypeArguments,

jdk/src/java.base/share/classes/sun/reflect/generics/repository/AbstractRepository.java

@@ -40,9 +40,9 @@ public abstract class AbstractRepository<T extends Tree> {
 
     // A factory used to produce reflective objects. Provided when the
     //repository is created. Will vary across implementations.
-    private GenericsFactory factory;
+    private final GenericsFactory factory;
 
-    private T tree; // the AST for the generic type info
+    private final T tree; // the AST for the generic type info
 
     //accessors
     private GenericsFactory getFactory() { return factory;}

jdk/src/java.base/share/classes/sun/reflect/generics/scope/AbstractScope.java

@@ -41,7 +41,7 @@ import java.lang.reflect.TypeVariable;
 public abstract class AbstractScope<D extends GenericDeclaration>
     implements Scope {
 
-    private D recvr; // the declaration whose scope this instance represents
+    private final D recvr; // the declaration whose scope this instance represents
     private Scope enclosingScope; // the enclosing scope of this scope
 
     /**

jdk/src/java.base/share/classes/sun/reflect/generics/scope/DummyScope.java

@@ -38,7 +38,7 @@ import java.lang.reflect.TypeVariable;
 public class DummyScope implements Scope {
     // Caches the unique instance of this class; instances contain no data
     // so we can use the singleton pattern
-    private static DummyScope singleton = new DummyScope();
+    private static final DummyScope singleton = new DummyScope();
 
     // constructor is private to enforce use of factory method
     private DummyScope(){}

jdk/src/java.base/share/classes/sun/reflect/generics/tree/ArrayTypeSignature.java

@@ -28,7 +28,7 @@ package sun.reflect.generics.tree;
 import sun.reflect.generics.visitor.TypeTreeVisitor;
 
 public class ArrayTypeSignature implements FieldTypeSignature {
-    private TypeSignature componentType;
+    private final TypeSignature componentType;
 
     private ArrayTypeSignature(TypeSignature ct) {componentType = ct;}
 

jdk/src/java.base/share/classes/sun/reflect/generics/tree/BooleanSignature.java

@@ -29,7 +29,7 @@ import sun.reflect.generics.visitor.TypeTreeVisitor;
 
 /** AST that represents the type boolean. */
 public class BooleanSignature implements BaseType {
-    private static BooleanSignature singleton = new BooleanSignature();
+    private static final BooleanSignature singleton = new BooleanSignature();
 
     private BooleanSignature(){}
 

jdk/src/java.base/share/classes/sun/reflect/generics/tree/BottomSignature.java

@@ -28,7 +28,7 @@ package sun.reflect.generics.tree;
 import sun.reflect.generics.visitor.TypeTreeVisitor;
 
 public class BottomSignature implements FieldTypeSignature {
-    private static BottomSignature singleton = new BottomSignature();
+    private static final BottomSignature singleton = new BottomSignature();
 
     private BottomSignature(){}
 

jdk/src/java.base/share/classes/sun/reflect/generics/tree/ByteSignature.java

@@ -29,7 +29,7 @@ import sun.reflect.generics.visitor.TypeTreeVisitor;
 
 /** AST that represents the type byte. */
 public class ByteSignature implements BaseType {
-    private static ByteSignature singleton = new ByteSignature();
+    private static final ByteSignature singleton = new ByteSignature();
 
     private ByteSignature(){}
 

jdk/src/java.base/share/classes/sun/reflect/generics/tree/CharSignature.java

@@ -29,7 +29,7 @@ import sun.reflect.generics.visitor.TypeTreeVisitor;
 
 /** AST that represents the type char. */
 public class CharSignature implements BaseType {
-    private static CharSignature singleton = new CharSignature();
+    private static final CharSignature singleton = new CharSignature();
 
     private CharSignature(){}
 

jdk/src/java.base/share/classes/sun/reflect/generics/tree/ClassSignature.java

@@ -28,9 +28,9 @@ package sun.reflect.generics.tree;
 import sun.reflect.generics.visitor.Visitor;
 
 public class ClassSignature implements Signature {
-    private FormalTypeParameter[] formalTypeParams;
-    private ClassTypeSignature superclass;
-    private ClassTypeSignature[] superInterfaces;
+    private final FormalTypeParameter[] formalTypeParams;
+    private final ClassTypeSignature superclass;
+    private final ClassTypeSignature[] superInterfaces;
 
     private ClassSignature(FormalTypeParameter[] ftps,
                                       ClassTypeSignature sc,

jdk/src/java.base/share/classes/sun/reflect/generics/tree/ClassTypeSignature.java

@@ -33,7 +33,7 @@ import sun.reflect.generics.visitor.TypeTreeVisitor;
  * AST representing class types.
  */
 public class ClassTypeSignature implements FieldTypeSignature {
-    private List<SimpleClassTypeSignature> path;
+    private final List<SimpleClassTypeSignature> path;
 
 
     private ClassTypeSignature(List<SimpleClassTypeSignature> p) {

jdk/src/java.base/share/classes/sun/reflect/generics/tree/DoubleSignature.java

@@ -29,7 +29,7 @@ import sun.reflect.generics.visitor.TypeTreeVisitor;
 
 /** AST that represents the type double. */
 public class DoubleSignature implements BaseType {
-    private static DoubleSignature singleton = new DoubleSignature();
+    private static final DoubleSignature singleton = new DoubleSignature();
 
     private DoubleSignature(){}
 

jdk/src/java.base/share/classes/sun/reflect/generics/tree/FloatSignature.java

@@ -29,7 +29,7 @@ import sun.reflect.generics.visitor.TypeTreeVisitor;
 
 /** AST that represents the type float. */
 public class FloatSignature implements BaseType {
-    private static FloatSignature singleton = new FloatSignature();
+    private static final FloatSignature singleton = new FloatSignature();
 
     private FloatSignature(){}
 

jdk/src/java.base/share/classes/sun/reflect/generics/tree/FormalTypeParameter.java

@@ -29,8 +29,8 @@ import sun.reflect.generics.visitor.TypeTreeVisitor;
 
 /** AST that represents a formal type parameter. */
 public class FormalTypeParameter implements TypeTree {
-    private String name;
-    private FieldTypeSignature[] bounds;
+    private final String name;
+    private final FieldTypeSignature[] bounds;
 
     private FormalTypeParameter(String n, FieldTypeSignature[] bs) {
         name = n;

jdk/src/java.base/share/classes/sun/reflect/generics/tree/IntSignature.java

@@ -29,7 +29,7 @@ import sun.reflect.generics.visitor.TypeTreeVisitor;
 
 /** AST that represents the type int. */
 public class IntSignature implements BaseType {
-    private static IntSignature singleton = new IntSignature();
+    private static final IntSignature singleton = new IntSignature();
 
     private IntSignature(){}
 

jdk/src/java.base/share/classes/sun/reflect/generics/tree/LongSignature.java

@@ -29,7 +29,7 @@ import sun.reflect.generics.visitor.TypeTreeVisitor;
 
 /** AST that represents the type long. */
 public class LongSignature implements BaseType {
-    private static LongSignature singleton = new LongSignature();
+    private static final LongSignature singleton = new LongSignature();
 
     private LongSignature(){}
 

jdk/src/java.base/share/classes/sun/reflect/generics/tree/MethodTypeSignature.java

@@ -28,10 +28,10 @@ package sun.reflect.generics.tree;
 import sun.reflect.generics.visitor.Visitor;
 
 public class MethodTypeSignature implements Signature {
-    private FormalTypeParameter[] formalTypeParams;
-    private TypeSignature[] parameterTypes;
-    private ReturnType returnType;
-    private FieldTypeSignature[] exceptionTypes;
+    private final FormalTypeParameter[] formalTypeParams;
+    private final TypeSignature[] parameterTypes;
+    private final ReturnType returnType;
+    private final FieldTypeSignature[] exceptionTypes;
 
     private MethodTypeSignature(FormalTypeParameter[] ftps,
                                 TypeSignature[] pts,

jdk/src/java.base/share/classes/sun/reflect/generics/tree/ShortSignature.java

@@ -29,7 +29,7 @@ import sun.reflect.generics.visitor.TypeTreeVisitor;
 
 /** AST that represents the type short. */
 public class ShortSignature implements BaseType {
-    private static ShortSignature singleton = new ShortSignature();
+    private static final ShortSignature singleton = new ShortSignature();
 
     private ShortSignature(){}
 

jdk/src/java.base/share/classes/sun/reflect/generics/tree/SimpleClassTypeSignature.java

@@ -28,9 +28,9 @@ package sun.reflect.generics.tree;
 import sun.reflect.generics.visitor.TypeTreeVisitor;
 
 public class SimpleClassTypeSignature implements FieldTypeSignature {
-    private boolean dollar;
-    private String name;
-    private TypeArgument[] typeArgs;
+    private final boolean dollar;
+    private final String name;
+    private final TypeArgument[] typeArgs;
 
     private SimpleClassTypeSignature(String n, boolean dollar, TypeArgument[] tas) {
         name = n;

jdk/src/java.base/share/classes/sun/reflect/generics/tree/TypeVariableSignature.java

@@ -28,7 +28,7 @@ package sun.reflect.generics.tree;
 import sun.reflect.generics.visitor.TypeTreeVisitor;
 
 public class TypeVariableSignature implements FieldTypeSignature {
-    private String identifier;
+    private final String identifier;
 
     private TypeVariableSignature(String id) {identifier = id;}
 

jdk/src/java.base/share/classes/sun/reflect/generics/tree/VoidDescriptor.java

@@ -30,7 +30,7 @@ import sun.reflect.generics.visitor.TypeTreeVisitor;
 
 /** AST that represents the pseudo-type void. */
 public class VoidDescriptor implements ReturnType {
-    private static VoidDescriptor singleton = new VoidDescriptor();
+    private static final VoidDescriptor singleton = new VoidDescriptor();
 
     private VoidDescriptor(){}
 

jdk/src/java.base/share/classes/sun/reflect/misc/MethodUtil.java

@@ -76,9 +76,9 @@ class Trampoline {
  * Create a trampoline class.
  */
 public final class MethodUtil extends SecureClassLoader {
-    private static String MISC_PKG = "sun.reflect.misc.";
-    private static String TRAMPOLINE = MISC_PKG + "Trampoline";
-    private static Method bounce = getTrampoline();
+    private static final String MISC_PKG = "sun.reflect.misc.";
+    private static final String TRAMPOLINE = MISC_PKG + "Trampoline";
+    private static final Method bounce = getTrampoline();
 
     private MethodUtil() {
         super();

jdk/test/java/lang/reflect/Generics/ThreadSafety.java

@@ -0,0 +1,129 @@
+/*
+ * Copyright 2014 Google Inc.  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
+ * @bug 8062771 8016236
+ * @summary Test publication of Class objects via a data race
+ * @run testng ThreadSafety
+ */
+
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.util.Collections;
+import java.util.concurrent.BrokenBarrierException;
+import java.util.concurrent.Callable;
+import java.util.concurrent.CyclicBarrier;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeoutException;
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.testng.Assert.*;
+import org.testng.annotations.Test;
+
+/**
+ * A test resulting from an attempt to repro this failure (in guice):
+ *
+ * java.lang.NullPointerException
+ *   at sun.reflect.generics.visitor.Reifier.visitClassTypeSignature(Reifier.java:125)
+ *   at sun.reflect.generics.tree.ClassTypeSignature.accept(ClassTypeSignature.java:49)
+ *   at sun.reflect.generics.repository.ClassRepository.getSuperclass(ClassRepository.java:84)
+ *   at java.lang.Class.getGenericSuperclass(Class.java:692)
+ *   at com.google.inject.TypeLiteral.getSuperclassTypeParameter(TypeLiteral.java:99)
+ *   at com.google.inject.TypeLiteral.<init>(TypeLiteral.java:79)
+ *
+ * However, as one would expect with thread safety problems in reflection, these
+ * are very hard to reproduce.  This very test has never been observed to fail,
+ * but a similar test has been observed to fail about once in 2000 executions
+ * (about once every 6 CPU-hours), in jdk7 only.  It appears to be fixed in jdk8+ by:
+ *
+ * 8016236: Class.getGenericInterfaces performance improvement.
+ * (by making Class.genericInfo volatile)
+ */
+public class ThreadSafety {
+    public static class EmptyClass {
+        public static class EmptyGenericSuperclass<T> {}
+        public static class EmptyGenericSubclass<T> extends EmptyGenericSuperclass<T> {}
+    }
+
+    /** published via data race */
+    private Class<?> racyClass = Object.class;
+
+    private URL[] urls = ((URLClassLoader) ThreadSafety.class.getClassLoader()).getURLs();
+
+    private Class<?> createNewEmptyGenericSubclassClass() throws Exception {
+        URLClassLoader ucl = new URLClassLoader(urls, null);
+        return Class.forName("ThreadSafety$EmptyClass$EmptyGenericSubclass", true, ucl);
+    }
+
+    @Test
+    public void testRacy_getGenericSuperclass() throws Exception {
+        final int nThreads = 10;
+        final int iterations = 30;
+        final int timeout = 10;
+        final CyclicBarrier newCycle = new CyclicBarrier(nThreads);
+        final Callable<Void> task = new Callable<Void>() {
+            public Void call() throws Exception {
+                for (int i = 0; i < iterations; i++) {
+                    final int threadId;
+                    try {
+                        threadId = newCycle.await(timeout, SECONDS);
+                    } catch (BrokenBarrierException e) {
+                        return null;
+                    }
+                    for (int j = 0; j < iterations; j++) {
+                        // one thread publishes the class object via a data
+                        // race, for the other threads to consume.
+                        if (threadId == 0) {
+                            racyClass = createNewEmptyGenericSubclassClass();
+                        } else {
+                            racyClass.getGenericSuperclass();
+                        }
+                    }
+                }
+                return null;
+            }};
+
+        final ExecutorService pool = Executors.newFixedThreadPool(nThreads);
+        try {
+            for (Future<Void> future :
+                     pool.invokeAll(Collections.nCopies(nThreads, task))) {
+                try {
+                    future.get(iterations * timeout, SECONDS);
+                } catch (ExecutionException e) {
+                    // ignore "collateral damage"
+                    if (!(e.getCause() instanceof BrokenBarrierException)
+                        &&
+                        !(e.getCause() instanceof TimeoutException)) {
+                        throw e;
+                    }
+                }
+            }
+        } finally {
+            pool.shutdownNow();
+            assertTrue(pool.awaitTermination(2 * timeout, SECONDS));
+        }
+    }
+}