Keep dex files live in class table

The DexFile.loadClass API allows callers to load classes using a
dex file without having that dex file owned by the specified class
loader. We now add the dex file to the class table to make sure it
stays live until the class loader is unreachable.

Fixes interpreter gcstress test 087 with 64 bit.

Bug: 22720414
Change-Id: Ia4341149f45b6293312f8b275c7a68cea179f718

diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index 395649e..f58aaa6 100644 (file)
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -6369,6 +6369,21 @@ void ClassLinker::VisitClassLoaders(ClassLoaderVisitor* visitor) const {
   }
 }
 
+void ClassLinker::InsertDexFileInToClassLoader(mirror::Object* dex_file,
+                                               mirror::ClassLoader* class_loader) {
+  DCHECK(dex_file != nullptr);
+  DCHECK(class_loader != nullptr);
+  Thread* const self = Thread::Current();
+  WriterMutexLock mu(self, *Locks::classlinker_classes_lock_);
+  ClassTable* const table = class_loader->GetClassTable();
+  DCHECK(table != nullptr);
+  if (table->InsertDexFile(dex_file)) {
+    // 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);
+  }
+}
+
 void ClassLinker::CleanupClassLoaders() {
   Thread* const self = Thread::Current();
   WriterMutexLock mu(self, *Locks::classlinker_classes_lock_);

diff --git a/runtime/class_linker.h b/runtime/class_linker.h
index fd30a46..a2d38ac 100644 (file)
--- a/runtime/class_linker.h
+++ b/runtime/class_linker.h
@@ -526,8 +526,8 @@ class ClassLinker {
 
   // Clean up class loaders, this needs to happen after JNI weak globals are cleared.
   void CleanupClassLoaders()
-      SHARED_REQUIRES(Locks::mutator_lock_)
-      REQUIRES(!Locks::classlinker_classes_lock_);
+      REQUIRES(!Locks::classlinker_classes_lock_)
+      SHARED_REQUIRES(Locks::mutator_lock_);
 
   // Unlike GetOrCreateAllocatorForClassLoader, GetAllocatorForClassLoader asserts that the
   // allocator for this class loader is already created.
@@ -537,8 +537,12 @@ class ClassLinker {
   // Return the linear alloc for a class loader if it is already allocated, otherwise allocate and
   // set it. TODO: Consider using a lock other than classlinker_classes_lock_.
   static LinearAlloc* GetOrCreateAllocatorForClassLoader(mirror::ClassLoader* class_loader)
-      SHARED_REQUIRES(Locks::mutator_lock_)
-      REQUIRES(!Locks::classlinker_classes_lock_);
+      REQUIRES(!Locks::classlinker_classes_lock_)
+      SHARED_REQUIRES(Locks::mutator_lock_);
+
+  void InsertDexFileInToClassLoader(mirror::Object* dex_file, mirror::ClassLoader* class_loader)
+      REQUIRES(!Locks::classlinker_classes_lock_)
+      SHARED_REQUIRES(Locks::mutator_lock_);
 
  private:
   struct ClassLoaderData {

diff --git a/runtime/class_table-inl.h b/runtime/class_table-inl.h
index dc60a2c..aef02b6 100644 (file)
--- a/runtime/class_table-inl.h
+++ b/runtime/class_table-inl.h
@@ -37,6 +37,9 @@ void ClassTable::VisitRoots(const Visitor& visitor) {
       visitor.VisitRoot(root.AddressWithoutBarrier());
     }
   }
+  for (GcRoot<mirror::Object>& root : dex_files_) {
+    visitor.VisitRoot(root.AddressWithoutBarrier());
+  }
 }
 
 }  // namespace art

diff --git a/runtime/class_table.cc b/runtime/class_table.cc
index 4b0cbc8..3ed1c95 100644 (file)
--- a/runtime/class_table.cc
+++ b/runtime/class_table.cc
@@ -137,4 +137,15 @@ std::size_t ClassTable::ClassDescriptorHashEquals::operator()(const char* descri
   return ComputeModifiedUtf8Hash(descriptor);
 }
 
+bool ClassTable::InsertDexFile(mirror::Object* dex_file) {
+  DCHECK(dex_file != nullptr);
+  for (GcRoot<mirror::Object>& root : dex_files_) {
+    if (root.Read() == dex_file) {
+      return false;
+    }
+  }
+  dex_files_.push_back(GcRoot<mirror::Object>(dex_file));
+  return true;
+}
+
 }  // namespace art

diff --git a/runtime/class_table.h b/runtime/class_table.h
index 727392e..002bb56 100644 (file)
--- a/runtime/class_table.h
+++ b/runtime/class_table.h
@@ -50,12 +50,14 @@ class ClassTable {
 
   // Used by image writer for checking.
   bool Contains(mirror::Class* klass)
-      REQUIRES(Locks::classlinker_classes_lock_) SHARED_REQUIRES(Locks::mutator_lock_);
+      REQUIRES(Locks::classlinker_classes_lock_)
+      SHARED_REQUIRES(Locks::mutator_lock_);
 
   // Freeze the current class tables by allocating a new table and never updating or modifying the
   // existing table. This helps prevents dirty pages after caused by inserting after zygote fork.
   void FreezeSnapshot()
-      REQUIRES(Locks::classlinker_classes_lock_) SHARED_REQUIRES(Locks::mutator_lock_);
+      REQUIRES(Locks::classlinker_classes_lock_)
+      SHARED_REQUIRES(Locks::mutator_lock_);
 
   // Returns the number of classes in previous snapshots.
   size_t NumZygoteClasses() const SHARED_REQUIRES(Locks::classlinker_classes_lock_);
@@ -65,17 +67,18 @@ class ClassTable {
 
   // Update a class in the table with the new class. Returns the existing class which was replaced.
   mirror::Class* UpdateClass(const char* descriptor, mirror::Class* new_klass, size_t hash)
-      REQUIRES(Locks::classlinker_classes_lock_) SHARED_REQUIRES(Locks::mutator_lock_);
+      REQUIRES(Locks::classlinker_classes_lock_)
+      SHARED_REQUIRES(Locks::mutator_lock_);
 
   // NO_THREAD_SAFETY_ANALYSIS for object marking requiring heap bitmap lock.
   template<class Visitor>
   void VisitRoots(Visitor& visitor)
-      SHARED_REQUIRES(Locks::classlinker_classes_lock_, Locks::mutator_lock_)
-      NO_THREAD_SAFETY_ANALYSIS;
+      NO_THREAD_SAFETY_ANALYSIS
+      SHARED_REQUIRES(Locks::classlinker_classes_lock_, Locks::mutator_lock_);
   template<class Visitor>
   void VisitRoots(const Visitor& visitor)
-      SHARED_REQUIRES(Locks::classlinker_classes_lock_, Locks::mutator_lock_)
-      NO_THREAD_SAFETY_ANALYSIS;
+      NO_THREAD_SAFETY_ANALYSIS
+      SHARED_REQUIRES(Locks::classlinker_classes_lock_, Locks::mutator_lock_);
 
   // Return false if the callback told us to exit.
   bool Visit(ClassVisitor* visitor)
@@ -85,13 +88,21 @@ class ClassTable {
       SHARED_REQUIRES(Locks::classlinker_classes_lock_, Locks::mutator_lock_);
 
   void Insert(mirror::Class* klass)
-      REQUIRES(Locks::classlinker_classes_lock_) SHARED_REQUIRES(Locks::mutator_lock_);
+      REQUIRES(Locks::classlinker_classes_lock_)
+      SHARED_REQUIRES(Locks::mutator_lock_);
   void InsertWithHash(mirror::Class* klass, size_t hash)
-      REQUIRES(Locks::classlinker_classes_lock_) SHARED_REQUIRES(Locks::mutator_lock_);
+      REQUIRES(Locks::classlinker_classes_lock_)
+      SHARED_REQUIRES(Locks::mutator_lock_);
 
   // Returns true if the class was found and removed, false otherwise.
   bool Remove(const char* descriptor)
-      REQUIRES(Locks::classlinker_classes_lock_) SHARED_REQUIRES(Locks::mutator_lock_);
+      REQUIRES(Locks::classlinker_classes_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)
+      REQUIRES(Locks::classlinker_classes_lock_)
+      SHARED_REQUIRES(Locks::mutator_lock_);
 
  private:
   class ClassDescriptorHashEquals {
@@ -123,6 +134,9 @@ class ClassTable {
   // TODO: shard lock to have one per class loader.
   // We have a vector to help prevent dirty pages after the zygote forks by calling FreezeSnapshot.
   std::vector<ClassSet> classes_ GUARDED_BY(Locks::classlinker_classes_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(Locks::classlinker_classes_lock_);
 };
 
 }  // namespace art

diff --git a/runtime/native/dalvik_system_DexFile.cc b/runtime/native/dalvik_system_DexFile.cc
index 4eea3f3..8b2f4d8 100644 (file)
--- a/runtime/native/dalvik_system_DexFile.cc
+++ b/runtime/native/dalvik_system_DexFile.cc
@@ -243,7 +243,8 @@ static jclass DexFile_defineClassNative(JNIEnv* env,
                                         jclass,
                                         jstring javaName,
                                         jobject javaLoader,
-                                        jobject cookie) {
+                                        jobject cookie,
+                                        jobject dexFile) {
   std::vector<const DexFile*> dex_files;
   const OatFile* oat_file;
   if (!ConvertJavaArrayToDexFiles(env, cookie, /*out*/ dex_files, /*out*/ oat_file)) {
@@ -276,6 +277,10 @@ static jclass DexFile_defineClassNative(JNIEnv* env,
                                                         class_loader,
                                                         *dex_file,
                                                         *dex_class_def);
+      // Add the used dex file. This only required for the DexFile.loadClass API since normal
+      // class loaders already keep their dex files live.
+      class_linker->InsertDexFileInToClassLoader(soa.Decode<mirror::Object*>(dexFile),
+                                                 class_loader.Get());
       if (result != nullptr) {
         VLOG(class_linker) << "DexFile_defineClassNative returning " << result
                            << " for " << class_name.c_str();
@@ -424,8 +429,13 @@ static jboolean DexFile_isDexOptNeeded(JNIEnv* env, jclass, jstring javaFilename
 
 static JNINativeMethod gMethods[] = {
   NATIVE_METHOD(DexFile, closeDexFile, "(Ljava/lang/Object;)Z"),
-  NATIVE_METHOD(DexFile, defineClassNative,
-                "(Ljava/lang/String;Ljava/lang/ClassLoader;Ljava/lang/Object;)Ljava/lang/Class;"),
+  NATIVE_METHOD(DexFile,
+                defineClassNative,
+                "(Ljava/lang/String;"
+                "Ljava/lang/ClassLoader;"
+                "Ljava/lang/Object;"
+                "Ldalvik/system/DexFile;"
+                ")Ljava/lang/Class;"),
   NATIVE_METHOD(DexFile, getClassNameList, "(Ljava/lang/Object;)[Ljava/lang/String;"),
   NATIVE_METHOD(DexFile, isDexOptNeeded, "(Ljava/lang/String;)Z"),
   NATIVE_METHOD(DexFile, getDexOptNeeded,

diff --git a/test/087-gc-after-link/src/Main.java b/test/087-gc-after-link/src/Main.java
index 2f6d496..7c47e99 100644 (file)
--- a/test/087-gc-after-link/src/Main.java
+++ b/test/087-gc-after-link/src/Main.java
@@ -91,6 +91,7 @@ public class Main {
                      * is an error we can't recover from.
                      */
                     meth.invoke(dexFile, name, this);
+                    System.out.println("Unreachable");
                 } finally {
                     if (dexFile != null) {
                         /* close the DexFile to make CloseGuard happy */