ART: Change behavior for rethrowing init failures

Allow to store a Throwable instance or a throwable class. Handle
rethrow accordingly.

Bug: 25444180
Change-Id: I703c2c6eaf34ad0e3bc0f5a104d65f2ff1b212ca

8 files changed, 119 insertions, 47 deletions

diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index da70456369..6053469016 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -104,12 +104,13 @@ static void ThrowNoClassDefFoundError(const char* fmt, ...) {
   va_end(args);
 }
 
-bool ClassLinker::HasInitWithString(Thread* self, const char* descriptor) {
+static bool HasInitWithString(Thread* self, ClassLinker* class_linker, const char* descriptor)
+    SHARED_REQUIRES(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));
-  mirror::Class* exception_class = FindClass(self, descriptor, class_loader);
+  mirror::Class* exception_class = class_linker->FindClass(self, descriptor, class_loader);
 
   if (exception_class == nullptr) {
     // No exc class ~ no <init>-with-string.
@@ -119,10 +120,39 @@ bool ClassLinker::HasInitWithString(Thread* self, const char* descriptor) {
   }
 
   ArtMethod* exception_init_method = exception_class->FindDeclaredDirectMethod(
-      "<init>", "(Ljava/lang/String;)V", image_pointer_size_);
+      "<init>", "(Ljava/lang/String;)V", class_linker->GetImagePointerSize());
   return exception_init_method != nullptr;
 }
 
+// Helper for ThrowEarlierClassFailure. Throws the stored error.
+static void HandleEarlierVerifyError(Thread* self, ClassLinker* class_linker, mirror::Class* c)
+    SHARED_REQUIRES(Locks::mutator_lock_) {
+  mirror::Object* obj = c->GetVerifyError();
+  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, PrettyDescriptor(c).c_str());
+    } else {
+      self->ThrowNewException(descriptor, nullptr);
+    }
+  } else {
+    // Previous error has been stored as an instance. Just rethrow.
+    mirror::Class* throwable_class =
+        self->DecodeJObject(WellKnownClasses::java_lang_Throwable)->AsClass();
+    mirror::Class* error_class = obj->GetClass();
+    CHECK(throwable_class->IsAssignableFrom(error_class));
+    self->SetException(obj->AsThrowable());
+  }
+  self->AssertPendingException();
+}
+
 void ClassLinker::ThrowEarlierClassFailure(mirror::Class* c) {
   // The class failed to initialize on a previous attempt, so we want to throw
   // a NoClassDefFoundError (v2 2.17.5).  The exception to this rule is if we
@@ -131,8 +161,11 @@ void ClassLinker::ThrowEarlierClassFailure(mirror::Class* c) {
   Runtime* const runtime = Runtime::Current();
   if (!runtime->IsAotCompiler()) {  // Give info if this occurs at runtime.
     std::string extra;
-    if (c->GetVerifyErrorClass() != nullptr) {
-      extra = PrettyDescriptor(c->GetVerifyErrorClass());
+    if (c->GetVerifyError() != nullptr) {
+      mirror::Class* descr_from = c->GetVerifyError()->IsClass()
+                                      ? c->GetVerifyError()->AsClass()
+                                      : c->GetVerifyError()->GetClass();
+      extra = PrettyDescriptor(descr_from);
     }
     LOG(INFO) << "Rejecting re-init on previously-failed class " << PrettyClass(c) << ": " << extra;
   }
@@ -144,17 +177,8 @@ void ClassLinker::ThrowEarlierClassFailure(mirror::Class* c) {
     mirror::Throwable* pre_allocated = runtime->GetPreAllocatedNoClassDefFoundError();
     self->SetException(pre_allocated);
   } else {
-    if (c->GetVerifyErrorClass() != nullptr) {
-      // TODO: change the verifier to store an _instance_, with a useful detail message?
-      // It's possible the exception doesn't have a <init>(String).
-      std::string temp;
-      const char* descriptor = c->GetVerifyErrorClass()->GetDescriptor(&temp);
-
-      if (HasInitWithString(self, descriptor)) {
-        self->ThrowNewException(descriptor, PrettyDescriptor(c).c_str());
-      } else {
-        self->ThrowNewException(descriptor, nullptr);
-      }
+    if (c->GetVerifyError() != nullptr) {
+      HandleEarlierVerifyError(self, this, c);
     } else {
       self->ThrowNewException("Ljava/lang/NoClassDefFoundError;",
                               PrettyDescriptor(c).c_str());
diff --git a/runtime/class_linker.h b/runtime/class_linker.h
index 392efd23e2..73f9d4b864 100644
--- a/runtime/class_linker.h
+++ b/runtime/class_linker.h
@@ -854,9 +854,6 @@ class ClassLinker {
       SHARED_REQUIRES(Locks::mutator_lock_)
       REQUIRES(!dex_lock_);
 
-  bool HasInitWithString(Thread* self, const char* descriptor)
-      SHARED_REQUIRES(Locks::mutator_lock_) REQUIRES(!dex_lock_);
-
   bool CanWeInitializeClass(mirror::Class* klass, bool can_init_statics, bool can_init_parents)
       SHARED_REQUIRES(Locks::mutator_lock_);
 
diff --git a/runtime/class_linker_test.cc b/runtime/class_linker_test.cc
index 04b890063d..2c086c59f0 100644
--- a/runtime/class_linker_test.cc
+++ b/runtime/class_linker_test.cc
@@ -515,7 +515,7 @@ struct ClassOffsets : public CheckOffsets<mirror::Class> {
     addOffset(OFFSETOF_MEMBER(mirror::Class, sfields_), "sFields");
     addOffset(OFFSETOF_MEMBER(mirror::Class, status_), "status");
     addOffset(OFFSETOF_MEMBER(mirror::Class, super_class_), "superClass");
-    addOffset(OFFSETOF_MEMBER(mirror::Class, verify_error_class_), "verifyErrorClass");
+    addOffset(OFFSETOF_MEMBER(mirror::Class, verify_error_), "verifyError");
     addOffset(OFFSETOF_MEMBER(mirror::Class, virtual_methods_), "virtualMethods");
     addOffset(OFFSETOF_MEMBER(mirror::Class, vtable_), "vtable");
   };
diff --git a/runtime/mirror/class-inl.h b/runtime/mirror/class-inl.h
index 19ee7f4dd4..174de0e2f6 100644
--- a/runtime/mirror/class-inl.h
+++ b/runtime/mirror/class-inl.h
@@ -520,15 +520,6 @@ inline void Class::SetClinitThreadId(pid_t new_clinit_thread_id) {
   }
 }
 
-inline void Class::SetVerifyErrorClass(Class* klass) {
-  CHECK(klass != nullptr) << PrettyClass(this);
-  if (Runtime::Current()->IsActiveTransaction()) {
-    SetFieldObject<true>(OFFSET_OF_OBJECT_MEMBER(Class, verify_error_class_), klass);
-  } else {
-    SetFieldObject<false>(OFFSET_OF_OBJECT_MEMBER(Class, verify_error_class_), klass);
-  }
-}
-
 template<VerifyObjectFlags kVerifyFlags>
 inline uint32_t Class::GetAccessFlags() {
   // Check class is loaded/retired or this is java.lang.String that has a
diff --git a/runtime/mirror/class.cc b/runtime/mirror/class.cc
index 9d01a1db60..77275f007b 100644
--- a/runtime/mirror/class.cc
+++ b/runtime/mirror/class.cc
@@ -57,6 +57,15 @@ void Class::VisitRoots(RootVisitor* visitor) {
   java_lang_Class_.VisitRootIfNonNull(visitor, RootInfo(kRootStickyClass));
 }
 
+inline void Class::SetVerifyError(mirror::Object* error) {
+  CHECK(error != nullptr) << PrettyClass(this);
+  if (Runtime::Current()->IsActiveTransaction()) {
+    SetFieldObject<true>(OFFSET_OF_OBJECT_MEMBER(Class, verify_error_), error);
+  } else {
+    SetFieldObject<false>(OFFSET_OF_OBJECT_MEMBER(Class, verify_error_), error);
+  }
+}
+
 void Class::SetStatus(Handle<Class> h_this, Status new_status, Thread* self) {
   Status old_status = h_this->GetStatus();
   ClassLinker* class_linker = Runtime::Current()->GetClassLinker();
@@ -109,7 +118,13 @@ void Class::SetStatus(Handle<Class> h_this, Status new_status, Thread* self) {
       // case.
       Class* exception_class = old_exception->GetClass();
       if (!eiie_class->IsAssignableFrom(exception_class)) {
-        h_this->SetVerifyErrorClass(exception_class);
+        // Store the exception class when this is the AoT compiler. Don't store full exceptions,
+        // as they need to be trimmed (native components are not storable in an image).
+        if (Runtime::Current()->IsAotCompiler()) {
+          h_this->SetVerifyError(exception_class);
+        } else {
+          h_this->SetVerifyError(old_exception.Get());
+        }
       }
     }
 
diff --git a/runtime/mirror/class.h b/runtime/mirror/class.h
index 8219d69b6e..c4339b9230 100644
--- a/runtime/mirror/class.h
+++ b/runtime/mirror/class.h
@@ -1015,9 +1015,9 @@ class MANAGED Class FINAL : public Object {
 
   void SetClinitThreadId(pid_t new_clinit_thread_id) SHARED_REQUIRES(Locks::mutator_lock_);
 
-  Class* GetVerifyErrorClass() SHARED_REQUIRES(Locks::mutator_lock_) {
+  Object* GetVerifyError() SHARED_REQUIRES(Locks::mutator_lock_) {
     // DCHECK(IsErroneous());
-    return GetFieldObject<Class>(OFFSET_OF_OBJECT_MEMBER(Class, verify_error_class_));
+    return GetFieldObject<Class>(OFFSET_OF_OBJECT_MEMBER(Class, verify_error_));
   }
 
   uint16_t GetDexClassDefIndex() SHARED_REQUIRES(Locks::mutator_lock_) {
@@ -1158,7 +1158,7 @@ class MANAGED Class FINAL : public Object {
       SHARED_REQUIRES(Locks::mutator_lock_);
 
  private:
-  void SetVerifyErrorClass(Class* klass) SHARED_REQUIRES(Locks::mutator_lock_);
+  void SetVerifyError(Object* klass) SHARED_REQUIRES(Locks::mutator_lock_);
 
   template <bool throw_on_failure, bool use_referrers_cache>
   bool ResolvedFieldAccessTest(Class* access_to, ArtField* field,
@@ -1230,8 +1230,9 @@ class MANAGED Class FINAL : public Object {
   // check for interfaces and return null.
   HeapReference<Class> super_class_;
 
-  // If class verify fails, we must return same error on subsequent tries.
-  HeapReference<Class> verify_error_class_;
+  // If class verify fails, we must return same error on subsequent tries. We may store either
+  // the class of the error, or an actual instance of Throwable here.
+  HeapReference<Object> verify_error_;
 
   // Virtual method table (vtable), for use by "invoke-virtual".  The vtable from the superclass is
   // copied in, and virtual methods from our class either replace those from the super or are
diff --git a/test/008-exceptions/expected.txt b/test/008-exceptions/expected.txt
index 92c79dc2a0..ea59a49677 100644
--- a/test/008-exceptions/expected.txt
+++ b/test/008-exceptions/expected.txt
@@ -1,12 +1,15 @@
 Got an NPE: second throw
 java.lang.NullPointerException: second throw
-	at Main.catchAndRethrow(Main.java:58)
-	at Main.exceptions_007(Main.java:41)
-	at Main.main(Main.java:49)
+	at Main.catchAndRethrow(Main.java:77)
+	at Main.exceptions_007(Main.java:59)
+	at Main.main(Main.java:67)
 Caused by: java.lang.NullPointerException: first throw
-	at Main.throwNullPointerException(Main.java:65)
-	at Main.catchAndRethrow(Main.java:55)
+	at Main.throwNullPointerException(Main.java:84)
+	at Main.catchAndRethrow(Main.java:74)
 	... 2 more
 Static Init
-BadError: This is bad by convention
-BadError: This is bad by convention
+BadError: This is bad by convention: BadInit
+BadError: This is bad by convention: BadInit
+Static BadInitNoStringInit
+BadErrorNoStringInit: This is bad by convention
+BadErrorNoStringInit: This is bad by convention
diff --git a/test/008-exceptions/src/Main.java b/test/008-exceptions/src/Main.java
index 7f6d0c5956..c0502047d0 100644
--- a/test/008-exceptions/src/Main.java
+++ b/test/008-exceptions/src/Main.java
@@ -14,20 +14,38 @@
  * limitations under the License.
  */
 
-// An exception that doesn't have a <init>(String) method.
+// An error class.
 class BadError extends Error {
-    public BadError() {
-        super("This is bad by convention");
+    public BadError(String s) {
+        super("This is bad by convention: " + s);
     }
 }
 
-// A class that throws BadException during static initialization.
+// A class that throws BadError during static initialization.
 class BadInit {
     static int dummy;
     static {
         System.out.println("Static Init");
         if (true) {
-            throw new BadError();
+            throw new BadError("BadInit");
+        }
+    }
+}
+
+// An error that doesn't have a <init>(String) method.
+class BadErrorNoStringInit extends Error {
+    public BadErrorNoStringInit() {
+        super("This is bad by convention");
+    }
+}
+
+// A class that throws BadErrorNoStringInit during static initialization.
+class BadInitNoStringInit {
+    static int dummy;
+    static {
+        System.out.println("Static BadInitNoStringInit");
+        if (true) {
+            throw new BadErrorNoStringInit();
         }
     }
 }
@@ -48,6 +66,7 @@ public class Main {
     public static void main (String args[]) {
         exceptions_007();
         exceptionsRethrowClassInitFailure();
+        exceptionsRethrowClassInitFailureNoStringInit();
     }
 
     private static void catchAndRethrow() {
@@ -86,4 +105,26 @@ public class Main {
             error.printStackTrace();
         }
     }
+
+    private static void exceptionsRethrowClassInitFailureNoStringInit() {
+        try {
+            try {
+                BadInitNoStringInit.dummy = 1;
+                throw new IllegalStateException("Should not reach here.");
+            } catch (BadErrorNoStringInit e) {
+                System.out.println(e);
+            }
+
+            // Check if it works a second time.
+
+            try {
+                BadInitNoStringInit.dummy = 1;
+                throw new IllegalStateException("Should not reach here.");
+            } catch (BadErrorNoStringInit e) {
+                System.out.println(e);
+            }
+        } catch (Exception error) {
+            error.printStackTrace();
+        }
+    }
 }