Clean up `IndirectReferenceTable` construction.

Split the parts that can fail to a separate function.

Test: m test-art-host-gtest
Test: testrunner.py --host --optimizing
Bug: 172332525
Change-Id: I95200a31cc757e4593d9cc7f956dd4d5ef624f92

8 files changed, 80 insertions, 104 deletions

diff --git a/runtime/indirect_reference_table.cc b/runtime/indirect_reference_table.cc
index ebf382f2ec..1e1adf0755 100644
--- a/runtime/indirect_reference_table.cc
+++ b/runtime/indirect_reference_table.cc
@@ -113,23 +113,24 @@ void SmallIrtAllocator::Deallocate(IrtEntry* unneeded) {
   small_irt_freelist_ = unneeded;
 }
 
-IndirectReferenceTable::IndirectReferenceTable(size_t max_count,
-                                               IndirectRefKind desired_kind,
-                                               ResizableCapacity resizable,
-                                               std::string* error_msg)
+IndirectReferenceTable::IndirectReferenceTable(IndirectRefKind desired_kind,
+                                               ResizableCapacity resizable)
     : segment_state_(kIRTFirstSegment),
       table_(nullptr),
       kind_(desired_kind),
-      max_entries_(max_count),
+      max_entries_(0u),
       current_num_holes_(0),
       resizable_(resizable) {
-  CHECK(error_msg != nullptr);
   CHECK_NE(desired_kind, kJniTransitionOrInvalid);
+}
+
+bool IndirectReferenceTable::Initialize(size_t max_count, std::string* error_msg) {
+  CHECK(error_msg != nullptr);
 
   // Overflow and maximum check.
   CHECK_LE(max_count, kMaxTableSizeInBytes / sizeof(IrtEntry));
 
-  if (max_entries_ <= kSmallIrtEntries) {
+  if (max_count <= kSmallIrtEntries) {
     table_ = Runtime::Current()->GetSmallIrtAllocator()->Allocate(error_msg);
     if (table_ != nullptr) {
       max_entries_ = kSmallIrtEntries;
@@ -139,20 +140,18 @@ IndirectReferenceTable::IndirectReferenceTable(size_t max_count,
   if (table_ == nullptr) {
     const size_t table_bytes = RoundUp(max_count * sizeof(IrtEntry), kPageSize);
     table_mem_map_ = NewIRTMap(table_bytes, error_msg);
-    if (!table_mem_map_.IsValid() && error_msg->empty()) {
-      *error_msg = "Unable to map memory for indirect ref table";
+    if (!table_mem_map_.IsValid()) {
+      DCHECK(!error_msg->empty());
+      return false;
     }
 
-    if (table_mem_map_.IsValid()) {
-      table_ = reinterpret_cast<IrtEntry*>(table_mem_map_.Begin());
-    } else {
-      table_ = nullptr;
-    }
+    table_ = reinterpret_cast<IrtEntry*>(table_mem_map_.Begin());
     // Take into account the actual length.
     max_entries_ = table_bytes / sizeof(IrtEntry);
   }
   segment_state_ = kIRTFirstSegment;
   last_known_previous_state_ = kIRTFirstSegment;
+  return true;
 }
 
 IndirectReferenceTable::~IndirectReferenceTable() {
diff --git a/runtime/indirect_reference_table.h b/runtime/indirect_reference_table.h
index e279422b4b..18d07307e1 100644
--- a/runtime/indirect_reference_table.h
+++ b/runtime/indirect_reference_table.h
@@ -257,17 +257,14 @@ class IndirectReferenceTable {
     kYes
   };
 
-  // WARNING: Construction of the IndirectReferenceTable may fail.
-  // error_msg must not be null. If error_msg is set by the constructor, then
-  // construction has failed and the IndirectReferenceTable will be in an
-  // invalid state. Use IsValid to check whether the object is in an invalid
-  // state.
+  // Constructs an uninitialized indirect reference table. Use `Initialize()` to initialize it.
+  IndirectReferenceTable(IndirectRefKind kind, ResizableCapacity resizable);
+
+  // Initialize the indirect reference table.
+  //
   // Max_count is the minimum initial capacity (resizable), or minimum total capacity
   // (not resizable). A value of 1 indicates an implementation-convenient small size.
-  IndirectReferenceTable(size_t max_count,
-                         IndirectRefKind kind,
-                         ResizableCapacity resizable,
-                         std::string* error_msg);
+  bool Initialize(size_t max_count, std::string* error_msg);
 
   ~IndirectReferenceTable();
 
diff --git a/runtime/indirect_reference_table_test.cc b/runtime/indirect_reference_table_test.cc
index af0ebaecf0..8e04469825 100644
--- a/runtime/indirect_reference_table_test.cc
+++ b/runtime/indirect_reference_table_test.cc
@@ -60,11 +60,9 @@ TEST_F(IndirectReferenceTableTest, BasicTest) {
   ScopedObjectAccess soa(Thread::Current());
   static const size_t kTableMax = 20;
   std::string error_msg;
-  IndirectReferenceTable irt(kTableMax,
-                             kGlobal,
-                             IndirectReferenceTable::ResizableCapacity::kNo,
-                             &error_msg);
-  ASSERT_TRUE(irt.IsValid()) << error_msg;
+  IndirectReferenceTable irt(kGlobal, IndirectReferenceTable::ResizableCapacity::kNo);
+  bool success = irt.Initialize(kTableMax, &error_msg);
+  ASSERT_TRUE(success) << error_msg;
 
   StackHandleScope<5> hs(soa.Self());
   Handle<mirror::Class> c =
@@ -302,11 +300,9 @@ TEST_F(IndirectReferenceTableTest, Holes) {
 
   // 1) Segment with holes (current_num_holes_ > 0), push new segment, add/remove reference.
   {
-    IndirectReferenceTable irt(kTableMax,
-                               kGlobal,
-                               IndirectReferenceTable::ResizableCapacity::kNo,
-                               &error_msg);
-    ASSERT_TRUE(irt.IsValid()) << error_msg;
+    IndirectReferenceTable irt(kGlobal, IndirectReferenceTable::ResizableCapacity::kNo);
+    bool success = irt.Initialize(kTableMax, &error_msg);
+    ASSERT_TRUE(success) << error_msg;
 
     const IRTSegmentState cookie0 = kIRTFirstSegment;
 
@@ -333,11 +329,9 @@ TEST_F(IndirectReferenceTableTest, Holes) {
 
   // 2) Segment with holes (current_num_holes_ > 0), pop segment, add/remove reference
   {
-    IndirectReferenceTable irt(kTableMax,
-                               kGlobal,
-                               IndirectReferenceTable::ResizableCapacity::kNo,
-                               &error_msg);
-    ASSERT_TRUE(irt.IsValid()) << error_msg;
+    IndirectReferenceTable irt(kGlobal, IndirectReferenceTable::ResizableCapacity::kNo);
+    bool success = irt.Initialize(kTableMax, &error_msg);
+    ASSERT_TRUE(success) << error_msg;
 
     const IRTSegmentState cookie0 = kIRTFirstSegment;
 
@@ -369,11 +363,9 @@ TEST_F(IndirectReferenceTableTest, Holes) {
   // 3) Segment with holes (current_num_holes_ > 0), push new segment, pop segment, add/remove
   //    reference.
   {
-    IndirectReferenceTable irt(kTableMax,
-                               kGlobal,
-                               IndirectReferenceTable::ResizableCapacity::kNo,
-                               &error_msg);
-    ASSERT_TRUE(irt.IsValid()) << error_msg;
+    IndirectReferenceTable irt(kGlobal, IndirectReferenceTable::ResizableCapacity::kNo);
+    bool success = irt.Initialize(kTableMax, &error_msg);
+    ASSERT_TRUE(success) << error_msg;
 
     const IRTSegmentState cookie0 = kIRTFirstSegment;
 
@@ -408,11 +400,9 @@ TEST_F(IndirectReferenceTableTest, Holes) {
 
   // 4) Empty segment, push new segment, create a hole, pop a segment, add/remove a reference.
   {
-    IndirectReferenceTable irt(kTableMax,
-                               kGlobal,
-                               IndirectReferenceTable::ResizableCapacity::kNo,
-                               &error_msg);
-    ASSERT_TRUE(irt.IsValid()) << error_msg;
+    IndirectReferenceTable irt(kGlobal, IndirectReferenceTable::ResizableCapacity::kNo);
+    bool success = irt.Initialize(kTableMax, &error_msg);
+    ASSERT_TRUE(success) << error_msg;
 
     const IRTSegmentState cookie0 = kIRTFirstSegment;
 
@@ -451,11 +441,9 @@ TEST_F(IndirectReferenceTableTest, Holes) {
   // 5) Base segment, push new segment, create a hole, pop a segment, push new segment, add/remove
   //    reference
   {
-    IndirectReferenceTable irt(kTableMax,
-                               kGlobal,
-                               IndirectReferenceTable::ResizableCapacity::kNo,
-                               &error_msg);
-    ASSERT_TRUE(irt.IsValid()) << error_msg;
+    IndirectReferenceTable irt(kGlobal, IndirectReferenceTable::ResizableCapacity::kNo);
+    bool success = irt.Initialize(kTableMax, &error_msg);
+    ASSERT_TRUE(success) << error_msg;
 
     const IRTSegmentState cookie0 = kIRTFirstSegment;
 
@@ -501,11 +489,9 @@ TEST_F(IndirectReferenceTableTest, Resize) {
   ASSERT_TRUE(obj0 != nullptr);
 
   std::string error_msg;
-  IndirectReferenceTable irt(kTableMax,
-                             kLocal,
-                             IndirectReferenceTable::ResizableCapacity::kYes,
-                             &error_msg);
-  ASSERT_TRUE(irt.IsValid()) << error_msg;
+  IndirectReferenceTable irt(kLocal, IndirectReferenceTable::ResizableCapacity::kYes);
+  bool success = irt.Initialize(kTableMax, &error_msg);
+  ASSERT_TRUE(success) << error_msg;
 
   CheckDump(&irt, 0, 0);
   const IRTSegmentState cookie = kIRTFirstSegment;
diff --git a/runtime/jni/java_vm_ext.cc b/runtime/jni/java_vm_ext.cc
index 8eeb093bc6..248335a78a 100644
--- a/runtime/jni/java_vm_ext.cc
+++ b/runtime/jni/java_vm_ext.cc
@@ -495,9 +495,7 @@ const JNIInvokeInterface gJniInvokeInterface = {
   JII::AttachCurrentThreadAsDaemon
 };
 
-JavaVMExt::JavaVMExt(Runtime* runtime,
-                     const RuntimeArgumentMap& runtime_options,
-                     std::string* error_msg)
+JavaVMExt::JavaVMExt(Runtime* runtime, const RuntimeArgumentMap& runtime_options)
     : runtime_(runtime),
       check_jni_abort_hook_(nullptr),
       check_jni_abort_hook_data_(nullptr),
@@ -506,13 +504,10 @@ JavaVMExt::JavaVMExt(Runtime* runtime,
       tracing_enabled_(runtime_options.Exists(RuntimeArgumentMap::JniTrace)
                        || VLOG_IS_ON(third_party_jni)),
       trace_(runtime_options.GetOrDefault(RuntimeArgumentMap::JniTrace)),
-      globals_(kGlobalsMax, kGlobal, IndirectReferenceTable::ResizableCapacity::kNo, error_msg),
+      globals_(kGlobal, IndirectReferenceTable::ResizableCapacity::kNo),
       libraries_(new Libraries),
       unchecked_functions_(&gJniInvokeInterface),
-      weak_globals_(kWeakGlobalsMax,
-                    kWeakGlobal,
-                    IndirectReferenceTable::ResizableCapacity::kNo,
-                    error_msg),
+      weak_globals_(kWeakGlobal, IndirectReferenceTable::ResizableCapacity::kNo),
       allow_accessing_weak_globals_(true),
       weak_globals_add_condition_("weak globals add condition",
                                   (CHECK(Locks::jni_weak_globals_lock_ != nullptr),
@@ -527,21 +522,23 @@ JavaVMExt::JavaVMExt(Runtime* runtime,
   SetCheckJniEnabled(runtime_options.Exists(RuntimeArgumentMap::CheckJni) || kIsDebugBuild);
 }
 
+bool JavaVMExt::Initialize(std::string* error_msg) {
+  return globals_.Initialize(kGlobalsMax, error_msg) &&
+         weak_globals_.Initialize(kWeakGlobalsMax, error_msg);
+}
+
 JavaVMExt::~JavaVMExt() {
   UnloadBootNativeLibraries();
 }
 
-// Checking "globals" and "weak_globals" usually requires locks, but we
-// don't need the locks to check for validity when constructing the
-// object. Use NO_THREAD_SAFETY_ANALYSIS for this.
 std::unique_ptr<JavaVMExt> JavaVMExt::Create(Runtime* runtime,
                                              const RuntimeArgumentMap& runtime_options,
-                                             std::string* error_msg) NO_THREAD_SAFETY_ANALYSIS {
-  std::unique_ptr<JavaVMExt> java_vm(new JavaVMExt(runtime, runtime_options, error_msg));
-  if (java_vm && java_vm->globals_.IsValid() && java_vm->weak_globals_.IsValid()) {
-    return java_vm;
+                                             std::string* error_msg) {
+  std::unique_ptr<JavaVMExt> java_vm(new JavaVMExt(runtime, runtime_options));
+  if (!java_vm->Initialize(error_msg)) {
+    return nullptr;
   }
-  return nullptr;
+  return java_vm;
 }
 
 jint JavaVMExt::HandleGetEnv(/*out*/void** env, jint version) {
diff --git a/runtime/jni/java_vm_ext.h b/runtime/jni/java_vm_ext.h
index 08de18b2ac..b97088aaf7 100644
--- a/runtime/jni/java_vm_ext.h
+++ b/runtime/jni/java_vm_ext.h
@@ -218,9 +218,12 @@ class JavaVMExt : public JavaVM {
   static jstring GetLibrarySearchPath(JNIEnv* env, jobject class_loader);
 
  private:
-  // The constructor should not be called directly. It may leave the object in
-  // an erroneous state, and the result needs to be checked.
-  JavaVMExt(Runtime* runtime, const RuntimeArgumentMap& runtime_options, std::string* error_msg);
+  // The constructor should not be called directly. Use `Create()` that initializes
+  // the new `JavaVMExt` object by calling `Initialize()`.
+  JavaVMExt(Runtime* runtime, const RuntimeArgumentMap& runtime_options);
+
+  // Initialize the `JavaVMExt` object.
+  bool Initialize(std::string* error_msg);
 
   // Return true if self can currently access weak globals.
   bool MayAccessWeakGlobals(Thread* self) const REQUIRES_SHARED(Locks::mutator_lock_);
diff --git a/runtime/jni/jni_env_ext.cc b/runtime/jni/jni_env_ext.cc
index 4510b37ff5..7d522c15ee 100644
--- a/runtime/jni/jni_env_ext.cc
+++ b/runtime/jni/jni_env_ext.cc
@@ -44,13 +44,6 @@ static constexpr size_t kMonitorsMax = 4096;  // Maximum number of monitors held
 
 const JNINativeInterface* JNIEnvExt::table_override_ = nullptr;
 
-bool JNIEnvExt::CheckLocalsValid(JNIEnvExt* in) NO_THREAD_SAFETY_ANALYSIS {
-  if (in == nullptr) {
-    return false;
-  }
-  return in->locals_.IsValid();
-}
-
 jint JNIEnvExt::GetEnvHandler(JavaVMExt* vm, /*out*/void** env, jint version) {
   UNUSED(vm);
   // GetEnv always returns a JNIEnv* for the most current supported JNI version,
@@ -66,18 +59,18 @@ jint JNIEnvExt::GetEnvHandler(JavaVMExt* vm, /*out*/void** env, jint version) {
 }
 
 JNIEnvExt* JNIEnvExt::Create(Thread* self_in, JavaVMExt* vm_in, std::string* error_msg) {
-  std::unique_ptr<JNIEnvExt> ret(new JNIEnvExt(self_in, vm_in, error_msg));
-  if (CheckLocalsValid(ret.get())) {
-    return ret.release();
+  std::unique_ptr<JNIEnvExt> ret(new JNIEnvExt(self_in, vm_in));
+  if (!ret->Initialize(error_msg)) {
+    return nullptr;
   }
-  return nullptr;
+  return ret.release();
 }
 
-JNIEnvExt::JNIEnvExt(Thread* self_in, JavaVMExt* vm_in, std::string* error_msg)
+JNIEnvExt::JNIEnvExt(Thread* self_in, JavaVMExt* vm_in)
     : self_(self_in),
       vm_(vm_in),
       local_ref_cookie_(kIRTFirstSegment),
-      locals_(1, kLocal, IndirectReferenceTable::ResizableCapacity::kYes, error_msg),
+      locals_(kLocal, IndirectReferenceTable::ResizableCapacity::kYes),
       monitors_("monitors", kMonitorsInitial, kMonitorsMax),
       critical_(0),
       check_jni_(false),
@@ -88,6 +81,10 @@ JNIEnvExt::JNIEnvExt(Thread* self_in, JavaVMExt* vm_in, std::string* error_msg)
   unchecked_functions_ = GetJniNativeInterface();
 }
 
+bool JNIEnvExt::Initialize(std::string* error_msg) {
+  return locals_.Initialize(/*max_count=*/ 1u, error_msg);
+}
+
 void JNIEnvExt::SetFunctionsToRuntimeShutdownFunctions() {
   functions = GetRuntimeShutdownNativeInterface();
 }
diff --git a/runtime/jni/jni_env_ext.h b/runtime/jni/jni_env_ext.h
index bdde5f8a2f..5952b3b68d 100644
--- a/runtime/jni/jni_env_ext.h
+++ b/runtime/jni/jni_env_ext.h
@@ -151,20 +151,18 @@ class JNIEnvExt : public JNIEnv {
       REQUIRES(!Locks::thread_list_lock_, !Locks::jni_function_table_lock_);
 
  private:
-  // Checking "locals" requires the mutator lock, but at creation time we're
-  // really only interested in validity, which isn't changing. To avoid grabbing
-  // the mutator lock, factored out and tagged with NO_THREAD_SAFETY_ANALYSIS.
-  static bool CheckLocalsValid(JNIEnvExt* in) NO_THREAD_SAFETY_ANALYSIS;
-
   // Override of function tables. This applies to both default as well as instrumented (CheckJNI)
   // function tables.
   static const JNINativeInterface* table_override_ GUARDED_BY(Locks::jni_function_table_lock_);
 
-  // The constructor should not be called directly. It may leave the object in an erroneous state,
-  // and the result needs to be checked.
-  JNIEnvExt(Thread* self, JavaVMExt* vm, std::string* error_msg)
+  // The constructor should not be called directly. Use `Create()` that initializes
+  // the new `JNIEnvExt` object by calling `Initialize()`.
+  JNIEnvExt(Thread* self, JavaVMExt* vm)
       REQUIRES(!Locks::jni_function_table_lock_);
 
+  // Initialize the `JNIEnvExt` object.
+  bool Initialize(std::string* error_msg);
+
   // Link to Thread::Current().
   Thread* const self_;
 
@@ -175,7 +173,7 @@ class JNIEnvExt : public JNIEnv {
   IRTSegmentState local_ref_cookie_;
 
   // JNI local references.
-  IndirectReferenceTable locals_ GUARDED_BY(Locks::mutator_lock_);
+  IndirectReferenceTable locals_;
 
   // Stack of cookies corresponding to PushLocalFrame/PopLocalFrame calls.
   // TODO: to avoid leaks (and bugs), we need to clear this vector on entry (or return)
diff --git a/runtime/jni/jni_internal_test.cc b/runtime/jni/jni_internal_test.cc
index eca5aec7a8..a41043b9ea 100644
--- a/runtime/jni/jni_internal_test.cc
+++ b/runtime/jni/jni_internal_test.cc
@@ -2580,11 +2580,10 @@ TEST_F(JniInternalTest, IndirectReferenceTableOffsets) {
   // by modifying memory.
   // The parameters don't really matter here.
   std::string error_msg;
-  IndirectReferenceTable irt(5,
-                             IndirectRefKind::kGlobal,
-                             IndirectReferenceTable::ResizableCapacity::kNo,
-                             &error_msg);
-  ASSERT_TRUE(irt.IsValid()) << error_msg;
+  IndirectReferenceTable irt(IndirectRefKind::kGlobal,
+                             IndirectReferenceTable::ResizableCapacity::kNo);
+  bool success = irt.Initialize(/*max_count=*/ 5, &error_msg);
+  ASSERT_TRUE(success) << error_msg;
   IRTSegmentState old_state = irt.GetSegmentState();
 
   // Write some new state directly. We invert parts of old_state to ensure a new value.