8249822: SymbolPropertyTable creates an extra OopHandle per entry

Add an assert to OopHandle assigment operator to catch leaking OopHandles, and fix code accordingly.

Reviewed-by: sspitsyn, eosterlund, dholmes

src/hotspot/share/classfile/dictionary.hpp

@@ -195,6 +195,9 @@ class SymbolPropertyEntry : public HashtableEntry<Symbol*, mtSymbol> {
   oop      method_type() const;
   void set_method_type(oop p);
 
+  // We need to clear the OopHandle because these hashtable entries are not constructed properly.
+  void clear_method_type() { _method_type = OopHandle(); }
+
   void free_entry();
 
   SymbolPropertyEntry* next() const {
@@ -247,7 +250,7 @@ private:
     symbol->increment_refcount();
     entry->set_symbol_mode(symbol_mode);
     entry->set_method(NULL);
-    entry->set_method_type(NULL);
+    entry->clear_method_type();
     return entry;
   }
 

src/hotspot/share/classfile/javaClasses.cpp

@@ -895,7 +895,7 @@ void java_lang_Class::fixup_mirror(Klass* k, TRAPS) {
       assert(present, "Missing archived mirror for %s", k->external_name());
       return;
     } else {
-      k->set_java_mirror_handle(OopHandle());
+      k->clear_java_mirror_handle();
       k->clear_has_raw_archived_mirror();
     }
   }
@@ -1201,7 +1201,7 @@ oop java_lang_Class::archive_mirror(Klass* k, TRAPS) {
           ik->is_shared_app_class())) {
       // Archiving mirror for classes from non-builtin loaders is not
       // supported. Clear the _java_mirror within the archived class.
-      k->set_java_mirror_handle(OopHandle());
+      k->clear_java_mirror_handle();
       return NULL;
     }
   }

src/hotspot/share/oops/klass.cpp

@@ -53,7 +53,7 @@
 
 void Klass::set_java_mirror(Handle m) {
   assert(!m.is_null(), "New mirror should never be null.");
-  assert(_java_mirror.resolve() == NULL, "should only be used to initialize mirror");
+  assert(_java_mirror.is_empty(), "should only be used to initialize mirror");
   _java_mirror = class_loader_data()->add_handle(m);
 }
 
@@ -61,6 +61,10 @@ oop Klass::java_mirror_no_keepalive() const {
   return _java_mirror.peek();
 }
 
+void Klass::replace_java_mirror(oop mirror) {
+  _java_mirror.replace(mirror);
+}
+
 bool Klass::is_cloneable() const {
   return _access_flags.is_cloneable_fast() ||
          is_subtype_of(SystemDictionary::Cloneable_klass());
@@ -195,10 +199,7 @@ void* Klass::operator new(size_t size, ClassLoaderData* loader_data, size_t word
 // which zeros out memory - calloc equivalent.
 // The constructor is also used from CppVtableCloner,
 // which doesn't zero out the memory before calling the constructor.
-// Need to set the _java_mirror field explicitly to not hit an assert that the field
-// should be NULL before setting it.
 Klass::Klass(KlassID id) : _id(id),
-                           _java_mirror(NULL),
                            _prototype_header(markWord::prototype()),
                            _shared_class_path_index(-1) {
   CDS_ONLY(_shared_class_flags = 0;)
@@ -555,7 +556,7 @@ void Klass::remove_java_mirror() {
     log_trace(cds, unshareable)("remove java_mirror: %s", external_name());
   }
   // Just null out the mirror.  The class_loader_data() no longer exists.
-  _java_mirror = OopHandle();
+  clear_java_mirror_handle();
 }
 
 void Klass::restore_unshareable_info(ClassLoaderData* loader_data, Handle protection_domain, TRAPS) {
@@ -609,7 +610,7 @@ void Klass::restore_unshareable_info(ClassLoaderData* loader_data, Handle protec
 
     // No archived mirror data
     log_debug(cds, mirror)("No archived mirror data for %s", external_name());
-    _java_mirror = OopHandle();
+    clear_java_mirror_handle();
     this->clear_has_raw_archived_mirror();
   }
 

src/hotspot/share/oops/klass.hpp

@@ -267,12 +267,11 @@ protected:
   void set_archived_java_mirror_raw(oop m) NOT_CDS_JAVA_HEAP_RETURN; // no GC barrier
 
   // Temporary mirror switch used by RedefineClasses
-  // Both mirrors are on the ClassLoaderData::_handles list already so no
-  // barriers are needed.
-  void set_java_mirror_handle(OopHandle mirror) { _java_mirror = mirror; }
-  OopHandle java_mirror_handle() const          {
-    return _java_mirror;
-  }
+  void replace_java_mirror(oop mirror);
+
+  // Set java mirror OopHandle to NULL for CDS
+  // This leaves the OopHandle in the CLD, but that's ok, you can't release them.
+  void clear_java_mirror_handle() { _java_mirror = OopHandle(); }
 
   // modifier flags
   jint modifier_flags() const          { return _modifier_flags; }

src/hotspot/share/oops/oopHandle.hpp

@@ -45,6 +45,16 @@ public:
   explicit OopHandle(oop* w) : _obj(w) {}
   OopHandle(OopStorage* storage, oop obj);
 
+  OopHandle(const OopHandle& copy) : _obj(copy._obj) {}
+
+  OopHandle& operator=(const OopHandle& copy) {
+    // Allow "this" to be junk if copy is empty; needed by initialization of
+    // raw memory in hashtables.
+    assert(is_empty() || copy.is_empty(), "can only copy if empty");
+    _obj = copy._obj;
+    return *this;
+  }
+
   inline oop resolve() const;
   inline oop peek() const;
 
@@ -52,6 +62,8 @@ public:
 
   inline void release(OopStorage* storage);
 
+  inline void replace(oop obj);
+
   // Used only for removing handle.
   oop* ptr_raw() const { return _obj; }
 };

src/hotspot/share/oops/oopHandle.inline.hpp

@@ -54,4 +54,10 @@ inline void OopHandle::release(OopStorage* storage) {
   }
 }
 
+inline void OopHandle::replace(oop obj) {
+  oop* ptr = ptr_raw();
+  assert(ptr != NULL, "should not use replace");
+  NativeAccess<>::oop_store(ptr, obj);
+}
+
 #endif // SHARE_OOPS_OOPHANDLE_INLINE_HPP

src/hotspot/share/prims/jvmtiRedefineClasses.cpp

@@ -1221,6 +1221,44 @@ bool VM_RedefineClasses::is_unresolved_class_mismatch(const constantPoolHandle&
 } // end is_unresolved_class_mismatch()
 
 
+// The bug 6214132 caused the verification to fail.
+// 1. What's done in RedefineClasses() before verification:
+//  a) A reference to the class being redefined (_the_class) and a
+//     reference to new version of the class (_scratch_class) are
+//     saved here for use during the bytecode verification phase of
+//     RedefineClasses.
+//  b) The _java_mirror field from _the_class is copied to the
+//     _java_mirror field in _scratch_class. This means that a jclass
+//     returned for _the_class or _scratch_class will refer to the
+//     same Java mirror. The verifier will see the "one true mirror"
+//     for the class being verified.
+// 2. See comments in JvmtiThreadState for what is done during verification.
+
+class RedefineVerifyMark : public StackObj {
+ private:
+  JvmtiThreadState* _state;
+  Klass*            _scratch_class;
+  Handle            _scratch_mirror;
+
+ public:
+
+  RedefineVerifyMark(Klass* the_class, Klass* scratch_class,
+                     JvmtiThreadState* state) : _state(state), _scratch_class(scratch_class)
+  {
+    _state->set_class_versions_map(the_class, scratch_class);
+    _scratch_mirror = Handle(_state->get_thread(), _scratch_class->java_mirror());
+    _scratch_class->replace_java_mirror(the_class->java_mirror());
+  }
+
+  ~RedefineVerifyMark() {
+    // Restore the scratch class's mirror, so when scratch_class is removed
+    // the correct mirror pointing to it can be cleared.
+    _scratch_class->replace_java_mirror(_scratch_mirror());
+    _state->clear_class_versions_map();
+  }
+};
+
+
 jvmtiError VM_RedefineClasses::load_new_class_versions(TRAPS) {
 
   // For consistency allocate memory using os::malloc wrapper.

src/hotspot/share/prims/jvmtiThreadState.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 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
@@ -247,19 +247,8 @@ class JvmtiThreadState : public CHeapObj<mtInternal> {
   // RedefineClasses support
   // The bug 6214132 caused the verification to fail.
   //
-  // Below is the detailed description of the fix approach taken:
-  // 1. What's done in RedefineClasses() before verification:
-  //  a) A reference to the class being redefined (_the_class) and a
-  //     reference to new version of the class (_scratch_class) are
-  //     saved here for use during the bytecode verification phase of
-  //     RedefineClasses. See RedefineVerifyMark for how these fields
-  //     are managed.
-  //   b) The _java_mirror field from _the_class is copied to the
-  //     _java_mirror field in _scratch_class. This means that a jclass
-  //     returned for _the_class or _scratch_class will refer to the
-  //     same Java mirror. The verifier will see the "one true mirror"
-  //     for the class being verified.
-  // 2. What is done at verification:
+  // What is done at verification:
+  //   (This seems to only apply to the old verifier.)
   //   When the verifier makes calls into the VM to ask questions about
   //   the class being verified, it will pass the jclass to JVM_* functions.
   //   The jclass is always pointing to the mirror of _the_class.
@@ -401,27 +390,4 @@ public:
   void run_nmethod_entry_barriers();
 };
 
-class RedefineVerifyMark : public StackObj {
- private:
-  JvmtiThreadState* _state;
-  Klass*            _scratch_class;
-  OopHandle         _scratch_mirror;
-
- public:
-  RedefineVerifyMark(Klass* the_class, Klass* scratch_class,
-                     JvmtiThreadState *state) : _state(state), _scratch_class(scratch_class)
-  {
-    _state->set_class_versions_map(the_class, scratch_class);
-    _scratch_mirror = _scratch_class->java_mirror_handle();
-    _scratch_class->set_java_mirror_handle(the_class->java_mirror_handle());
-  }
-
-  ~RedefineVerifyMark() {
-    // Restore the scratch class's mirror, so when scratch_class is removed
-    // the correct mirror pointing to it can be cleared.
-    _scratch_class->set_java_mirror_handle(_scratch_mirror);
-    _state->clear_class_versions_map();
-  }
-};
-
 #endif // SHARE_PRIMS_JVMTITHREADSTATE_HPP