8168914: Crash in ClassLoaderData/JNIHandleBlock::oops_do during concurrent marking
Reviewed-by: dholmes, tschatzl, coleenp, kbarrett, eosterlund, stefank
--- a/hotspot/src/share/vm/classfile/classLoaderData.cpp Fri Feb 24 04:32:11 2017 -0800
+++ b/hotspot/src/share/vm/classfile/classLoaderData.cpp Thu Feb 23 18:57:10 2017 +0100
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2012, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2012, 2017, 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
@@ -93,7 +93,7 @@
_keep_alive((is_anonymous || h_class_loader.is_null()) ? 1 : 0),
_metaspace(NULL), _unloading(false), _klasses(NULL),
_modules(NULL), _packages(NULL),
- _claimed(0), _jmethod_ids(NULL), _handles(NULL), _deallocate_list(NULL),
+ _claimed(0), _jmethod_ids(NULL), _handles(), _deallocate_list(NULL),
_next(NULL), _dependencies(dependencies),
_metaspace_lock(new Mutex(Monitor::leaf+1, "Metaspace allocation lock", true,
Monitor::_safepoint_check_never)) {
@@ -112,6 +112,76 @@
_list_head = oopFactory::new_objectArray(2, CHECK);
}
+ClassLoaderData::ChunkedHandleList::~ChunkedHandleList() {
+ Chunk* c = _head;
+ while (c != NULL) {
+ Chunk* next = c->_next;
+ delete c;
+ c = next;
+ }
+}
+
+oop* ClassLoaderData::ChunkedHandleList::add(oop o) {
+ if (_head == NULL || _head->_size == Chunk::CAPACITY) {
+ Chunk* next = new Chunk(_head);
+ OrderAccess::release_store_ptr(&_head, next);
+ }
+ oop* handle = &_head->_data[_head->_size];
+ *handle = o;
+ OrderAccess::release_store(&_head->_size, _head->_size + 1);
+ return handle;
+}
+
+inline void ClassLoaderData::ChunkedHandleList::oops_do_chunk(OopClosure* f, Chunk* c, const juint size) {
+ for (juint i = 0; i < size; i++) {
+ if (c->_data[i] != NULL) {
+ f->do_oop(&c->_data[i]);
+ }
+ }
+}
+
+void ClassLoaderData::ChunkedHandleList::oops_do(OopClosure* f) {
+ Chunk* head = (Chunk*) OrderAccess::load_ptr_acquire(&_head);
+ if (head != NULL) {
+ // Must be careful when reading size of head
+ oops_do_chunk(f, head, OrderAccess::load_acquire(&head->_size));
+ for (Chunk* c = head->_next; c != NULL; c = c->_next) {
+ oops_do_chunk(f, c, c->_size);
+ }
+ }
+}
+
+#ifdef ASSERT
+class VerifyContainsOopClosure : public OopClosure {
+ oop* _target;
+ bool _found;
+
+ public:
+ VerifyContainsOopClosure(oop* target) : _target(target), _found(false) {}
+
+ void do_oop(oop* p) {
+ if (p == _target) {
+ _found = true;
+ }
+ }
+
+ void do_oop(narrowOop* p) {
+ // The ChunkedHandleList should not contain any narrowOop
+ ShouldNotReachHere();
+ }
+
+ bool found() const {
+ return _found;
+ }
+};
+
+bool ClassLoaderData::ChunkedHandleList::contains(oop* p) {
+ VerifyContainsOopClosure cl(p);
+ oops_do(&cl);
+ return cl.found();
+}
+#endif
+
bool ClassLoaderData::claim() {
if (_claimed == 1) {
return false;
@@ -146,9 +216,9 @@
f->do_oop(&_class_loader);
_dependencies.oops_do(f);
- if (_handles != NULL) {
- _handles->oops_do(f);
- }
+
+ _handles.oops_do(f);
+
if (klass_closure != NULL) {
classes_do(klass_closure);
}
@@ -484,12 +554,6 @@
_metaspace = NULL;
delete m;
}
- // release the handles
- if (_handles != NULL) {
- JNIHandleBlock::release_block(_handles);
- _handles = NULL;
- }
-
// Clear all the JNI handles for methods
// These aren't deallocated and are going to look like a leak, but that's
// needed because we can't really get rid of jmethodIDs because we don't
@@ -563,19 +627,14 @@
return metaspace;
}
-JNIHandleBlock* ClassLoaderData::handles() const { return _handles; }
-void ClassLoaderData::set_handles(JNIHandleBlock* handles) { _handles = handles; }
-
jobject ClassLoaderData::add_handle(Handle h) {
MutexLockerEx ml(metaspace_lock(), Mutex::_no_safepoint_check_flag);
- if (handles() == NULL) {
- set_handles(JNIHandleBlock::allocate_block());
- }
- return handles()->allocate_handle(h());
+ return (jobject) _handles.add(h());
}
-void ClassLoaderData::remove_handle(jobject h) {
- _handles->release_handle(h);
+void ClassLoaderData::remove_handle_unsafe(jobject h) {
+ assert(_handles.contains((oop*) h), "Got unexpected handle " PTR_FORMAT, p2i((oop*) h));
+ *((oop*) h) = NULL;
}
// Add this metadata pointer to be freed when it's safe. This is only during
@@ -645,7 +704,6 @@
p2i(class_loader() != NULL ? class_loader()->klass() : NULL), loader_name());
if (claimed()) out->print(" claimed ");
if (is_unloading()) out->print(" unloading ");
- out->print(" handles " INTPTR_FORMAT, p2i(handles()));
out->cr();
if (metaspace_or_null() != NULL) {
out->print_cr("metaspace: " INTPTR_FORMAT, p2i(metaspace_or_null()));
--- a/hotspot/src/share/vm/classfile/classLoaderData.hpp Fri Feb 24 04:32:11 2017 -0800
+++ b/hotspot/src/share/vm/classfile/classLoaderData.hpp Thu Feb 23 18:57:10 2017 +0100
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2012, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2012, 2017, 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
@@ -52,7 +52,6 @@
class ClassLoaderData;
class JNIMethodBlock;
-class JNIHandleBlock;
class Metadebug;
class ModuleEntry;
class PackageEntry;
@@ -160,6 +159,34 @@
void oops_do(OopClosure* f);
};
+ class ChunkedHandleList VALUE_OBJ_CLASS_SPEC {
+ struct Chunk : public CHeapObj<mtClass> {
+ static const size_t CAPACITY = 32;
+
+ oop _data[CAPACITY];
+ volatile juint _size;
+ Chunk* _next;
+
+ Chunk(Chunk* c) : _next(c), _size(0) { }
+ };
+
+ Chunk* _head;
+
+ void oops_do_chunk(OopClosure* f, Chunk* c, const juint size);
+
+ public:
+ ChunkedHandleList() : _head(NULL) {}
+ ~ChunkedHandleList();
+
+ // Only one thread at a time can add, guarded by ClassLoaderData::metaspace_lock().
+ // However, multiple threads can execute oops_do concurrently with add.
+ oop* add(oop o);
+#ifdef ASSERT
+ bool contains(oop* p);
+#endif
+ void oops_do(OopClosure* f);
+ };
+
friend class ClassLoaderDataGraph;
friend class ClassLoaderDataGraphKlassIteratorAtomic;
friend class ClassLoaderDataGraphMetaspaceIterator;
@@ -185,8 +212,8 @@
volatile int _claimed; // true if claimed, for example during GC traces.
// To avoid applying oop closure more than once.
// Has to be an int because we cas it.
- JNIHandleBlock* _handles; // Handles to constant pool arrays, Modules, etc, which
- // have the same life cycle of the corresponding ClassLoader.
+ ChunkedHandleList _handles; // Handles to constant pool arrays, Modules, etc, which
+ // have the same life cycle of the corresponding ClassLoader.
Klass* volatile _klasses; // The classes defined by the class loader.
PackageEntryTable* volatile _packages; // The packages defined by the class loader.
@@ -217,9 +244,6 @@
ClassLoaderData(Handle h_class_loader, bool is_anonymous, Dependencies dependencies);
~ClassLoaderData();
- JNIHandleBlock* handles() const;
- void set_handles(JNIHandleBlock* handles);
-
// GC interface.
void clear_claimed() { _claimed = 0; }
bool claimed() const { return _claimed == 1; }
@@ -312,7 +336,7 @@
const char* loader_name();
jobject add_handle(Handle h);
- void remove_handle(jobject h);
+ void remove_handle_unsafe(jobject h);
void add_class(Klass* k, bool publicize = true);
void remove_class(Klass* k);
bool contains_klass(Klass* k);
--- a/hotspot/src/share/vm/classfile/moduleEntry.cpp Fri Feb 24 04:32:11 2017 -0800
+++ b/hotspot/src/share/vm/classfile/moduleEntry.cpp Thu Feb 23 18:57:10 2017 +0100
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2017, 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
@@ -87,11 +87,11 @@
// Set the shared ProtectionDomain atomically
void ModuleEntry::set_shared_protection_domain(ClassLoaderData *loader_data,
Handle pd_h) {
- // Create a JNI handle for the shared ProtectionDomain and save it atomically.
- // If someone beats us setting the _pd cache, the created JNI handle is destroyed.
+ // Create a handle for the shared ProtectionDomain and save it atomically.
+ // If someone beats us setting the _pd cache, the created handle is destroyed.
jobject obj = loader_data->add_handle(pd_h);
if (Atomic::cmpxchg_ptr(obj, &_pd, NULL) != NULL) {
- loader_data->remove_handle(obj);
+ loader_data->remove_handle_unsafe(obj);
}
}