Abort transaction when Class.forName() fails.

And update VmClassLoader.findLoadedClass implementation in
UnstartedRuntime which has erroneously diverged since
    https://android-review.googlesource.com/145075 .
Also prevent transactional interpreter from transfering
control to a catch handler for aborted transactions.

Also clean up Transaction::kAbortExceptionDescriptor naming
and some unused parameters.

Test: TransactionTest.CatchClassForNameAbortClass
Test: m test-art-host-gtest
Test: testrunner.py --host --optimizing
Change-Id: Ibfc544283f5434efbaab238d11a6152ed2578050

11 files changed, 146 insertions, 71 deletions

diff --git a/dex2oat/driver/compiler_driver.cc b/dex2oat/driver/compiler_driver.cc
index def5af9a0b..fdf52d2015 100644
--- a/dex2oat/driver/compiler_driver.cc
+++ b/dex2oat/driver/compiler_driver.cc
@@ -2238,7 +2238,7 @@ class InitializeClassVisitor : public CompilationVisitor {
             // compiler and will be pruned by ImageWriter.
             Handle<mirror::Class> exception_class =
                 hs.NewHandle(class_linker->FindClass(self,
-                                                     Transaction::kAbortExceptionSignature,
+                                                     Transaction::kAbortExceptionDescriptor,
                                                      class_loader));
             bool exception_initialized =
                 class_linker->EnsureInitialized(self, exception_class, true, true);
diff --git a/runtime/interpreter/interpreter.cc b/runtime/interpreter/interpreter.cc
index 0586ad9e76..302551f8dd 100644
--- a/runtime/interpreter/interpreter.cc
+++ b/runtime/interpreter/interpreter.cc
@@ -303,14 +303,13 @@ static inline JValue Execute(
         // any value.
         DCHECK(Runtime::Current()->AreNonStandardExitsEnabled());
         JValue ret = JValue();
-        bool res = PerformNonStandardReturn<MonitorState::kNoMonitorsLocked>(
+        PerformNonStandardReturn<MonitorState::kNoMonitorsLocked>(
             self,
             shadow_frame,
             ret,
             instrumentation,
             accessor.InsSize(),
             0);
-        DCHECK(res) << "Expected to perform non-standard return!";
         return ret;
       }
       if (UNLIKELY(self->IsExceptionPending())) {
@@ -321,14 +320,13 @@ static inline JValue Execute(
         JValue ret = JValue();
         if (UNLIKELY(shadow_frame.GetForcePopFrame())) {
           DCHECK(Runtime::Current()->AreNonStandardExitsEnabled());
-          bool res = PerformNonStandardReturn<MonitorState::kNoMonitorsLocked>(
+          PerformNonStandardReturn<MonitorState::kNoMonitorsLocked>(
               self,
               shadow_frame,
               ret,
               instrumentation,
               accessor.InsSize(),
               0);
-          DCHECK(res) << "Expected to perform non-standard return!";
         }
         return ret;
       }
diff --git a/runtime/interpreter/interpreter_common.h b/runtime/interpreter/interpreter_common.h
index 65bff8e114..959df0010d 100644
--- a/runtime/interpreter/interpreter_common.h
+++ b/runtime/interpreter/interpreter_common.h
@@ -158,7 +158,8 @@ NeedsMethodExitEvent(const instrumentation::Instrumentation* ins)
 template <bool kMonitorCounting>
 static NO_INLINE void UnlockHeldMonitors(Thread* self, ShadowFrame* shadow_frame)
     REQUIRES_SHARED(Locks::mutator_lock_) {
-  DCHECK(shadow_frame->GetForcePopFrame());
+  DCHECK(shadow_frame->GetForcePopFrame() ||
+         Runtime::Current()->IsTransactionAborted());
   // Unlock all monitors.
   if (kMonitorCounting && shadow_frame->GetMethod()->MustCountLocks()) {
     // Get the monitors from the shadow-frame monitor-count data.
@@ -194,7 +195,7 @@ enum class MonitorState {
 };
 
 template<MonitorState kMonitorState>
-static inline ALWAYS_INLINE WARN_UNUSED bool PerformNonStandardReturn(
+static inline ALWAYS_INLINE void PerformNonStandardReturn(
       Thread* self,
       ShadowFrame& frame,
       JValue& result,
@@ -202,28 +203,23 @@ static inline ALWAYS_INLINE WARN_UNUSED bool PerformNonStandardReturn(
       uint16_t num_dex_inst,
       uint32_t dex_pc) REQUIRES_SHARED(Locks::mutator_lock_) {
   static constexpr bool kMonitorCounting = (kMonitorState == MonitorState::kCountingMonitors);
-  if (UNLIKELY(frame.GetForcePopFrame())) {
-    ObjPtr<mirror::Object> thiz(frame.GetThisObject(num_dex_inst));
-    StackHandleScope<1> hs(self);
-    Handle<mirror::Object> h_thiz(hs.NewHandle(thiz));
-    DCHECK(Runtime::Current()->AreNonStandardExitsEnabled());
-    if (UNLIKELY(self->IsExceptionPending())) {
-      LOG(WARNING) << "Suppressing exception for non-standard method exit: "
-                   << self->GetException()->Dump();
-      self->ClearException();
-    }
-    if (kMonitorState != MonitorState::kNoMonitorsLocked) {
-      UnlockHeldMonitors<kMonitorCounting>(self, &frame);
-    }
-    DoMonitorCheckOnExit<kMonitorCounting>(self, &frame);
-    result = JValue();
-    if (UNLIKELY(NeedsMethodExitEvent(instrumentation))) {
-      SendMethodExitEvents(
-          self, instrumentation, frame, h_thiz.Get(), frame.GetMethod(), dex_pc, result);
-    }
-    return true;
+  ObjPtr<mirror::Object> thiz(frame.GetThisObject(num_dex_inst));
+  StackHandleScope<1u> hs(self);
+  Handle<mirror::Object> h_thiz(hs.NewHandle(thiz));
+  if (UNLIKELY(self->IsExceptionPending())) {
+    LOG(WARNING) << "Suppressing exception for non-standard method exit: "
+                 << self->GetException()->Dump();
+    self->ClearException();
+  }
+  if (kMonitorState != MonitorState::kNoMonitorsLocked) {
+    UnlockHeldMonitors<kMonitorCounting>(self, &frame);
+  }
+  DoMonitorCheckOnExit<kMonitorCounting>(self, &frame);
+  result = JValue();
+  if (UNLIKELY(NeedsMethodExitEvent(instrumentation))) {
+    SendMethodExitEvents(
+        self, instrumentation, frame, h_thiz.Get(), frame.GetMethod(), dex_pc, result);
   }
-  return false;
 }
 
 // Handles all invoke-XXX/range instructions except for invoke-polymorphic[/range].
diff --git a/runtime/interpreter/interpreter_switch_impl-inl.h b/runtime/interpreter/interpreter_switch_impl-inl.h
index 538ac12bf6..d7a1b1b149 100644
--- a/runtime/interpreter/interpreter_switch_impl-inl.h
+++ b/runtime/interpreter/interpreter_switch_impl-inl.h
@@ -55,13 +55,37 @@ class InstructionHandler {
  public:
 #define HANDLER_ATTRIBUTES ALWAYS_INLINE FLATTEN WARN_UNUSED REQUIRES_SHARED(Locks::mutator_lock_)
 
+  HANDLER_ATTRIBUTES bool CheckTransactionAbort() {
+    if (transaction_active && Runtime::Current()->IsTransactionAborted()) {
+      // Transaction abort cannot be caught by catch handlers.
+      // Preserve the abort exception while doing non-standard return.
+      StackHandleScope<1u> hs(Self());
+      Handle<mirror::Throwable> abort_exception = hs.NewHandle(Self()->GetException());
+      DCHECK(abort_exception != nullptr);
+      DCHECK(abort_exception->GetClass()->DescriptorEquals(Transaction::kAbortExceptionDescriptor));
+      Self()->ClearException();
+      PerformNonStandardReturn<kMonitorState>(Self(),
+                                              shadow_frame_,
+                                              ctx_->result,
+                                              Instrumentation(),
+                                              Accessor().InsSize(),
+                                              inst_->GetDexPc(Insns()));
+      Self()->SetException(abort_exception.Get());
+      ExitInterpreterLoop();
+      return false;
+    }
+    return true;
+  }
+
   HANDLER_ATTRIBUTES bool CheckForceReturn() {
-    if (PerformNonStandardReturn<kMonitorState>(Self(),
-                                                shadow_frame_,
-                                                ctx_->result,
-                                                Instrumentation(),
-                                                Accessor().InsSize(),
-                                                inst_->GetDexPc(Insns()))) {
+    if (shadow_frame_.GetForcePopFrame()) {
+      DCHECK(Runtime::Current()->AreNonStandardExitsEnabled());
+      PerformNonStandardReturn<kMonitorState>(Self(),
+                                              shadow_frame_,
+                                              ctx_->result,
+                                              Instrumentation(),
+                                              Accessor().InsSize(),
+                                              inst_->GetDexPc(Insns()));
       ExitInterpreterLoop();
       return false;
     }
@@ -71,6 +95,9 @@ class InstructionHandler {
   HANDLER_ATTRIBUTES bool HandlePendingException() {
     DCHECK(Self()->IsExceptionPending());
     Self()->AllowThreadSuspension();
+    if (!CheckTransactionAbort()) {
+      return false;
+    }
     if (!CheckForceReturn()) {
       return false;
     }
diff --git a/runtime/interpreter/unstarted_runtime.cc b/runtime/interpreter/unstarted_runtime.cc
index 2c040ab29b..73fffe8b8b 100644
--- a/runtime/interpreter/unstarted_runtime.cc
+++ b/runtime/interpreter/unstarted_runtime.cc
@@ -127,24 +127,17 @@ void UnstartedRuntime::UnstartedCharacterToUpperCase(
 }
 
 // Helper function to deal with class loading in an unstarted runtime.
-static void UnstartedRuntimeFindClass(Thread* self, Handle<mirror::String> className,
-                                      Handle<mirror::ClassLoader> class_loader, JValue* result,
-                                      const std::string& method_name, bool initialize_class,
-                                      bool abort_if_not_found)
+static void UnstartedRuntimeFindClass(Thread* self,
+                                      Handle<mirror::String> className,
+                                      Handle<mirror::ClassLoader> class_loader,
+                                      JValue* result,
+                                      bool initialize_class)
     REQUIRES_SHARED(Locks::mutator_lock_) {
   CHECK(className != nullptr);
   std::string descriptor(DotToDescriptor(className->ToModifiedUtf8().c_str()));
   ClassLinker* class_linker = Runtime::Current()->GetClassLinker();
 
   ObjPtr<mirror::Class> found = class_linker->FindClass(self, descriptor.c_str(), class_loader);
-  if (found == nullptr && abort_if_not_found) {
-    if (!self->IsExceptionPending()) {
-      AbortTransactionOrFail(self, "%s failed in un-started runtime for class: %s",
-                             method_name.c_str(),
-                             PrettyDescriptor(descriptor.c_str()).c_str());
-    }
-    return;
-  }
   if (found != nullptr && initialize_class) {
     StackHandleScope<1> hs(self);
     HandleWrapperObjPtr<mirror::Class> h_class = hs.NewHandleWrapper(&found);
@@ -164,9 +157,23 @@ static void UnstartedRuntimeFindClass(Thread* self, Handle<mirror::String> class
 static void CheckExceptionGenerateClassNotFound(Thread* self)
     REQUIRES_SHARED(Locks::mutator_lock_) {
   if (self->IsExceptionPending()) {
-    // If it is not the transaction abort exception, wrap it.
-    std::string type(mirror::Object::PrettyTypeOf(self->GetException()));
-    if (type != Transaction::kAbortExceptionDescriptor) {
+    Runtime* runtime = Runtime::Current();
+    DCHECK_EQ(runtime->IsTransactionAborted(),
+              self->GetException()->GetClass()->DescriptorEquals(
+                  Transaction::kAbortExceptionDescriptor))
+        << self->GetException()->GetClass()->PrettyDescriptor();
+    if (runtime->IsActiveTransaction()) {
+      // The boot class path at run time may contain additional dex files with
+      // the required class definition(s). We cannot throw a normal exception at
+      // compile time because a class initializer could catch it and successfully
+      // initialize a class differently than when executing at run time.
+      // If we're not aborting the transaction yet, abort now. b/183691501
+      if (!runtime->IsTransactionAborted()) {
+        AbortTransactionF(self, "ClassNotFoundException");
+      }
+    } else {
+      // If not in a transaction, it cannot be the transaction abort exception. Wrap it.
+      DCHECK(!runtime->IsTransactionAborted());
       self->ThrowNewWrappedException("Ljava/lang/ClassNotFoundException;",
                                      "ClassNotFoundException");
     }
@@ -206,8 +213,7 @@ void UnstartedRuntime::UnstartedClassForNameCommon(Thread* self,
                                                    ShadowFrame* shadow_frame,
                                                    JValue* result,
                                                    size_t arg_offset,
-                                                   bool long_form,
-                                                   const char* caller) {
+                                                   bool long_form) {
   ObjPtr<mirror::String> class_name = GetClassName(self, shadow_frame, arg_offset);
   if (class_name == nullptr) {
     return;
@@ -239,20 +245,18 @@ void UnstartedRuntime::UnstartedClassForNameCommon(Thread* self,
                             h_class_name,
                             ScopedNullHandle<mirror::ClassLoader>(),
                             result,
-                            caller,
-                            initialize_class,
-                            false);
+                            initialize_class);
   CheckExceptionGenerateClassNotFound(self);
 }
 
 void UnstartedRuntime::UnstartedClassForName(
     Thread* self, ShadowFrame* shadow_frame, JValue* result, size_t arg_offset) {
-  UnstartedClassForNameCommon(self, shadow_frame, result, arg_offset, false, "Class.forName");
+  UnstartedClassForNameCommon(self, shadow_frame, result, arg_offset, /*long_form=*/ false);
 }
 
 void UnstartedRuntime::UnstartedClassForNameLong(
     Thread* self, ShadowFrame* shadow_frame, JValue* result, size_t arg_offset) {
-  UnstartedClassForNameCommon(self, shadow_frame, result, arg_offset, true, "Class.forName");
+  UnstartedClassForNameCommon(self, shadow_frame, result, arg_offset, /*long_form=*/ true);
 }
 
 void UnstartedRuntime::UnstartedClassGetPrimitiveClass(
@@ -271,7 +275,7 @@ void UnstartedRuntime::UnstartedClassGetPrimitiveClass(
 
 void UnstartedRuntime::UnstartedClassClassForName(
     Thread* self, ShadowFrame* shadow_frame, JValue* result, size_t arg_offset) {
-  UnstartedClassForNameCommon(self, shadow_frame, result, arg_offset, true, "Class.classForName");
+  UnstartedClassForNameCommon(self, shadow_frame, result, arg_offset, /*long_form=*/ true);
 }
 
 void UnstartedRuntime::UnstartedClassNewInstance(
@@ -710,13 +714,27 @@ void UnstartedRuntime::UnstartedVmClassLoaderFindLoadedClass(
   StackHandleScope<2> hs(self);
   Handle<mirror::String> h_class_name(hs.NewHandle(class_name));
   Handle<mirror::ClassLoader> h_class_loader(hs.NewHandle(class_loader));
-  UnstartedRuntimeFindClass(self, h_class_name, h_class_loader, result,
-                            "VMClassLoader.findLoadedClass", false, false);
+  UnstartedRuntimeFindClass(self,
+                            h_class_name,
+                            h_class_loader,
+                            result,
+                            /*initialize_class=*/ false);
   // This might have an error pending. But semantics are to just return null.
   if (self->IsExceptionPending()) {
-    // If it is an InternalError, keep it. See CheckExceptionGenerateClassNotFound.
-    std::string type(mirror::Object::PrettyTypeOf(self->GetException()));
-    if (type != "java.lang.InternalError") {
+    Runtime* runtime = Runtime::Current();
+    DCHECK_EQ(runtime->IsTransactionAborted(),
+              self->GetException()->GetClass()->DescriptorEquals(
+                  Transaction::kAbortExceptionDescriptor))
+        << self->GetException()->GetClass()->PrettyDescriptor();
+    if (runtime->IsActiveTransaction()) {
+      // If we're not aborting the transaction yet, abort now. b/183691501
+      // See CheckExceptionGenerateClassNotFound() for more detailed explanation.
+      if (!runtime->IsTransactionAborted()) {
+        AbortTransactionF(self, "ClassNotFoundException");
+      }
+    } else {
+      // If not in a transaction, it cannot be the transaction abort exception. Clear it.
+      DCHECK(!runtime->IsTransactionAborted());
       self->ClearException();
     }
   }
diff --git a/runtime/interpreter/unstarted_runtime.h b/runtime/interpreter/unstarted_runtime.h
index 4a716455ac..8b31f8f441 100644
--- a/runtime/interpreter/unstarted_runtime.h
+++ b/runtime/interpreter/unstarted_runtime.h
@@ -93,8 +93,7 @@ class UnstartedRuntime {
                                           ShadowFrame* shadow_frame,
                                           JValue* result,
                                           size_t arg_offset,
-                                          bool long_form,
-                                          const char* caller) REQUIRES_SHARED(Locks::mutator_lock_);
+                                          bool long_form) REQUIRES_SHARED(Locks::mutator_lock_);
 
   static void InitializeInvokeHandlers(Thread* self) REQUIRES_SHARED(Locks::mutator_lock_);
   static void InitializeJNIHandlers(Thread* self) REQUIRES_SHARED(Locks::mutator_lock_);
diff --git a/runtime/interpreter/unstarted_runtime_test.cc b/runtime/interpreter/unstarted_runtime_test.cc
index 77075a6c54..ecee216e25 100644
--- a/runtime/interpreter/unstarted_runtime_test.cc
+++ b/runtime/interpreter/unstarted_runtime_test.cc
@@ -216,7 +216,7 @@ class UnstartedRuntimeTest : public CommonRuntimeTest {
   void PrepareForAborts() REQUIRES_SHARED(Locks::mutator_lock_) {
     ObjPtr<mirror::Object> result = Runtime::Current()->GetClassLinker()->FindClass(
         Thread::Current(),
-        Transaction::kAbortExceptionSignature,
+        Transaction::kAbortExceptionDescriptor,
         ScopedNullHandle<mirror::ClassLoader>());
     CHECK(result != nullptr);
   }
diff --git a/runtime/transaction.cc b/runtime/transaction.cc
index 5d9456bbac..0293d231f4 100644
--- a/runtime/transaction.cc
+++ b/runtime/transaction.cc
@@ -21,6 +21,7 @@
 #include "aot_class_linker.h"
 #include "base/mutex-inl.h"
 #include "base/stl_util.h"
+#include "dex/descriptors_names.h"
 #include "gc/accounting/card_table-inl.h"
 #include "gc/heap.h"
 #include "gc_root-inl.h"
@@ -90,16 +91,16 @@ void Transaction::Abort(const std::string& abort_message) {
 void Transaction::ThrowAbortError(Thread* self, const std::string* abort_message) {
   const bool rethrow = (abort_message == nullptr);
   if (kIsDebugBuild && rethrow) {
-    CHECK(IsAborted()) << "Rethrow " << Transaction::kAbortExceptionDescriptor
+    CHECK(IsAborted()) << "Rethrow " << DescriptorToDot(Transaction::kAbortExceptionDescriptor)
                        << " while transaction is not aborted";
   }
   if (rethrow) {
     // Rethrow an exception with the earlier abort message stored in the transaction.
-    self->ThrowNewWrappedException(Transaction::kAbortExceptionSignature,
+    self->ThrowNewWrappedException(Transaction::kAbortExceptionDescriptor,
                                    GetAbortMessage().c_str());
   } else {
     // Throw an exception with the given abort message.
-    self->ThrowNewWrappedException(Transaction::kAbortExceptionSignature,
+    self->ThrowNewWrappedException(Transaction::kAbortExceptionDescriptor,
                                    abort_message->c_str());
   }
 }
diff --git a/runtime/transaction.h b/runtime/transaction.h
index 184280f9c8..2fd0e2cdc5 100644
--- a/runtime/transaction.h
+++ b/runtime/transaction.h
@@ -45,8 +45,7 @@ template<class MirrorType> class ObjPtr;
 
 class Transaction final {
  public:
-  static constexpr const char* kAbortExceptionDescriptor = "dalvik.system.TransactionAbortError";
-  static constexpr const char* kAbortExceptionSignature = "Ldalvik/system/TransactionAbortError;";
+  static constexpr const char* kAbortExceptionDescriptor = "Ldalvik/system/TransactionAbortError;";
 
   Transaction(bool strict, mirror::Class* root);
   ~Transaction();
diff --git a/runtime/transaction_test.cc b/runtime/transaction_test.cc
index 91eafabf77..ee7c59135c 100644
--- a/runtime/transaction_test.cc
+++ b/runtime/transaction_test.cc
@@ -54,7 +54,7 @@ class TransactionTest : public CommonRuntimeTest {
     ASSERT_TRUE(h_klass->IsInitialized());
 
     h_klass.Assign(class_linker_->FindSystemClass(soa.Self(),
-                                                  Transaction::kAbortExceptionSignature));
+                                                  Transaction::kAbortExceptionDescriptor));
     ASSERT_TRUE(h_klass != nullptr);
     class_linker_->EnsureInitialized(soa.Self(), h_klass, true, true);
     ASSERT_TRUE(h_klass->IsInitialized());
@@ -599,6 +599,18 @@ TEST_F(TransactionTest, MultipleNativeCallAbortClass) {
   testTransactionAbort("LTransaction$MultipleNativeCallAbortClass;");
 }
 
+// Tests failing class initialization due to Class.forName() not finding the class,
+// even if an "all" catch handler catches the exception thrown when aborting the transaction.
+TEST_F(TransactionTest, CatchClassForNameAbortClass) {
+  testTransactionAbort("LTransaction$CatchClassForNameAbortClass;");
+}
+
+// Same as CatchClassForNameAbortClass but the class initializer tries to do the work twice.
+// This would trigger a DCHECK() if we continued executing bytecode with an aborted transaction.
+TEST_F(TransactionTest, CatchClassForNameAbortClassTwice) {
+  testTransactionAbort("LTransaction$CatchClassForNameAbortClassTwice;");
+}
+
 // Tests failing class initialization due to allocating instance of finalizable class.
 TEST_F(TransactionTest, FinalizableAbortClass) {
   testTransactionAbort("LTransaction$FinalizableAbortClass;");
diff --git a/test/Transaction/Transaction.java b/test/Transaction/Transaction.java
index e7085c1734..6a4f5ef773 100644
--- a/test/Transaction/Transaction.java
+++ b/test/Transaction/Transaction.java
@@ -74,6 +74,31 @@ public class Transaction {
       }
     }
 
+    static class CatchClassForNameAbortClass {
+      static {
+        try {
+          Class.forName("non.existent.TestClass");
+        } catch (Throwable e) {
+          // ignore exception.
+        }
+      }
+    }
+
+    static class CatchClassForNameAbortClassTwice {
+      static {
+        try {
+          Class.forName("non.existent.TestClass");
+        } catch (Throwable e) {
+          // ignore exception.
+        }
+        try {
+          Class.forName("non.existent.TestClass");
+        } catch (Throwable e) {
+          // ignore exception.
+        }
+      }
+    }
+
     // Helper class to abort transaction: finalizable class with natve methods.
     static class AbortHelperClass {
       public void finalize() throws Throwable {