Fix deadlock on dex_lock_ in ClassLinker::RegisterDexFile
Change-Id: I08d6487fe5e00488abace9df5d5224111961788c
diff --git a/src/class_linker.cc b/src/class_linker.cc
index 94011b23..61af24d 100644
--- a/src/class_linker.cc
+++ b/src/class_linker.cc
@@ -197,7 +197,8 @@
}
ClassLinker::ClassLinker(InternTable* intern_table)
- : lock_("ClassLinker lock"),
+ : dex_lock_("ClassLinker dex lock"),
+ classes_lock_("ClassLinker classes lock"),
class_roots_(NULL),
array_interfaces_(NULL),
array_iftable_(NULL),
@@ -549,7 +550,7 @@
}
OatFile* ClassLinker::OpenOat(const Space* space) {
- MutexLock mu(lock_);
+ MutexLock mu(dex_lock_);
const Runtime* runtime = Runtime::Current();
if (runtime->IsVerboseStartup()) {
LOG(INFO) << "ClassLinker::OpenOat entering";
@@ -580,7 +581,7 @@
}
const OatFile* ClassLinker::FindOatFile(const DexFile& dex_file) {
- MutexLock mu(lock_);
+ MutexLock mu(dex_lock_);
// TODO: check if dex_file matches an OatDexFile location and checksum
return FindOatFile(OatFile::DexFileToOatFilename(dex_file));
}
@@ -732,7 +733,7 @@
}
{
- MutexLock mu(lock_);
+ MutexLock mu(classes_lock_);
typedef Table::const_iterator It; // TODO: C++0x auto
for (It it = classes_.begin(), end = classes_.end(); it != end; ++it) {
visitor(it->second, arg);
@@ -1277,7 +1278,7 @@
}
bool ClassLinker::IsDexFileRegisteredLocked(const DexFile& dex_file) const {
- lock_.AssertHeld();
+ dex_lock_.AssertHeld();
for (size_t i = 0; i != dex_files_.size(); ++i) {
if (dex_files_[i] == &dex_file) {
return true;
@@ -1287,12 +1288,12 @@
}
bool ClassLinker::IsDexFileRegistered(const DexFile& dex_file) const {
- MutexLock mu(lock_);
+ MutexLock mu(dex_lock_);
return IsDexFileRegisteredLocked(dex_file);
}
void ClassLinker::RegisterDexFileLocked(const DexFile& dex_file, DexCache* dex_cache) {
- lock_.AssertHeld();
+ dex_lock_.AssertHeld();
CHECK(dex_cache != NULL) << dex_file.GetLocation();
CHECK(dex_cache->GetLocation()->Equals(dex_file.GetLocation()));
dex_files_.push_back(&dex_file);
@@ -1300,21 +1301,33 @@
}
void ClassLinker::RegisterDexFile(const DexFile& dex_file) {
- MutexLock mu(lock_);
- if (IsDexFileRegisteredLocked(dex_file)) {
- return;
+ {
+ MutexLock mu(dex_lock_);
+ if (IsDexFileRegisteredLocked(dex_file)) {
+ return;
+ }
}
- RegisterDexFileLocked(dex_file, AllocDexCache(dex_file));
+ // Don't alloc while holding the lock, since allocation may need to
+ // suspend all threads and another thread may need the dex_lock_ to
+ // get to a suspend point.
+ DexCache* dex_cache = AllocDexCache(dex_file);
+ {
+ MutexLock mu(dex_lock_);
+ if (IsDexFileRegisteredLocked(dex_file)) {
+ return;
+ }
+ RegisterDexFileLocked(dex_file, dex_cache);
+ }
}
void ClassLinker::RegisterDexFile(const DexFile& dex_file, DexCache* dex_cache) {
- MutexLock mu(lock_);
+ MutexLock mu(dex_lock_);
RegisterDexFileLocked(dex_file, dex_cache);
}
const DexFile& ClassLinker::FindDexFile(const DexCache* dex_cache) const {
CHECK(dex_cache != NULL);
- MutexLock mu(lock_);
+ MutexLock mu(dex_lock_);
for (size_t i = 0; i != dex_caches_.size(); ++i) {
if (dex_caches_[i] == dex_cache) {
return *dex_files_[i];
@@ -1325,7 +1338,7 @@
}
DexCache* ClassLinker::FindDexCache(const DexFile& dex_file) const {
- MutexLock mu(lock_);
+ MutexLock mu(dex_lock_);
for (size_t i = 0; i != dex_files_.size(); ++i) {
if (dex_files_[i] == &dex_file) {
return dex_caches_[i];
@@ -1516,14 +1529,14 @@
bool ClassLinker::InsertClass(const std::string& descriptor, Class* klass) {
size_t hash = StringPieceHash()(descriptor);
- MutexLock mu(lock_);
+ MutexLock mu(classes_lock_);
Table::iterator it = classes_.insert(std::make_pair(hash, klass));
return ((*it).second == klass);
}
Class* ClassLinker::LookupClass(const std::string& descriptor, const ClassLoader* class_loader) {
size_t hash = StringPieceHash()(descriptor);
- MutexLock mu(lock_);
+ MutexLock mu(classes_lock_);
typedef Table::const_iterator It; // TODO: C++0x auto
for (It it = classes_.find(hash), end = classes_.end(); it != end; ++it) {
Class* klass = it->second;
@@ -2671,7 +2684,7 @@
// lock held, because it might need to resolve a field's type, which would try to take the lock.
std::vector<Class*> all_classes;
{
- MutexLock mu(lock_);
+ MutexLock mu(classes_lock_);
typedef Table::const_iterator It; // TODO: C++0x auto
for (It it = classes_.begin(), end = classes_.end(); it != end; ++it) {
all_classes.push_back(it->second);
@@ -2684,12 +2697,16 @@
}
size_t ClassLinker::NumLoadedClasses() const {
- MutexLock mu(lock_);
+ MutexLock mu(classes_lock_);
return classes_.size();
}
-pid_t ClassLinker::GetLockOwner() {
- return lock_.GetOwner();
+pid_t ClassLinker::GetClassesLockOwner() {
+ return classes_lock_.GetOwner();
+}
+
+pid_t ClassLinker::GetDexLockOwner() {
+ return dex_lock_.GetOwner();
}
} // namespace art
diff --git a/src/class_linker.h b/src/class_linker.h
index 21944d1..f14b770 100644
--- a/src/class_linker.h
+++ b/src/class_linker.h
@@ -229,7 +229,8 @@
Class* CreateProxyClass(String* name, ObjectArray<Class>* interfaces, ClassLoader* loader,
ObjectArray<Method>* methods, ObjectArray<ObjectArray<Class> >* throws);
- pid_t GetLockOwner(); // For SignalCatcher.
+ pid_t GetClassesLockOwner(); // For SignalCatcher.
+ pid_t GetDexLockOwner(); // For SignalCatcher.
private:
explicit ClassLinker(InternTable*);
@@ -353,21 +354,22 @@
Method* CreateProxyConstructor(Class* klass);
Method* CreateProxyMethod(Class* klass, Method* prototype, ObjectArray<Class>* throws);
- // lock to protect ClassLinker state
- mutable Mutex lock_;
-
- std::vector<const OatFile*> oat_files_;
-
std::vector<const DexFile*> boot_class_path_;
std::vector<const DexFile*> dex_files_;
std::vector<DexCache*> dex_caches_;
+ std::vector<const OatFile*> oat_files_;
+ // lock to protect concurrent access to dex_files_, dex_caches_, and oat_files_
+ mutable Mutex dex_lock_;
+
// multimap from a string hash code of a class descriptor to
// Class* instances. Results should be compared for a matching
// Class::descriptor_ and Class::class_loader_.
+ // Protected by classes_lock_
typedef std::tr1::unordered_multimap<size_t, Class*> Table;
Table classes_;
+ mutable Mutex classes_lock_;
// indexes into class_roots_.
// needs to be kept in sync with class_roots_descriptors_.
diff --git a/src/signal_catcher.cc b/src/signal_catcher.cc
index 22d08b5..a3e7bb1 100644
--- a/src/signal_catcher.cc
+++ b/src/signal_catcher.cc
@@ -69,7 +69,8 @@
LOG(INFO) << "Heap lock owner tid: " << Heap::GetLockOwner() << "\n"
<< "ThreadList lock owner tid: " << thread_list->GetLockOwner() << "\n"
- << "ClassLinker lock owner tid: " << class_linker->GetLockOwner() << "\n";
+ << "ClassLinker classes lock owner tid: " << class_linker->GetClassesLockOwner() << "\n"
+ << "ClassLinker dex lock owner tid: " << class_linker->GetDexLockOwner() << "\n";
thread_list->SuspendAll();