8009728: nsk/jvmti/AttachOnDemand/attach030 crashes on Win32

Summary: ActiveMethodOopsCache was used to keep track of old versions of some methods that are cached in Universe but is buggy with permgen removal and not needed anymore
Reviewed-by: sspitsyn, dcubed, mseledtsov

src/share/vm/memory/universe.cpp

@@ -105,10 +105,9 @@ objArrayOop Universe::_the_empty_class_klass_array    = NULL;
 Array<Klass*>* Universe::_the_array_interfaces_array = NULL;
 oop Universe::_the_null_string                        = NULL;
 oop Universe::_the_min_jint_string                   = NULL;
-LatestMethodOopCache* Universe::_finalizer_register_cache = NULL;
-LatestMethodOopCache* Universe::_loader_addClass_cache    = NULL;
-LatestMethodOopCache* Universe::_pd_implies_cache         = NULL;
-ActiveMethodOopsCache* Universe::_reflect_invoke_cache    = NULL;
+LatestMethodCache* Universe::_finalizer_register_cache = NULL;
+LatestMethodCache* Universe::_loader_addClass_cache    = NULL;
+LatestMethodCache* Universe::_pd_implies_cache         = NULL;
 oop Universe::_out_of_memory_error_java_heap          = NULL;
 oop Universe::_out_of_memory_error_metaspace          = NULL;
 oop Universe::_out_of_memory_error_class_metaspace    = NULL;
@@ -225,7 +224,6 @@ void Universe::serialize(SerializeClosure* f, bool do_all) {
   f->do_ptr((void**)&_the_empty_klass_array);
   _finalizer_register_cache->serialize(f);
   _loader_addClass_cache->serialize(f);
-  _reflect_invoke_cache->serialize(f);
   _pd_implies_cache->serialize(f);
 }
 
@@ -649,10 +647,9 @@ jint universe_init() {
 
   // We have a heap so create the Method* caches before
   // Metaspace::initialize_shared_spaces() tries to populate them.
-  Universe::_finalizer_register_cache = new LatestMethodOopCache();
-  Universe::_loader_addClass_cache    = new LatestMethodOopCache();
-  Universe::_pd_implies_cache         = new LatestMethodOopCache();
-  Universe::_reflect_invoke_cache     = new ActiveMethodOopsCache();
+  Universe::_finalizer_register_cache = new LatestMethodCache();
+  Universe::_loader_addClass_cache    = new LatestMethodCache();
+  Universe::_pd_implies_cache         = new LatestMethodCache();
 
   if (UseSharedSpaces) {
     // Read the data structures supporting the shared spaces (shared
@@ -1088,35 +1085,21 @@ bool universe_post_init() {
                                   vmSymbols::register_method_name(),
                                   vmSymbols::register_method_signature());
   if (m == NULL || !m->is_static()) {
-    THROW_MSG_(vmSymbols::java_lang_NoSuchMethodException(),
-      "java.lang.ref.Finalizer.register", false);
+    tty->print_cr("Unable to link/verify Finalizer.register method");
+    return false; // initialization failed (cannot throw exception yet)
   }
   Universe::_finalizer_register_cache->init(
-    SystemDictionary::Finalizer_klass(), m, CHECK_false);
-
-  // Resolve on first use and initialize class.
-  // Note: No race-condition here, since a resolve will always return the same result
-
-  // Setup method for security checks
-  k = SystemDictionary::resolve_or_fail(vmSymbols::java_lang_reflect_Method(), true, CHECK_false);
-  k_h = instanceKlassHandle(THREAD, k);
-  k_h->link_class(CHECK_false);
-  m = k_h->find_method(vmSymbols::invoke_name(), vmSymbols::object_object_array_object_signature());
-  if (m == NULL || m->is_static()) {
-    THROW_MSG_(vmSymbols::java_lang_NoSuchMethodException(),
-      "java.lang.reflect.Method.invoke", false);
-  }
-  Universe::_reflect_invoke_cache->init(k_h(), m, CHECK_false);
+    SystemDictionary::Finalizer_klass(), m);
 
   // Setup method for registering loaded classes in class loader vector
   InstanceKlass::cast(SystemDictionary::ClassLoader_klass())->link_class(CHECK_false);
   m = InstanceKlass::cast(SystemDictionary::ClassLoader_klass())->find_method(vmSymbols::addClass_name(), vmSymbols::class_void_signature());
   if (m == NULL || m->is_static()) {
-    THROW_MSG_(vmSymbols::java_lang_NoSuchMethodException(),
-      "java.lang.ClassLoader.addClass", false);
+    tty->print_cr("Unable to link/verify ClassLoader.addClass method");
+    return false; // initialization failed (cannot throw exception yet)
   }
   Universe::_loader_addClass_cache->init(
-    SystemDictionary::ClassLoader_klass(), m, CHECK_false);
+    SystemDictionary::ClassLoader_klass(), m);
 
   // Setup method for checking protection domain
   InstanceKlass::cast(SystemDictionary::ProtectionDomain_klass())->link_class(CHECK_false);
@@ -1132,7 +1115,7 @@ bool universe_post_init() {
       return false; // initialization failed
     }
     Universe::_pd_implies_cache->init(
-      SystemDictionary::ProtectionDomain_klass(), m, CHECK_false);;
+      SystemDictionary::ProtectionDomain_klass(), m);;
   }
 
   // The folowing is initializing converter functions for serialization in
@@ -1455,7 +1438,7 @@ void Universe::compute_verify_oop_data() {
 }
 
 
-void CommonMethodOopCache::init(Klass* k, Method* m, TRAPS) {
+void LatestMethodCache::init(Klass* k, Method* m) {
   if (!UseSharedSpaces) {
     _klass = k;
   }
@@ -1471,88 +1454,7 @@ void CommonMethodOopCache::init(Klass* k, Method* m, TRAPS) {
 }
 
 
-ActiveMethodOopsCache::~ActiveMethodOopsCache() {
-  if (_prev_methods != NULL) {
-    delete _prev_methods;
-    _prev_methods = NULL;
-  }
-}
-
-
-void ActiveMethodOopsCache::add_previous_version(Method* method) {
-  assert(Thread::current()->is_VM_thread(),
-    "only VMThread can add previous versions");
-
-  // Only append the previous method if it is executing on the stack.
-  if (method->on_stack()) {
-
-    if (_prev_methods == NULL) {
-      // This is the first previous version so make some space.
-      // Start with 2 elements under the assumption that the class
-      // won't be redefined much.
-      _prev_methods = new (ResourceObj::C_HEAP, mtClass) GrowableArray<Method*>(2, true);
-    }
-
-    // RC_TRACE macro has an embedded ResourceMark
-    RC_TRACE(0x00000100,
-      ("add: %s(%s): adding prev version ref for cached method @%d",
-        method->name()->as_C_string(), method->signature()->as_C_string(),
-        _prev_methods->length()));
-
-    _prev_methods->append(method);
-  }
-
-
-  // Since the caller is the VMThread and we are at a safepoint, this is a good
-  // time to clear out unused method references.
-
-  if (_prev_methods == NULL) return;
-
-  for (int i = _prev_methods->length() - 1; i >= 0; i--) {
-    Method* method = _prev_methods->at(i);
-    assert(method != NULL, "weak method ref was unexpectedly cleared");
-
-    if (!method->on_stack()) {
-      // This method isn't running anymore so remove it
-      _prev_methods->remove_at(i);
-      MetadataFactory::free_metadata(method->method_holder()->class_loader_data(), method);
-    } else {
-      // RC_TRACE macro has an embedded ResourceMark
-      RC_TRACE(0x00000400,
-        ("add: %s(%s): previous cached method @%d is alive",
-         method->name()->as_C_string(), method->signature()->as_C_string(), i));
-    }
-  }
-} // end add_previous_version()
-
-
-bool ActiveMethodOopsCache::is_same_method(const Method* method) const {
-  InstanceKlass* ik = InstanceKlass::cast(klass());
-  const Method* check_method = ik->method_with_idnum(method_idnum());
-  assert(check_method != NULL, "sanity check");
-  if (check_method == method) {
-    // done with the easy case
-    return true;
-  }
-
-  if (_prev_methods != NULL) {
-    // The cached method has been redefined at least once so search
-    // the previous versions for a match.
-    for (int i = 0; i < _prev_methods->length(); i++) {
-      check_method = _prev_methods->at(i);
-      if (check_method == method) {
-        // a previous version matches
-        return true;
-      }
-    }
-  }
-
-  // either no previous versions or no previous version matched
-  return false;
-}
-
-
-Method* LatestMethodOopCache::get_Method() {
+Method* LatestMethodCache::get_method() {
   if (klass() == NULL) return NULL;
   InstanceKlass* ik = InstanceKlass::cast(klass());
   Method* m = ik->method_with_idnum(method_idnum());

src/share/vm/memory/universe.hpp

@@ -41,10 +41,11 @@ class CollectedHeap;
 class DeferredObjAllocEvent;
 
 
-// Common parts of a Method* cache. This cache safely interacts with
-// the RedefineClasses API.
-//
-class CommonMethodOopCache : public CHeapObj<mtClass> {
+// A helper class for caching a Method* when the user of the cache
+// only cares about the latest version of the Method*.  This cache safely
+// interacts with the RedefineClasses API.
+
+class LatestMethodCache : public CHeapObj<mtClass> {
   // We save the Klass* and the idnum of Method* in order to get
   // the current cached Method*.
  private:
@@ -52,13 +53,15 @@ class CommonMethodOopCache : public CHeapObj<mtClass> {
   int                   _method_idnum;
 
  public:
-  CommonMethodOopCache()   { _klass = NULL; _method_idnum = -1; }
-  ~CommonMethodOopCache()  { _klass = NULL; _method_idnum = -1; }
+  LatestMethodCache()   { _klass = NULL; _method_idnum = -1; }
+  ~LatestMethodCache()  { _klass = NULL; _method_idnum = -1; }
 
-  void     init(Klass* k, Method* m, TRAPS);
+  void   init(Klass* k, Method* m);
   Klass* klass() const           { return _klass; }
   int    method_idnum() const    { return _method_idnum; }
 
+  Method* get_method();
+
   // Enhanced Class Redefinition support
   void classes_do(void f(Klass*)) {
     f(_klass);
@@ -72,39 +75,6 @@ class CommonMethodOopCache : public CHeapObj<mtClass> {
 };
 
 
-// A helper class for caching a Method* when the user of the cache
-// cares about all versions of the Method*.
-//
-class ActiveMethodOopsCache : public CommonMethodOopCache {
-  // This subclass adds weak references to older versions of the
-  // Method* and a query method for a Method*.
-
- private:
-  // If the cached Method* has not been redefined, then
-  // _prev_methods will be NULL. If all of the previous
-  // versions of the method have been collected, then
-  // _prev_methods can have a length of zero.
-  GrowableArray<Method*>* _prev_methods;
-
- public:
-  ActiveMethodOopsCache()   { _prev_methods = NULL; }
-  ~ActiveMethodOopsCache();
-
-  void add_previous_version(Method* method);
-  bool is_same_method(const Method* method) const;
-};
-
-
-// A helper class for caching a Method* when the user of the cache
-// only cares about the latest version of the Method*.
-//
-class LatestMethodOopCache : public CommonMethodOopCache {
-  // This subclass adds a getter method for the latest Method*.
-
- public:
-  Method* get_Method();
-};
-
 // For UseCompressedOops and UseCompressedKlassPointers.
 struct NarrowPtrStruct {
   // Base address for oop/klass-within-java-object materialization.
@@ -174,10 +144,10 @@ class Universe: AllStatic {
   static objArrayOop  _the_empty_class_klass_array;   // Canonicalized obj array of type java.lang.Class
   static oop          _the_null_string;               // A cache of "null" as a Java string
   static oop          _the_min_jint_string;          // A cache of "-2147483648" as a Java string
-  static LatestMethodOopCache* _finalizer_register_cache; // static method for registering finalizable objects
-  static LatestMethodOopCache* _loader_addClass_cache;    // method for registering loaded classes in class loader vector
-  static LatestMethodOopCache* _pd_implies_cache;         // method for checking protection domain attributes
-  static ActiveMethodOopsCache* _reflect_invoke_cache;    // method for security checks
+  static LatestMethodCache* _finalizer_register_cache; // static method for registering finalizable objects
+  static LatestMethodCache* _loader_addClass_cache;    // method for registering loaded classes in class loader vector
+  static LatestMethodCache* _pd_implies_cache;         // method for checking protection domain attributes
+
   // preallocated error objects (no backtrace)
   static oop          _out_of_memory_error_java_heap;
   static oop          _out_of_memory_error_metaspace;
@@ -334,11 +304,11 @@ class Universe: AllStatic {
   static Array<Klass*>* the_array_interfaces_array() { return _the_array_interfaces_array;   }
   static oop          the_null_string()               { return _the_null_string;               }
   static oop          the_min_jint_string()          { return _the_min_jint_string;          }
-  static Method*      finalizer_register_method()     { return _finalizer_register_cache->get_Method(); }
-  static Method*      loader_addClass_method()        { return _loader_addClass_cache->get_Method(); }
 
-  static Method*      protection_domain_implies_method() { return _pd_implies_cache->get_Method(); }
-  static ActiveMethodOopsCache* reflect_invoke_cache() { return _reflect_invoke_cache; }
+  static Method*      finalizer_register_method()     { return _finalizer_register_cache->get_method(); }
+  static Method*      loader_addClass_method()        { return _loader_addClass_cache->get_method(); }
+
+  static Method*      protection_domain_implies_method() { return _pd_implies_cache->get_method(); }
 
   static oop          null_ptr_exception_instance()   { return _null_ptr_exception_instance;   }
   static oop          arithmetic_exception_instance() { return _arithmetic_exception_instance; }

src/share/vm/oops/method.cpp

@@ -981,7 +981,6 @@ bool Method::should_not_be_cached() const {
 bool Method::is_ignored_by_security_stack_walk() const {
   const bool use_new_reflection = JDK_Version::is_gte_jdk14x_version() && UseNewReflection;
 
-  assert(intrinsic_id() != vmIntrinsics::_invoke || Universe::reflect_invoke_cache()->is_same_method((Method*)this), "sanity");
   if (intrinsic_id() == vmIntrinsics::_invoke) {
     // This is Method.invoke() -- ignore it
     return true;

src/share/vm/prims/jvmtiRedefineClasses.cpp

@@ -3217,15 +3217,6 @@ void VM_RedefineClasses::redefine_single_class(jclass the_jclass,
   JvmtiBreakpoints& jvmti_breakpoints = JvmtiCurrentBreakpoints::get_jvmti_breakpoints();
   jvmti_breakpoints.clearall_in_class_at_safepoint(the_class_oop);
 
-  if (the_class_oop == Universe::reflect_invoke_cache()->klass()) {
-    // We are redefining java.lang.reflect.Method. Method.invoke() is
-    // cached and users of the cache care about each active version of
-    // the method so we have to track this previous version.
-    // Do this before methods get switched
-    Universe::reflect_invoke_cache()->add_previous_version(
-      the_class->method_with_idnum(Universe::reflect_invoke_cache()->method_idnum()));
-  }
-
   // Deoptimize all compiled code that depends on this class
   flush_dependent_code(the_class, THREAD);
 

test/runtime/RedefineObject/Agent.java

 /*
- * Copyright (c) 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2013, 2013, 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
@@ -22,6 +22,7 @@
  */
 import java.security.*;
 import java.lang.instrument.*;
+import java.lang.reflect.*;
 
 public class Agent implements ClassFileTransformer {
     public synchronized byte[] transform(final ClassLoader classLoader,
@@ -29,23 +30,35 @@ public class Agent implements ClassFileTransformer {
                                          Class<?> classBeingRedefined,
                                          ProtectionDomain protectionDomain,
                                          byte[] classfileBuffer) {
-        //System.out.println("Transforming class " + className);
+        System.out.println("Transforming class " + className);
         return classfileBuffer;
     }
 
-    public static void premain(String agentArgs, Instrumentation instrumentation) {
-
-        Agent transformer = new Agent();
-
-        instrumentation.addTransformer(transformer, true);
+    public static void redefine(String agentArgs, Instrumentation instrumentation, Class to_redefine) {
 
-        Class c = Object.class;
         try {
-            instrumentation.retransformClasses(c);
+            instrumentation.retransformClasses(to_redefine);
         } catch (Exception e) {
             e.printStackTrace();
         }
 
+    }
+
+    public static void premain(String agentArgs, Instrumentation instrumentation) {
+        Agent transformer = new Agent();
+        instrumentation.addTransformer(transformer, true);
+
+        // Redefine java/lang/Object and java/lang/reflect/Method.invoke and
+        // java/lang/ClassLoader
+        Class object_class = Object.class;
+        redefine(agentArgs, instrumentation, object_class);
+
+        Class method_class = Method.class;
+        redefine(agentArgs, instrumentation, method_class);
+
+        Class loader_class = ClassLoader.class;
+        redefine(agentArgs, instrumentation, loader_class);
+
         instrumentation.removeTransformer(transformer);
     }
 
@@ -57,5 +70,14 @@ public class Agent implements ClassFileTransformer {
             System.gc();
             ba.clone();
         }
+        try {
+            // Use java/lang/reflect/Method.invoke to call
+            WalkThroughInvoke a = new WalkThroughInvoke();
+            Class aclass = WalkThroughInvoke.class;
+            Method m = aclass.getMethod("stackWalk");
+            m.invoke(a);
+        } catch (Exception x) {
+            x.printStackTrace();
+        }
     }
 }

test/runtime/RedefineObject/TestRedefineObject.java

 /*
- * Copyright (c) 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2013, 2013, 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
@@ -26,14 +26,17 @@ import com.oracle.java.testlibrary.*;
 /*
  * Test to redefine java/lang/Object and verify that it doesn't crash on vtable
  * call on basic array type.
+ * Test to redefine java/lang/ClassLoader and java/lang/reflect/Method to make
+ * sure cached versions used afterward are the current version.
  *
  * @test
  * @bug 8005056
+ * @bug 8009728
  * @library /testlibrary
  * @build Agent
  * @run main ClassFileInstaller Agent
  * @run main TestRedefineObject
- * @run main/othervm -javaagent:agent.jar Agent
+ * @run main/othervm -javaagent:agent.jar -XX:TraceRedefineClasses=5 Agent
  */
 public class TestRedefineObject {
     public static void main(String[] args) throws Exception  {

test/runtime/RedefineObject/WalkThroughInvoke.java

+/*
+ * Copyright (c) 2013, 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.
+ */
+import java.lang.reflect.*;
+
+public class WalkThroughInvoke {
+  public void stackWalk() {
+      try {
+          Class b = Object.class;
+          SecurityManager sm = new SecurityManager();
+          // Walks the stack with Method.invoke in the stack (which is the
+          // purpose of the test) before it gets an AccessControlException.
+          sm.checkMemberAccess(b, Member.DECLARED);
+      } catch (java.security.AccessControlException e) {
+          // Ignoring an 'AccessControlException' exception since
+          // it is expected as part of this test.
+      }
+  }
+};