8042235: redefining method used by multiple MethodHandles crashes VM

Summary: note all MemberNames created on internal list for adjusting method entries.
Reviewed-by: sspitsyn, dcubed, lfoltan

7 files changed, 90 insertions, 85 deletions

diff --git a/src/share/vm/classfile/javaClasses.cpp b/src/share/vm/classfile/javaClasses.cpp
index 525fa1f0c..d5869a757 100644
--- a/src/share/vm/classfile/javaClasses.cpp
+++ b/src/share/vm/classfile/javaClasses.cpp
@@ -41,6 +41,7 @@
 #include "oops/method.hpp"
 #include "oops/symbol.hpp"
 #include "oops/typeArrayOop.hpp"
+#include "prims/jvmtiRedefineClassesTrace.hpp"
 #include "runtime/fieldDescriptor.hpp"
 #include "runtime/handles.inline.hpp"
 #include "runtime/interfaceSupport.hpp"
@@ -2775,12 +2776,35 @@ Metadata* java_lang_invoke_MemberName::vmtarget(oop mname) {
   return (Metadata*)mname->address_field(_vmtarget_offset);
 }
 
+bool java_lang_invoke_MemberName::is_method(oop mname) {
+  assert(is_instance(mname), "must be MemberName");
+  return (flags(mname) & (MN_IS_METHOD | MN_IS_CONSTRUCTOR)) > 0;
+}
+
 #if INCLUDE_JVMTI
 // Can be executed on VM thread only
-void java_lang_invoke_MemberName::adjust_vmtarget(oop mname, Metadata* ref) {
-  assert((is_instance(mname) && (flags(mname) & (MN_IS_METHOD | MN_IS_CONSTRUCTOR)) > 0), "wrong type");
+void java_lang_invoke_MemberName::adjust_vmtarget(oop mname, Method* old_method,
+                                                  Method* new_method, bool* trace_name_printed) {
+  assert(is_method(mname), "wrong type");
   assert(Thread::current()->is_VM_thread(), "not VM thread");
-  mname->address_field_put(_vmtarget_offset, (address)ref);
+
+  Method* target = (Method*)mname->address_field(_vmtarget_offset);
+  if (target == old_method) {
+    mname->address_field_put(_vmtarget_offset, (address)new_method);
+
+    if (RC_TRACE_IN_RANGE(0x00100000, 0x00400000)) {
+      if (!(*trace_name_printed)) {
+        // RC_TRACE_MESG macro has an embedded ResourceMark
+        RC_TRACE_MESG(("adjust: name=%s",
+                       old_method->method_holder()->external_name()));
+        *trace_name_printed = true;
+      }
+      // RC_TRACE macro has an embedded ResourceMark
+      RC_TRACE(0x00400000, ("MemberName method update: %s(%s)",
+                            new_method->name()->as_C_string(),
+                            new_method->signature()->as_C_string()));
+    }
+  }
 }
 #endif // INCLUDE_JVMTI
 
diff --git a/src/share/vm/classfile/javaClasses.hpp b/src/share/vm/classfile/javaClasses.hpp
index d641f175e..7b041ee22 100644
--- a/src/share/vm/classfile/javaClasses.hpp
+++ b/src/share/vm/classfile/javaClasses.hpp
@@ -1096,7 +1096,8 @@ class java_lang_invoke_MemberName: AllStatic {
   static Metadata*      vmtarget(oop mname);
   static void       set_vmtarget(oop mname, Metadata* target);
 #if INCLUDE_JVMTI
-  static void       adjust_vmtarget(oop mname, Metadata* target);
+  static void       adjust_vmtarget(oop mname, Method* old_method, Method* new_method,
+                                    bool* trace_name_printed);
 #endif // INCLUDE_JVMTI
 
   static intptr_t       vmindex(oop mname);
@@ -1110,6 +1111,8 @@ class java_lang_invoke_MemberName: AllStatic {
     return obj != NULL && is_subclass(obj->klass());
   }
 
+  static bool is_method(oop obj);
+
   // Relevant integer codes (keep these in synch. with MethodHandleNatives.Constants):
   enum {
     MN_IS_METHOD            = 0x00010000, // method (not constructor)
diff --git a/src/share/vm/oops/instanceKlass.cpp b/src/share/vm/oops/instanceKlass.cpp
index 6805f62a6..707826497 100644
--- a/src/share/vm/oops/instanceKlass.cpp
+++ b/src/share/vm/oops/instanceKlass.cpp
@@ -2947,28 +2947,27 @@ nmethod* InstanceKlass::lookup_osr_nmethod(const Method* m, int bci, int comp_le
   return NULL;
 }
 
-void InstanceKlass::add_member_name(int index, Handle mem_name) {
+bool InstanceKlass::add_member_name(Handle mem_name) {
   jweak mem_name_wref = JNIHandles::make_weak_global(mem_name);
   MutexLocker ml(MemberNameTable_lock);
-  assert(0 <= index && index < idnum_allocated_count(), "index is out of bounds");
   DEBUG_ONLY(No_Safepoint_Verifier nsv);
 
-  if (_member_names == NULL) {
-    _member_names = new (ResourceObj::C_HEAP, mtClass) MemberNameTable(idnum_allocated_count());
+  // Check if method has been redefined while taking out MemberNameTable_lock, if so
+  // return false.  We cannot cache obsolete methods. They will crash when the function
+  // is called!
+  Method* method = (Method*)java_lang_invoke_MemberName::vmtarget(mem_name());
+  if (method->is_obsolete()) {
+    return false;
+  } else if (method->is_old()) {
+    // Replace method with redefined version
+    java_lang_invoke_MemberName::set_vmtarget(mem_name(), method_with_idnum(method->method_idnum()));
   }
-  _member_names->add_member_name(index, mem_name_wref);
-}
-
-oop InstanceKlass::get_member_name(int index) {
-  MutexLocker ml(MemberNameTable_lock);
-  assert(0 <= index && index < idnum_allocated_count(), "index is out of bounds");
-  DEBUG_ONLY(No_Safepoint_Verifier nsv);
 
   if (_member_names == NULL) {
-    return NULL;
+    _member_names = new (ResourceObj::C_HEAP, mtClass) MemberNameTable(idnum_allocated_count());
   }
-  oop mem_name =_member_names->get_member_name(index);
-  return mem_name;
+  _member_names->add_member_name(mem_name_wref);
+  return true;
 }
 
 // -----------------------------------------------------------------------------------------------------
diff --git a/src/share/vm/oops/instanceKlass.hpp b/src/share/vm/oops/instanceKlass.hpp
index 96e7fe3c3..1cd3c0d33 100644
--- a/src/share/vm/oops/instanceKlass.hpp
+++ b/src/share/vm/oops/instanceKlass.hpp
@@ -1091,8 +1091,7 @@ public:
   // JSR-292 support
   MemberNameTable* member_names() { return _member_names; }
   void set_member_names(MemberNameTable* member_names) { _member_names = member_names; }
-  void add_member_name(int index, Handle member_name);
-  oop  get_member_name(int index);
+  bool add_member_name(Handle member_name);
 
 public:
   // JVMTI support
diff --git a/src/share/vm/prims/jvm.cpp b/src/share/vm/prims/jvm.cpp
index 070bf88a5..c39b10252 100644
--- a/src/share/vm/prims/jvm.cpp
+++ b/src/share/vm/prims/jvm.cpp
@@ -603,13 +603,14 @@ JVM_ENTRY(jobject, JVM_Clone(JNIEnv* env, jobject handle))
 
   // Make shallow object copy
   const int size = obj->size();
-  oop new_obj = NULL;
+  oop new_obj_oop = NULL;
   if (obj->is_array()) {
     const int length = ((arrayOop)obj())->length();
-    new_obj = CollectedHeap::array_allocate(klass, size, length, CHECK_NULL);
+    new_obj_oop = CollectedHeap::array_allocate(klass, size, length, CHECK_NULL);
   } else {
-    new_obj = CollectedHeap::obj_allocate(klass, size, CHECK_NULL);
+    new_obj_oop = CollectedHeap::obj_allocate(klass, size, CHECK_NULL);
   }
+
   // 4839641 (4840070): We must do an oop-atomic copy, because if another thread
   // is modifying a reference field in the clonee, a non-oop-atomic copy might
   // be suspended in the middle of copying the pointer and end up with parts
@@ -620,24 +621,41 @@ JVM_ENTRY(jobject, JVM_Clone(JNIEnv* env, jobject handle))
   // The same is true of StubRoutines::object_copy and the various oop_copy
   // variants, and of the code generated by the inline_native_clone intrinsic.
   assert(MinObjAlignmentInBytes >= BytesPerLong, "objects misaligned");
-  Copy::conjoint_jlongs_atomic((jlong*)obj(), (jlong*)new_obj,
+  Copy::conjoint_jlongs_atomic((jlong*)obj(), (jlong*)new_obj_oop,
                                (size_t)align_object_size(size) / HeapWordsPerLong);
   // Clear the header
-  new_obj->init_mark();
+  new_obj_oop->init_mark();
 
   // Store check (mark entire object and let gc sort it out)
   BarrierSet* bs = Universe::heap()->barrier_set();
   assert(bs->has_write_region_opt(), "Barrier set does not have write_region");
-  bs->write_region(MemRegion((HeapWord*)new_obj, size));
+  bs->write_region(MemRegion((HeapWord*)new_obj_oop, size));
+
+  Handle new_obj(THREAD, new_obj_oop);
+  // Special handling for MemberNames.  Since they contain Method* metadata, they
+  // must be registered so that RedefineClasses can fix metadata contained in them.
+  if (java_lang_invoke_MemberName::is_instance(new_obj()) &&
+      java_lang_invoke_MemberName::is_method(new_obj())) {
+    Method* method = (Method*)java_lang_invoke_MemberName::vmtarget(new_obj());
+    // MemberName may be unresolved, so doesn't need registration until resolved.
+    if (method != NULL) {
+      methodHandle m(THREAD, method);
+      // This can safepoint and redefine method, so need both new_obj and method
+      // in a handle, for two different reasons.  new_obj can move, method can be
+      // deleted if nothing is using it on the stack.
+      m->method_holder()->add_member_name(new_obj());
+    }
+  }
 
   // Caution: this involves a java upcall, so the clone should be
   // "gc-robust" by this stage.
   if (klass->has_finalizer()) {
     assert(obj->is_instance(), "should be instanceOop");
-    new_obj = InstanceKlass::register_finalizer(instanceOop(new_obj), CHECK_NULL);
+    new_obj_oop = InstanceKlass::register_finalizer(instanceOop(new_obj()), CHECK_NULL);
+    new_obj = Handle(THREAD, new_obj_oop);
   }
 
-  return JNIHandles::make_local(env, oop(new_obj));
+  return JNIHandles::make_local(env, new_obj());
 JVM_END
 
 // java.lang.Compiler ////////////////////////////////////////////////////
diff --git a/src/share/vm/prims/methodHandles.cpp b/src/share/vm/prims/methodHandles.cpp
index 0fa73313f..7c95d515f 100644
--- a/src/share/vm/prims/methodHandles.cpp
+++ b/src/share/vm/prims/methodHandles.cpp
@@ -29,7 +29,6 @@
 #include "interpreter/oopMapCache.hpp"
 #include "memory/allocation.inline.hpp"
 #include "memory/oopFactory.hpp"
-#include "prims/jvmtiRedefineClassesTrace.hpp"
 #include "prims/methodHandles.hpp"
 #include "runtime/compilationPolicy.hpp"
 #include "runtime/javaCalls.hpp"
@@ -271,9 +270,12 @@ oop MethodHandles::init_method_MemberName(Handle mname, CallInfo& info) {
   // This is done eagerly, since it is readily available without
   // constructing any new objects.
   // TO DO: maybe intern mname_oop
-  m->method_holder()->add_member_name(m->method_idnum(), mname);
-
-  return mname();
+  if (m->method_holder()->add_member_name(mname)) {
+    return mname();
+  } else {
+    // Redefinition caused this to fail.  Return NULL (and an exception?)
+    return NULL;
+  }
 }
 
 oop MethodHandles::init_field_MemberName(Handle mname, fieldDescriptor& fd, bool is_setter) {
@@ -946,63 +948,27 @@ MemberNameTable::~MemberNameTable() {
   }
 }
 
-void MemberNameTable::add_member_name(int index, jweak mem_name_wref) {
-  assert_locked_or_safepoint(MemberNameTable_lock);
-  this->at_put_grow(index, mem_name_wref);
-}
-
-// Return a member name oop or NULL.
-oop MemberNameTable::get_member_name(int index) {
+void MemberNameTable::add_member_name(jweak mem_name_wref) {
   assert_locked_or_safepoint(MemberNameTable_lock);
-
-  jweak ref = this->at(index);
-  oop mem_name = JNIHandles::resolve(ref);
-  return mem_name;
+  this->push(mem_name_wref);
 }
 
 #if INCLUDE_JVMTI
-oop MemberNameTable::find_member_name_by_method(Method* old_method) {
-  assert_locked_or_safepoint(MemberNameTable_lock);
-  oop found = NULL;
-  int len = this->length();
-
-  for (int idx = 0; idx < len; idx++) {
-    oop mem_name = JNIHandles::resolve(this->at(idx));
-    if (mem_name == NULL) {
-      continue;
-    }
-    Method* method = (Method*)java_lang_invoke_MemberName::vmtarget(mem_name);
-    if (method == old_method) {
-      found = mem_name;
-      break;
-    }
-  }
-  return found;
-}
-
-// It is called at safepoint only
+// It is called at safepoint only for RedefineClasses
 void MemberNameTable::adjust_method_entries(Method** old_methods, Method** new_methods,
                                             int methods_length, bool *trace_name_printed) {
   assert(SafepointSynchronize::is_at_safepoint(), "only called at safepoint");
-  // search the MemberNameTable for uses of either obsolete or EMCP methods
+  // For each redefined method
   for (int j = 0; j < methods_length; j++) {
     Method* old_method = old_methods[j];
     Method* new_method = new_methods[j];
-    oop mem_name = find_member_name_by_method(old_method);
-    if (mem_name != NULL) {
-      java_lang_invoke_MemberName::adjust_vmtarget(mem_name, new_method);
-
-      if (RC_TRACE_IN_RANGE(0x00100000, 0x00400000)) {
-        if (!(*trace_name_printed)) {
-          // RC_TRACE_MESG macro has an embedded ResourceMark
-          RC_TRACE_MESG(("adjust: name=%s",
-                         old_method->method_holder()->external_name()));
-          *trace_name_printed = true;
-        }
-        // RC_TRACE macro has an embedded ResourceMark
-        RC_TRACE(0x00400000, ("MemberName method update: %s(%s)",
-                              new_method->name()->as_C_string(),
-                              new_method->signature()->as_C_string()));
+
+    // search the MemberNameTable for uses of either obsolete or EMCP methods
+    for (int idx = 0; idx < length(); idx++) {
+      oop mem_name = JNIHandles::resolve(this->at(idx));
+      if (mem_name != NULL) {
+        java_lang_invoke_MemberName::adjust_vmtarget(mem_name, old_method, new_method,
+                                                     trace_name_printed);
       }
     }
   }
diff --git a/src/share/vm/prims/methodHandles.hpp b/src/share/vm/prims/methodHandles.hpp
index 323cd8236..9caf8004e 100644
--- a/src/share/vm/prims/methodHandles.hpp
+++ b/src/share/vm/prims/methodHandles.hpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2008, 2014, 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
@@ -236,18 +236,14 @@ class MemberNameTable : public GrowableArray<jweak> {
  public:
   MemberNameTable(int methods_cnt);
   ~MemberNameTable();
-  void add_member_name(int index, jweak mem_name_ref);
-  oop  get_member_name(int index);
+  void add_member_name(jweak mem_name_ref);
 
 #if INCLUDE_JVMTI
- public:
   // RedefineClasses() API support:
   // If a MemberName refers to old_method then update it
   // to refer to new_method.
   void adjust_method_entries(Method** old_methods, Method** new_methods,
                              int methods_length, bool *trace_name_printed);
- private:
-  oop find_member_name_by_method(Method* old_method);
 #endif // INCLUDE_JVMTI
 };