Hold dex caches live in class table
Prevents temporary dex caches being unloaded for the same dex file.
Usually this is OK, but if someone resolved a string in that dex
cache, it could leave stale pointers in BSS. Also it can use extra
memory in linear alloc if we allocate dex cache arrays multiple
times.
Bug: 29083330
Change-Id: Ia44668f013ceef1f5eb80f653a48d0f8004548c9
diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index 9e144dd..e789db6 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -2469,9 +2469,7 @@
self->AssertPendingOOMException();
return nullptr;
}
- mirror::DexCache* dex_cache = RegisterDexFile(
- dex_file,
- GetOrCreateAllocatorForClassLoader(class_loader.Get()));
+ mirror::DexCache* dex_cache = RegisterDexFile(dex_file, class_loader.Get());
if (dex_cache == nullptr) {
self->AssertPendingOOMException();
return nullptr;
@@ -3230,7 +3228,8 @@
dex_caches_.push_back(data);
}
-mirror::DexCache* ClassLinker::RegisterDexFile(const DexFile& dex_file, LinearAlloc* linear_alloc) {
+mirror::DexCache* ClassLinker::RegisterDexFile(const DexFile& dex_file,
+ mirror::ClassLoader* class_loader) {
Thread* self = Thread::Current();
{
ReaderMutexLock mu(self, dex_lock_);
@@ -3239,21 +3238,31 @@
return dex_cache;
}
}
+ LinearAlloc* const linear_alloc = GetOrCreateAllocatorForClassLoader(class_loader);
+ DCHECK(linear_alloc != nullptr);
+ ClassTable* table;
+ {
+ WriterMutexLock mu(self, *Locks::classlinker_classes_lock_);
+ table = InsertClassTableForClassLoader(class_loader);
+ }
// 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.
StackHandleScope<1> hs(self);
Handle<mirror::DexCache> h_dex_cache(hs.NewHandle(AllocDexCache(self, dex_file, linear_alloc)));
- WriterMutexLock mu(self, dex_lock_);
- mirror::DexCache* dex_cache = FindDexCacheLocked(self, dex_file, true);
- if (dex_cache != nullptr) {
- return dex_cache;
+ {
+ WriterMutexLock mu(self, dex_lock_);
+ mirror::DexCache* dex_cache = FindDexCacheLocked(self, dex_file, true);
+ if (dex_cache != nullptr) {
+ return dex_cache;
+ }
+ if (h_dex_cache.Get() == nullptr) {
+ self->AssertPendingOOMException();
+ return nullptr;
+ }
+ RegisterDexFileLocked(dex_file, h_dex_cache);
}
- if (h_dex_cache.Get() == nullptr) {
- self->AssertPendingOOMException();
- return nullptr;
- }
- RegisterDexFileLocked(dex_file, h_dex_cache);
+ table->InsertStrongRoot(h_dex_cache.Get());
return h_dex_cache.Get();
}
@@ -7991,7 +8000,7 @@
WriterMutexLock mu(self, *Locks::classlinker_classes_lock_);
ClassTable* const table = ClassTableForClassLoader(class_loader);
DCHECK(table != nullptr);
- if (table->InsertDexFile(dex_file) && class_loader != nullptr) {
+ if (table->InsertStrongRoot(dex_file) && class_loader != nullptr) {
// It was not already inserted, perform the write barrier to let the GC know the class loader's
// class table was modified.
Runtime::Current()->GetHeap()->WriteBarrierEveryFieldOf(class_loader);
diff --git a/runtime/class_linker.h b/runtime/class_linker.h
index f6ce545..17b87cd 100644
--- a/runtime/class_linker.h
+++ b/runtime/class_linker.h
@@ -377,7 +377,8 @@
SHARED_REQUIRES(Locks::mutator_lock_)
REQUIRES(!dex_lock_, !Roles::uninterruptible_);
- mirror::DexCache* RegisterDexFile(const DexFile& dex_file, LinearAlloc* linear_alloc)
+ mirror::DexCache* RegisterDexFile(const DexFile& dex_file,
+ mirror::ClassLoader* class_loader)
REQUIRES(!dex_lock_)
SHARED_REQUIRES(Locks::mutator_lock_);
void RegisterDexFile(const DexFile& dex_file, Handle<mirror::DexCache> dex_cache)
diff --git a/runtime/class_table-inl.h b/runtime/class_table-inl.h
index 42e320a..d52365d 100644
--- a/runtime/class_table-inl.h
+++ b/runtime/class_table-inl.h
@@ -29,7 +29,7 @@
visitor.VisitRoot(root.AddressWithoutBarrier());
}
}
- for (GcRoot<mirror::Object>& root : dex_files_) {
+ for (GcRoot<mirror::Object>& root : strong_roots_) {
visitor.VisitRoot(root.AddressWithoutBarrier());
}
}
@@ -42,7 +42,7 @@
visitor.VisitRoot(root.AddressWithoutBarrier());
}
}
- for (GcRoot<mirror::Object>& root : dex_files_) {
+ for (GcRoot<mirror::Object>& root : strong_roots_) {
visitor.VisitRoot(root.AddressWithoutBarrier());
}
}
diff --git a/runtime/class_table.cc b/runtime/class_table.cc
index 8267c68..f5ebcc5 100644
--- a/runtime/class_table.cc
+++ b/runtime/class_table.cc
@@ -146,15 +146,15 @@
return ComputeModifiedUtf8Hash(descriptor);
}
-bool ClassTable::InsertDexFile(mirror::Object* dex_file) {
+bool ClassTable::InsertStrongRoot(mirror::Object* obj) {
WriterMutexLock mu(Thread::Current(), lock_);
- DCHECK(dex_file != nullptr);
- for (GcRoot<mirror::Object>& root : dex_files_) {
- if (root.Read() == dex_file) {
+ DCHECK(obj != nullptr);
+ for (GcRoot<mirror::Object>& root : strong_roots_) {
+ if (root.Read() == obj) {
return false;
}
}
- dex_files_.push_back(GcRoot<mirror::Object>(dex_file));
+ strong_roots_.push_back(GcRoot<mirror::Object>(obj));
return true;
}
diff --git a/runtime/class_table.h b/runtime/class_table.h
index 686381d..854a85e 100644
--- a/runtime/class_table.h
+++ b/runtime/class_table.h
@@ -133,8 +133,8 @@
REQUIRES(!lock_)
SHARED_REQUIRES(Locks::mutator_lock_);
- // Return true if we inserted the dex file, false if it already exists.
- bool InsertDexFile(mirror::Object* dex_file)
+ // Return true if we inserted the strong root, false if it already exists.
+ bool InsertStrongRoot(mirror::Object* obj)
REQUIRES(!lock_)
SHARED_REQUIRES(Locks::mutator_lock_);
@@ -162,9 +162,10 @@
mutable ReaderWriterMutex lock_;
// We have a vector to help prevent dirty pages after the zygote forks by calling FreezeSnapshot.
std::vector<ClassSet> classes_ GUARDED_BY(lock_);
- // Dex files used by the class loader which may not be owned by the class loader. We keep these
- // live so that we do not have issues closing any of the dex files.
- std::vector<GcRoot<mirror::Object>> dex_files_ GUARDED_BY(lock_);
+ // Extra strong roots that can be either dex files or dex caches. Dex files used by the class
+ // loader which may not be owned by the class loader must be held strongly live. Also dex caches
+ // are held live to prevent them being unloading once they have classes in them.
+ std::vector<GcRoot<mirror::Object>> strong_roots_ GUARDED_BY(lock_);
};
} // namespace art
diff --git a/runtime/native/dalvik_system_DexFile.cc b/runtime/native/dalvik_system_DexFile.cc
index f30f7a6..8c7c966 100644
--- a/runtime/native/dalvik_system_DexFile.cc
+++ b/runtime/native/dalvik_system_DexFile.cc
@@ -278,9 +278,7 @@
StackHandleScope<1> hs(soa.Self());
Handle<mirror::ClassLoader> class_loader(
hs.NewHandle(soa.Decode<mirror::ClassLoader*>(javaLoader)));
- class_linker->RegisterDexFile(
- *dex_file,
- class_linker->GetOrCreateAllocatorForClassLoader(class_loader.Get()));
+ class_linker->RegisterDexFile(*dex_file, class_loader.Get());
mirror::Class* result = class_linker->DefineClass(soa.Self(),
descriptor.c_str(),
hash,
diff --git a/runtime/native/dalvik_system_VMRuntime.cc b/runtime/native/dalvik_system_VMRuntime.cc
index 6c943dc..79b18aa 100644
--- a/runtime/native/dalvik_system_VMRuntime.cc
+++ b/runtime/native/dalvik_system_VMRuntime.cc
@@ -504,8 +504,7 @@
const DexFile* dex_file = boot_class_path[i];
CHECK(dex_file != nullptr);
StackHandleScope<1> hs(soa.Self());
- Handle<mirror::DexCache> dex_cache(
- hs.NewHandle(linker->RegisterDexFile(*dex_file, runtime->GetLinearAlloc())));
+ Handle<mirror::DexCache> dex_cache(hs.NewHandle(linker->RegisterDexFile(*dex_file, nullptr)));
if (kPreloadDexCachesStrings) {
for (size_t j = 0; j < dex_cache->NumStrings(); j++) {