Revert^8 "Make sure that const-class linkage is preserved."

Replaced two ReaderMutexLocks with WriterMutexLocks.
Removed some unnecessary debugging output.

Test: m test-art-host
Bug: 30627598
Original-Change-Id: Ie9b721464b4e9a5dcce8df8095548e983bba1fe8

This reverts commit 2c8c6b63da6ecb2ac701cc30f9b4fa4a8eea5cc8.

Change-Id: I3a1aeecf64e4b202cef61cceb248d48106a2f4a6

1 files changed, 129 insertions, 57 deletions

diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index 319991b956..313776943b 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -1981,13 +1981,36 @@ class VisitClassLoaderClassesVisitor : public ClassLoaderVisitor {
   void Visit(ObjPtr<mirror::ClassLoader> class_loader)
       REQUIRES_SHARED(Locks::classlinker_classes_lock_, Locks::mutator_lock_) OVERRIDE {
     ClassTable* const class_table = class_loader->GetClassTable();
-    if (!done_ && class_table != nullptr && !class_table->Visit(*visitor_)) {
-      // If the visitor ClassTable returns false it means that we don't need to continue.
-      done_ = true;
+    if (!done_ && class_table != nullptr) {
+      DefiningClassLoaderFilterVisitor visitor(class_loader, visitor_);
+      if (!class_table->Visit(visitor)) {
+        // If the visitor ClassTable returns false it means that we don't need to continue.
+        done_ = true;
+      }
     }
   }
 
  private:
+  // Class visitor that limits the class visits from a ClassTable to the classes with
+  // the provided defining class loader. This filter is used to avoid multiple visits
+  // of the same class which can be recorded for multiple initiating class loaders.
+  class DefiningClassLoaderFilterVisitor : public ClassVisitor {
+   public:
+    DefiningClassLoaderFilterVisitor(ObjPtr<mirror::ClassLoader> defining_class_loader,
+                                     ClassVisitor* visitor)
+        : defining_class_loader_(defining_class_loader), visitor_(visitor) { }
+
+    bool operator()(ObjPtr<mirror::Class> klass) OVERRIDE REQUIRES_SHARED(Locks::mutator_lock_) {
+      if (klass->GetClassLoader() != defining_class_loader_) {
+        return true;
+      }
+      return (*visitor_)(klass);
+    }
+
+    ObjPtr<mirror::ClassLoader> const defining_class_loader_;
+    ClassVisitor* const visitor_;
+  };
+
   ClassVisitor* const visitor_;
   // If done is true then we don't need to do any more visiting.
   bool done_;
@@ -2474,56 +2497,109 @@ mirror::Class* ClassLinker::FindClass(Thread* self,
     }
   } else {
     ScopedObjectAccessUnchecked soa(self);
-    ObjPtr<mirror::Class> cp_klass;
-    if (FindClassInBaseDexClassLoader(soa, self, descriptor, hash, class_loader, &cp_klass)) {
-      // The chain was understood. So the value in cp_klass is either the class we were looking
-      // for, or not found.
-      if (cp_klass != nullptr) {
-        return cp_klass.Ptr();
-      }
-      // TODO: We handle the boot classpath loader in FindClassInBaseDexClassLoader. Try to unify
-      //       this and the branch above. TODO: throw the right exception here.
-
-      // We'll let the Java-side rediscover all this and throw the exception with the right stack
-      // trace.
-    }
+    ObjPtr<mirror::Class> result_ptr;
+    bool descriptor_equals;
+    bool known_hierarchy =
+        FindClassInBaseDexClassLoader(soa, self, descriptor, hash, class_loader, &result_ptr);
+    if (result_ptr != nullptr) {
+      // The chain was understood and we found the class. We still need to add the class to
+      // the class table to protect from racy programs that can try and redefine the path list
+      // which would change the Class<?> returned for subsequent evaluation of const-class.
+      DCHECK(known_hierarchy);
+      DCHECK(result_ptr->DescriptorEquals(descriptor));
+      descriptor_equals = true;
+    } else {
+      // Either the chain wasn't understood or the class wasn't found.
+      //
+      // If the chain was understood but we did not find the class, let the Java-side
+      // rediscover all this and throw the exception with the right stack trace. Note that
+      // the Java-side could still succeed for racy programs if another thread is actively
+      // modifying the class loader's path list.
 
-    if (Runtime::Current()->IsAotCompiler()) {
-      // Oops, compile-time, can't run actual class-loader code.
-      ObjPtr<mirror::Throwable> pre_allocated = Runtime::Current()->GetPreAllocatedNoClassDefFoundError();
-      self->SetException(pre_allocated);
-      return nullptr;
-    }
+      if (Runtime::Current()->IsAotCompiler()) {
+        // Oops, compile-time, can't run actual class-loader code.
+        ObjPtr<mirror::Throwable> pre_allocated =
+            Runtime::Current()->GetPreAllocatedNoClassDefFoundError();
+        self->SetException(pre_allocated);
+        return nullptr;
+      }
 
-    ScopedLocalRef<jobject> class_loader_object(soa.Env(),
-                                                soa.AddLocalReference<jobject>(class_loader.Get()));
-    std::string class_name_string(DescriptorToDot(descriptor));
-    ScopedLocalRef<jobject> result(soa.Env(), nullptr);
-    {
-      ScopedThreadStateChange tsc(self, kNative);
-      ScopedLocalRef<jobject> class_name_object(soa.Env(),
-                                                soa.Env()->NewStringUTF(class_name_string.c_str()));
-      if (class_name_object.get() == nullptr) {
-        DCHECK(self->IsExceptionPending());  // OOME.
+      ScopedLocalRef<jobject> class_loader_object(
+          soa.Env(), soa.AddLocalReference<jobject>(class_loader.Get()));
+      std::string class_name_string(DescriptorToDot(descriptor));
+      ScopedLocalRef<jobject> result(soa.Env(), nullptr);
+      {
+        ScopedThreadStateChange tsc(self, kNative);
+        ScopedLocalRef<jobject> class_name_object(
+            soa.Env(), soa.Env()->NewStringUTF(class_name_string.c_str()));
+        if (class_name_object.get() == nullptr) {
+          DCHECK(self->IsExceptionPending());  // OOME.
+          return nullptr;
+        }
+        CHECK(class_loader_object.get() != nullptr);
+        result.reset(soa.Env()->CallObjectMethod(class_loader_object.get(),
+                                                 WellKnownClasses::java_lang_ClassLoader_loadClass,
+                                                 class_name_object.get()));
+      }
+      if (self->IsExceptionPending()) {
+        // If the ClassLoader threw, pass that exception up.
+        // However, to comply with the RI behavior, first check if another thread succeeded.
+        result_ptr = LookupClass(self, descriptor, hash, class_loader.Get());
+        if (result_ptr != nullptr && !result_ptr->IsErroneous()) {
+          self->ClearException();
+          return EnsureResolved(self, descriptor, result_ptr);
+        }
+        return nullptr;
+      } else if (result.get() == nullptr) {
+        // broken loader - throw NPE to be compatible with Dalvik
+        ThrowNullPointerException(StringPrintf("ClassLoader.loadClass returned null for %s",
+                                               class_name_string.c_str()).c_str());
         return nullptr;
       }
-      CHECK(class_loader_object.get() != nullptr);
-      result.reset(soa.Env()->CallObjectMethod(class_loader_object.get(),
-                                               WellKnownClasses::java_lang_ClassLoader_loadClass,
-                                               class_name_object.get()));
+      result_ptr = soa.Decode<mirror::Class>(result.get());
+      // Check the name of the returned class.
+      descriptor_equals = result_ptr->DescriptorEquals(descriptor);
     }
-    if (self->IsExceptionPending()) {
-      // If the ClassLoader threw, pass that exception up.
-      return nullptr;
-    } else if (result.get() == nullptr) {
-      // broken loader - throw NPE to be compatible with Dalvik
-      ThrowNullPointerException(StringPrintf("ClassLoader.loadClass returned null for %s",
-                                             class_name_string.c_str()).c_str());
+
+    // Try to insert the class to the class table, checking for mismatch.
+    ObjPtr<mirror::Class> old;
+    {
+      WriterMutexLock mu(self, *Locks::classlinker_classes_lock_);
+      ClassTable* const class_table = InsertClassTableForClassLoader(class_loader.Get());
+      old = class_table->Lookup(descriptor, hash);
+      if (old == nullptr) {
+        old = result_ptr;  // For the comparison below, after releasing the lock.
+        if (descriptor_equals) {
+          class_table->InsertWithHash(result_ptr.Ptr(), hash);
+          Runtime::Current()->GetHeap()->WriteBarrierEveryFieldOf(class_loader.Get());
+        }  // else throw below, after releasing the lock.
+      }
+    }
+    if (UNLIKELY(old != result_ptr)) {
+      // Return `old` (even if `!descriptor_equals`) to mimic the RI behavior for parallel
+      // capable class loaders.  (All class loaders are considered parallel capable on Android.)
+      mirror::Class* loader_class = class_loader->GetClass();
+      const char* loader_class_name =
+          loader_class->GetDexFile().StringByTypeIdx(loader_class->GetDexTypeIndex());
+      LOG(WARNING) << "Initiating class loader of type " << DescriptorToDot(loader_class_name)
+          << " is not well-behaved; it returned a different Class for racing loadClass(\""
+          << DescriptorToDot(descriptor) << "\").";
+      return EnsureResolved(self, descriptor, old);
+    }
+    if (UNLIKELY(!descriptor_equals)) {
+      std::string result_storage;
+      const char* result_name = result_ptr->GetDescriptor(&result_storage);
+      std::string loader_storage;
+      const char* loader_class_name = class_loader->GetClass()->GetDescriptor(&loader_storage);
+      ThrowNoClassDefFoundError(
+          "Initiating class loader of type %s returned class %s instead of %s.",
+          DescriptorToDot(loader_class_name).c_str(),
+          DescriptorToDot(result_name).c_str(),
+          DescriptorToDot(descriptor).c_str());
       return nullptr;
-    } else {
-      // success, return mirror::Class*
-      return soa.Decode<mirror::Class>(result.get()).Ptr();
     }
+    // success, return mirror::Class*
+    return result_ptr.Ptr();
   }
   UNREACHABLE();
 }
@@ -3609,12 +3685,6 @@ void ClassLinker::UpdateClassMethods(ObjPtr<mirror::Class> klass,
   Runtime::Current()->GetHeap()->WriteBarrierEveryFieldOf(klass);
 }
 
-bool ClassLinker::RemoveClass(const char* descriptor, ObjPtr<mirror::ClassLoader> class_loader) {
-  WriterMutexLock mu(Thread::Current(), *Locks::classlinker_classes_lock_);
-  ClassTable* const class_table = ClassTableForClassLoader(class_loader);
-  return class_table != nullptr && class_table->Remove(descriptor);
-}
-
 mirror::Class* ClassLinker::LookupClass(Thread* self,
                                         const char* descriptor,
                                         size_t hash,
@@ -3665,7 +3735,8 @@ class LookupClassesVisitor : public ClassLoaderVisitor {
       REQUIRES_SHARED(Locks::classlinker_classes_lock_, Locks::mutator_lock_) OVERRIDE {
     ClassTable* const class_table = class_loader->GetClassTable();
     ObjPtr<mirror::Class> klass = class_table->Lookup(descriptor_, hash_);
-    if (klass != nullptr) {
+    // Add `klass` only if `class_loader` is its defining (not just initiating) class loader.
+    if (klass != nullptr && klass->GetClassLoader() == class_loader) {
       result_->push_back(klass);
     }
   }
@@ -3684,6 +3755,7 @@ void ClassLinker::LookupClasses(const char* descriptor,
   const size_t hash = ComputeModifiedUtf8Hash(descriptor);
   ObjPtr<mirror::Class> klass = boot_class_table_.Lookup(descriptor, hash);
   if (klass != nullptr) {
+    DCHECK(klass->GetClassLoader() == nullptr);
     result.push_back(klass);
   }
   LookupClassesVisitor visitor(descriptor, hash, &result);
@@ -7972,8 +8044,8 @@ class CountClassesVisitor : public ClassLoaderVisitor {
       REQUIRES_SHARED(Locks::classlinker_classes_lock_, Locks::mutator_lock_) OVERRIDE {
     ClassTable* const class_table = class_loader->GetClassTable();
     if (class_table != nullptr) {
-      num_zygote_classes += class_table->NumZygoteClasses();
-      num_non_zygote_classes += class_table->NumNonZygoteClasses();
+      num_zygote_classes += class_table->NumZygoteClasses(class_loader);
+      num_non_zygote_classes += class_table->NumNonZygoteClasses(class_loader);
     }
   }
 
@@ -7984,13 +8056,13 @@ class CountClassesVisitor : public ClassLoaderVisitor {
 size_t ClassLinker::NumZygoteClasses() const {
   CountClassesVisitor visitor;
   VisitClassLoaders(&visitor);
-  return visitor.num_zygote_classes + boot_class_table_.NumZygoteClasses();
+  return visitor.num_zygote_classes + boot_class_table_.NumZygoteClasses(nullptr);
 }
 
 size_t ClassLinker::NumNonZygoteClasses() const {
   CountClassesVisitor visitor;
   VisitClassLoaders(&visitor);
-  return visitor.num_non_zygote_classes + boot_class_table_.NumNonZygoteClasses();
+  return visitor.num_non_zygote_classes + boot_class_table_.NumNonZygoteClasses(nullptr);
 }
 
 size_t ClassLinker::NumLoadedClasses() {