Don't wrap VerifyError into NoClassDefFoundError.

Follow RI behavior by returning the VerifyError. NoClassDefFoundError
only wraps initializer errors.

Also rename the field in ClassExt from verifyError to
erroneousStateError for better clarity.

And remove now unused feature of storing a class in the verifyError
field.

Test: test.py
Test: 824-verification-rethrow
Bug: 28313047
Change-Id: I19383f7b74f22a62ab1e0b8a13bea75a14c7b33f

14 files changed, 104 insertions, 75 deletions

diff --git a/dex2oat/linker/image_writer.cc b/dex2oat/linker/image_writer.cc
index 34ec5a8a6f..38855cb163 100644
--- a/dex2oat/linker/image_writer.cc
+++ b/dex2oat/linker/image_writer.cc
@@ -955,7 +955,7 @@ bool ImageWriter::PruneImageClassInternal(
     result = true;
   } else {
     ObjPtr<mirror::ClassExt> ext(klass->GetExtData());
-    CHECK(ext.IsNull() || ext->GetVerifyError() == nullptr) << klass->PrettyClass();
+    CHECK(ext.IsNull() || ext->GetErroneousStateError() == nullptr) << klass->PrettyClass();
   }
   if (!result) {
     // Check interfaces since these wont be visited through VisitReferences.)
diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index 0563f3830b..7fcc8a8b68 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -171,63 +171,35 @@ static void ThrowNoClassDefFoundError(const char* fmt, ...) {
   va_end(args);
 }
 
-static bool HasInitWithString(Thread* self, ClassLinker* class_linker, const char* descriptor)
-    REQUIRES_SHARED(Locks::mutator_lock_) {
-  ArtMethod* method = self->GetCurrentMethod(nullptr);
-  StackHandleScope<1> hs(self);
-  Handle<mirror::ClassLoader> class_loader(hs.NewHandle(method != nullptr ?
-      method->GetDeclaringClass()->GetClassLoader() : nullptr));
-  ObjPtr<mirror::Class> exception_class = class_linker->FindClass(self, descriptor, class_loader);
-
-  if (exception_class == nullptr) {
-    // No exc class ~ no <init>-with-string.
-    CHECK(self->IsExceptionPending());
-    self->ClearException();
-    return false;
-  }
-
-  ArtMethod* exception_init_method = exception_class->FindConstructor(
-      "(Ljava/lang/String;)V", class_linker->GetImagePointerSize());
-  return exception_init_method != nullptr;
-}
-
-static ObjPtr<mirror::Object> GetVerifyError(ObjPtr<mirror::Class> c)
+static ObjPtr<mirror::Object> GetErroneousStateError(ObjPtr<mirror::Class> c)
     REQUIRES_SHARED(Locks::mutator_lock_) {
   ObjPtr<mirror::ClassExt> ext(c->GetExtData());
   if (ext == nullptr) {
     return nullptr;
   } else {
-    return ext->GetVerifyError();
+    return ext->GetErroneousStateError();
   }
 }
 
+static bool IsVerifyError(ObjPtr<mirror::Object> obj)
+    REQUIRES_SHARED(Locks::mutator_lock_) {
+  // This is slow, but we only use it for rethrowing an error, and for DCHECK.
+  return obj->GetClass()->DescriptorEquals("Ljava/lang/VerifyError;");
+}
+
 // Helper for ThrowEarlierClassFailure. Throws the stored error.
-static void HandleEarlierVerifyError(Thread* self,
-                                     ClassLinker* class_linker,
-                                     ObjPtr<mirror::Class> c)
+static void HandleEarlierErroneousStateError(Thread* self,
+                                             ClassLinker* class_linker,
+                                             ObjPtr<mirror::Class> c)
     REQUIRES_SHARED(Locks::mutator_lock_) {
-  ObjPtr<mirror::Object> obj = GetVerifyError(c);
+  ObjPtr<mirror::Object> obj = GetErroneousStateError(c);
   DCHECK(obj != nullptr);
   self->AssertNoPendingException();
-  if (obj->IsClass()) {
-    // Previous error has been stored as class. Create a new exception of that type.
-
-    // It's possible the exception doesn't have a <init>(String).
-    std::string temp;
-    const char* descriptor = obj->AsClass()->GetDescriptor(&temp);
-
-    if (HasInitWithString(self, class_linker, descriptor)) {
-      self->ThrowNewException(descriptor, c->PrettyDescriptor().c_str());
-    } else {
-      self->ThrowNewException(descriptor, nullptr);
-    }
-  } else {
-    // Previous error has been stored as an instance. Just rethrow.
-    ObjPtr<mirror::Class> throwable_class = GetClassRoot<mirror::Throwable>(class_linker);
-    ObjPtr<mirror::Class> error_class = obj->GetClass();
-    CHECK(throwable_class->IsAssignableFrom(error_class));
-    self->SetException(obj->AsThrowable());
-  }
+  DCHECK(!obj->IsClass());
+  ObjPtr<mirror::Class> throwable_class = GetClassRoot<mirror::Throwable>(class_linker);
+  ObjPtr<mirror::Class> error_class = obj->GetClass();
+  CHECK(throwable_class->IsAssignableFrom(error_class));
+  self->SetException(obj->AsThrowable());
   self->AssertPendingException();
 }
 
@@ -530,13 +502,10 @@ void ClassLinker::ThrowEarlierClassFailure(ObjPtr<mirror::Class> c,
   Runtime* const runtime = Runtime::Current();
   if (!runtime->IsAotCompiler()) {  // Give info if this occurs at runtime.
     std::string extra;
-    ObjPtr<mirror::Object> verify_error = GetVerifyError(c);
+    ObjPtr<mirror::Object> verify_error = GetErroneousStateError(c);
     if (verify_error != nullptr) {
-      if (verify_error->IsClass()) {
-        extra = mirror::Class::PrettyDescriptor(verify_error->AsClass());
-      } else {
-        extra = verify_error->AsThrowable()->Dump();
-      }
+      DCHECK(!verify_error->IsClass());
+      extra = verify_error->AsThrowable()->Dump();
     }
     if (log) {
       LOG(INFO) << "Rejecting re-init on previously-failed class " << c->PrettyClass()
@@ -551,15 +520,16 @@ void ClassLinker::ThrowEarlierClassFailure(ObjPtr<mirror::Class> c,
     ObjPtr<mirror::Throwable> pre_allocated = runtime->GetPreAllocatedNoClassDefFoundError();
     self->SetException(pre_allocated);
   } else {
-    ObjPtr<mirror::Object> verify_error = GetVerifyError(c);
-    if (verify_error != nullptr) {
+    ObjPtr<mirror::Object> erroneous_state_error = GetErroneousStateError(c);
+    if (erroneous_state_error != nullptr) {
       // Rethrow stored error.
-      HandleEarlierVerifyError(self, this, c);
+      HandleEarlierErroneousStateError(self, this, c);
     }
     // TODO This might be wrong if we hit an OOME while allocating the ClassExt. In that case we
     // might have meant to go down the earlier if statement with the original error but it got
     // swallowed by the OOM so we end up here.
-    if (verify_error == nullptr || wrap_in_no_class_def) {
+    if (erroneous_state_error == nullptr ||
+        (wrap_in_no_class_def && !IsVerifyError(erroneous_state_error))) {
       // If there isn't a recorded earlier error, or this is a repeat throw from initialization,
       // the top-level exception must be a NoClassDefFoundError. The potentially already pending
       // exception will be a cause.
@@ -5296,8 +5266,7 @@ bool ClassLinker::InitializeClass(Thread* self,
           // whether an exception is already pending.
           if (self->IsExceptionPending()) {
             // Check that it's a VerifyError.
-            DCHECK_EQ("java.lang.Class<java.lang.VerifyError>",
-                      mirror::Class::PrettyClass(self->GetException()->GetClass()));
+            DCHECK(IsVerifyError(self->GetException()));
           } else {
             // Check that another thread attempted initialization.
             DCHECK_NE(0, klass->GetClinitThreadId());
diff --git a/runtime/class_linker_test.cc b/runtime/class_linker_test.cc
index 691fcf1293..d1f3a17792 100644
--- a/runtime/class_linker_test.cc
+++ b/runtime/class_linker_test.cc
@@ -613,6 +613,7 @@ struct ClassOffsets : public CheckOffsets<mirror::Class> {
 
 struct ClassExtOffsets : public CheckOffsets<mirror::ClassExt> {
   ClassExtOffsets() : CheckOffsets<mirror::ClassExt>(false, "Ldalvik/system/ClassExt;") {
+    addOffset(OFFSETOF_MEMBER(mirror::ClassExt, erroneous_state_error_), "erroneousStateError");
     addOffset(OFFSETOF_MEMBER(mirror::ClassExt, instance_jfield_ids_), "instanceJfieldIDs");
     addOffset(OFFSETOF_MEMBER(mirror::ClassExt, jmethod_ids_), "jmethodIDs");
     addOffset(OFFSETOF_MEMBER(mirror::ClassExt, obsolete_class_), "obsoleteClass");
@@ -624,7 +625,6 @@ struct ClassExtOffsets : public CheckOffsets<mirror::ClassExt> {
     addOffset(OFFSETOF_MEMBER(mirror::ClassExt, pre_redefine_dex_file_ptr_),
               "preRedefineDexFilePtr");
     addOffset(OFFSETOF_MEMBER(mirror::ClassExt, static_jfield_ids_), "staticJfieldIDs");
-    addOffset(OFFSETOF_MEMBER(mirror::ClassExt, verify_error_), "verifyError");
   }
 };
 
diff --git a/runtime/mirror/class.cc b/runtime/mirror/class.cc
index d411a24f6d..f4849b4015 100644
--- a/runtime/mirror/class.cc
+++ b/runtime/mirror/class.cc
@@ -232,7 +232,7 @@ void Class::SetStatus(Handle<Class> h_this, ClassStatus new_status, Thread* self
     ObjPtr<ClassExt> ext(EnsureExtDataPresent(h_this, self));
     if (!ext.IsNull()) {
       self->AssertPendingException();
-      ext->SetVerifyError(self->GetException());
+      ext->SetErroneousStateError(self->GetException());
     } else {
       self->AssertPendingOOMException();
     }
diff --git a/runtime/mirror/class_ext-inl.h b/runtime/mirror/class_ext-inl.h
index b8493c1707..ddd46b9bcb 100644
--- a/runtime/mirror/class_ext-inl.h
+++ b/runtime/mirror/class_ext-inl.h
@@ -144,9 +144,8 @@ inline bool ClassExt::HasMethodPointerIdMarker() {
   return !arr.IsNull() && !arr->IsArrayInstance();
 }
 
-
-inline ObjPtr<Object> ClassExt::GetVerifyError() {
-  return GetFieldObject<ClassExt>(OFFSET_OF_OBJECT_MEMBER(ClassExt, verify_error_));
+inline ObjPtr<Throwable> ClassExt::GetErroneousStateError() {
+  return GetFieldObject<Throwable>(OFFSET_OF_OBJECT_MEMBER(ClassExt, erroneous_state_error_));
 }
 
 inline ObjPtr<ObjectArray<DexCache>> ClassExt::GetObsoleteDexCaches() {
diff --git a/runtime/mirror/class_ext.cc b/runtime/mirror/class_ext.cc
index 7543ab6d71..097e0de2fc 100644
--- a/runtime/mirror/class_ext.cc
+++ b/runtime/mirror/class_ext.cc
@@ -118,11 +118,11 @@ ObjPtr<ClassExt> ClassExt::Alloc(Thread* self) {
   return ObjPtr<ClassExt>::DownCast(GetClassRoot<ClassExt>()->AllocObject(self));
 }
 
-void ClassExt::SetVerifyError(ObjPtr<Object> err) {
+void ClassExt::SetErroneousStateError(ObjPtr<Throwable> err) {
   if (Runtime::Current()->IsActiveTransaction()) {
-    SetFieldObject<true>(OFFSET_OF_OBJECT_MEMBER(ClassExt, verify_error_), err);
+    SetFieldObject<true>(OFFSET_OF_OBJECT_MEMBER(ClassExt, erroneous_state_error_), err);
   } else {
-    SetFieldObject<false>(OFFSET_OF_OBJECT_MEMBER(ClassExt, verify_error_), err);
+    SetFieldObject<false>(OFFSET_OF_OBJECT_MEMBER(ClassExt, erroneous_state_error_), err);
   }
 }
 
diff --git a/runtime/mirror/class_ext.h b/runtime/mirror/class_ext.h
index 2fd8c393bb..4ce3b100c0 100644
--- a/runtime/mirror/class_ext.h
+++ b/runtime/mirror/class_ext.h
@@ -42,9 +42,9 @@ class MANAGED ClassExt : public Object {
     return sizeof(ClassExt);
   }
 
-  void SetVerifyError(ObjPtr<Object> obj) REQUIRES_SHARED(Locks::mutator_lock_);
+  void SetErroneousStateError(ObjPtr<Throwable> obj) REQUIRES_SHARED(Locks::mutator_lock_);
 
-  ObjPtr<Object> GetVerifyError() REQUIRES_SHARED(Locks::mutator_lock_);
+  ObjPtr<Throwable> GetErroneousStateError() REQUIRES_SHARED(Locks::mutator_lock_);
 
   ObjPtr<ObjectArray<DexCache>> GetObsoleteDexCaches() REQUIRES_SHARED(Locks::mutator_lock_);
 
@@ -156,6 +156,9 @@ class MANAGED ClassExt : public Object {
   bool EnsureJniIdsArrayPresent(MemberOffset off, size_t count)
       REQUIRES_SHARED(Locks::mutator_lock_);
 
+  // The saved error for this class being erroneous.
+  HeapReference<Throwable> erroneous_state_error_;
+
   // Field order required by test "ValidateFieldOrderOfJavaCppUnionClasses".
   // An array containing the jfieldIDs assigned to each field in the corresponding position in the
   // classes ifields_ array or '0' if no id has been assigned to that field yet.
@@ -178,9 +181,6 @@ class MANAGED ClassExt : public Object {
   // classes sfields_ array or '0' if no id has been assigned to that field yet.
   HeapReference<PointerArray> static_jfield_ids_;
 
-  // The saved verification error of this class.
-  HeapReference<Object> verify_error_;
-
   // Native pointer to DexFile and ClassDef index of this class before it was JVMTI-redefined.
   int64_t pre_redefine_dex_file_ptr_;
   int32_t pre_redefine_class_def_index_;
diff --git a/test/142-classloader2/expected-stdout.txt b/test/142-classloader2/expected-stdout.txt
index 056d9785de..1ef91daaf6 100644
--- a/test/142-classloader2/expected-stdout.txt
+++ b/test/142-classloader2/expected-stdout.txt
@@ -1,5 +1,5 @@
 Loaded class B.
 Caught VerifyError.
 Loaded class B.
-Caught wrapped VerifyError.
+Caught existing VerifyError.
 Everything OK.
diff --git a/test/142-classloader2/src/Main.java b/test/142-classloader2/src/Main.java
index 193fd5dea0..dbb48e1fbb 100644
--- a/test/142-classloader2/src/Main.java
+++ b/test/142-classloader2/src/Main.java
@@ -72,6 +72,7 @@ public class Main {
         }
 
         // Try to load a dex file with bad dex code. Use new instance to force verification.
+        VerifyError existing = null;
         try {
           Class<?> badClass = Main.class.getClassLoader().loadClass("B");
           System.out.println("Loaded class B.");
@@ -79,6 +80,7 @@ public class Main {
           System.out.println("Should not be able to instantiate B with bad dex bytecode.");
         } catch (VerifyError e) {
           System.out.println("Caught VerifyError.");
+          existing = e;
         }
 
         // Make sure the same error is rethrown when reloading the bad class.
@@ -87,9 +89,9 @@ public class Main {
           System.out.println("Loaded class B.");
           badClass.newInstance();
           System.out.println("Should not be able to instantiate B with bad dex bytecode.");
-        } catch (NoClassDefFoundError e) {
-          if (e.getCause() instanceof VerifyError) {
-            System.out.println("Caught wrapped VerifyError.");
+        } catch (VerifyError e) {
+          if (e == existing) {
+            System.out.println("Caught existing VerifyError.");
           } else {
             e.printStackTrace(System.out);
           }
diff --git a/test/824-verification-rethrow/expected-stderr.txt b/test/824-verification-rethrow/expected-stderr.txt
new file mode 100644
index 0000000000..e69de29bb2
--- /dev/null
+++ b/test/824-verification-rethrow/expected-stderr.txt
diff --git a/test/824-verification-rethrow/expected-stdout.txt b/test/824-verification-rethrow/expected-stdout.txt
new file mode 100644
index 0000000000..e69de29bb2
--- /dev/null
+++ b/test/824-verification-rethrow/expected-stdout.txt
diff --git a/test/824-verification-rethrow/info.txt b/test/824-verification-rethrow/info.txt
new file mode 100644
index 0000000000..651efbf48d
--- /dev/null
+++ b/test/824-verification-rethrow/info.txt
@@ -0,0 +1,2 @@
+Regression test that a VerifyError does not get wrapped in a
+NoClassDefFoundError.
diff --git a/test/824-verification-rethrow/jasmin/Bar.j b/test/824-verification-rethrow/jasmin/Bar.j
new file mode 100644
index 0000000000..9d65299910
--- /dev/null
+++ b/test/824-verification-rethrow/jasmin/Bar.j
@@ -0,0 +1,24 @@
+; Copyright (C) 2021 The Android Open Source Project
+;
+; Licensed under the Apache License, Version 2.0 (the "License");
+; you may not use this file except in compliance with the License.
+; You may obtain a copy of the License at
+;
+;      http://www.apache.org/licenses/LICENSE-2.0
+;
+; Unless required by applicable law or agreed to in writing, software
+; distributed under the License is distributed on an "AS IS" BASIS,
+; WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+; See the License for the specific language governing permissions and
+; limitations under the License.
+
+.class                   public Bar
+.super                   java/lang/Object
+
+; Missing super constructor call will trigger a verification error.
+.method                  public <init>()V
+   .limit stack          1
+   .limit locals         1
+   return
+.end method
+
diff --git a/test/824-verification-rethrow/src/Main.java b/test/824-verification-rethrow/src/Main.java
new file mode 100644
index 0000000000..3460a7f501
--- /dev/null
+++ b/test/824-verification-rethrow/src/Main.java
@@ -0,0 +1,33 @@
+/*
+ * Copyright (C) 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+public class Main {
+  public static void main(String[] args) throws Exception {
+    // When calling resolution of a failed class twice, we expect a VerifyError to be
+    // thrown and not wrapped through a NoClassDefFoundError.
+    forName();
+    forName();
+  }
+
+  public static void forName() throws Exception {
+    try {
+      Class.forName("Bar");
+      throw new Error("Expected VerifyError");
+    } catch (VerifyError e) {
+      // Expected
+    }
+  }
+}