Revert "Perform SetEvent under the user_code_suspend_count_lock_"

This reverts commit 3fa8b6db7b556035f192d037b35210677250798e.

Bug: 130150240
Reason for revert: Causes lock-level violations on gcstress

Change-Id: Ie580be5658da63f951652ba74cfbf25fa845765c
diff --git a/openjdkjvmti/events.cc b/openjdkjvmti/events.cc
index beb864b..2c5ebfe 100644
--- a/openjdkjvmti/events.cc
+++ b/openjdkjvmti/events.cc
@@ -38,7 +38,6 @@
 #include "art_field-inl.h"
 #include "art_jvmti.h"
 #include "art_method-inl.h"
-#include "base/mutex.h"
 #include "deopt_manager.h"
 #include "dex/dex_file_types.h"
 #include "gc/allocation_listener.h"
@@ -1268,65 +1267,57 @@
     return ERR(MUST_POSSESS_CAPABILITY);
   }
 
-  if (thread != nullptr && !IsThreadControllable(event)) {
-    return ERR(ILLEGAL_ARGUMENT);
+  art::Thread* art_thread = nullptr;
+  if (thread != nullptr) {
+    if (!IsThreadControllable(event)) {
+      return ERR(ILLEGAL_ARGUMENT);
+    }
+    art::ScopedObjectAccess soa(art::Thread::Current());
+    art::MutexLock mu(soa.Self(), *art::Locks::thread_list_lock_);
+    jvmtiError err = ERR(INTERNAL);
+    if (!ThreadUtil::GetAliveNativeThread(thread, soa, &art_thread, &err)) {
+      return err;
+    } else if (art_thread->IsStillStarting()) {
+      return ERR(THREAD_NOT_ALIVE);
+    }
+    art::ThreadState state = art_thread->GetState();
+    if (state == art::ThreadState::kStarting || state == art::ThreadState::kTerminated) {
+      return ERR(THREAD_NOT_ALIVE);
+    }
   }
 
-  art::Thread* self = art::Thread::Current();
-  art::Thread* target = nullptr;
-  do {
-    ThreadUtil::SuspendCheck(self);
-    art::MutexLock ucsl_mu(self, *art::Locks::user_code_suspension_lock_);
-    // Make sure we won't be suspended in the middle of holding the
-    // thread_suspend_count_lock_ by a user-code suspension. We retry and do
-    // another SuspendCheck to clear this.
-    if (ThreadUtil::WouldSuspendForUserCodeLocked(self)) {
-      continue;
-    }
-    bool old_state;
-    bool new_state;
-    {
-      // From now on we know we cannot get suspended by user-code.
-      // NB This does a SuspendCheck (during thread state change) so we need to
-      // make sure we don't have the 'suspend_lock' locked here.
-      art::ScopedObjectAccess soa(self);
-      art::WriterMutexLock el_mu(self, envs_lock_);
-      art::MutexLock tll_mu(self, *art::Locks::thread_list_lock_);
-      jvmtiError err = ERR(INTERNAL);
-      if (thread != nullptr) {
-        if (!ThreadUtil::GetAliveNativeThread(thread, soa, &target, &err)) {
-          return err;
-        } else if (target->IsStillStarting() ||
-                  target->GetState() == art::ThreadState::kStarting) {
-          target->Dump(LOG_STREAM(WARNING) << "Is not alive: ");
-          return ERR(THREAD_NOT_ALIVE);
-        }
-      }
+  // TODO We use art_thread simply as a global unique identifier here. It is not safe to actually
+  // use it without holding the thread_list_lock_.
 
+  bool old_state;
+  bool new_state;
 
-      {
-        art::WriterMutexLock ei_mu(self, env->event_info_mutex_);
-        old_state = global_mask.Test(event);
-        if (mode == JVMTI_ENABLE) {
-          env->event_masks.EnableEvent(env, target, event);
-          global_mask.Set(event);
-          new_state = true;
-        } else {
-          DCHECK_EQ(mode, JVMTI_DISABLE);
+  {
+    // Change the event masks atomically.
+    art::Thread* self = art::Thread::Current();
+    art::WriterMutexLock mu(self, envs_lock_);
+    art::WriterMutexLock mu_env_info(self, env->event_info_mutex_);
+    old_state = global_mask.Test(event);
+    if (mode == JVMTI_ENABLE) {
+      env->event_masks.EnableEvent(env, art_thread, event);
+      global_mask.Set(event);
+      new_state = true;
+    } else {
+      DCHECK_EQ(mode, JVMTI_DISABLE);
 
-          env->event_masks.DisableEvent(env, target, event);
-          RecalculateGlobalEventMaskLocked(event);
-          new_state = global_mask.Test(event);
-        }
-      }
+      // TODO Replace art_thread with a uintptr_t or something to indicate we cannot read from it.
+      env->event_masks.DisableEvent(env, art_thread, event);
+      RecalculateGlobalEventMaskLocked(event);
+      new_state = global_mask.Test(event);
     }
-    // Handle any special work required for the event type. We still have the
-    // user_code_suspend_count_lock_ so there won't be any interleaving here.
-    if (new_state != old_state) {
-      return HandleEventType(event, thread, mode == JVMTI_ENABLE);
-    }
-    return OK;
-  } while (true);
+  }
+
+  // Handle any special work required for the event type.
+  if (new_state != old_state) {
+    return HandleEventType(event, thread, mode == JVMTI_ENABLE);
+  }
+
+  return ERR(NONE);
 }
 
 void EventHandler::HandleBreakpointEventsChanged(bool added) {
@@ -1349,7 +1340,7 @@
 }
 
 EventHandler::EventHandler()
-  : envs_lock_("JVMTI Environment List Lock", art::LockLevel::kPostMutatorTopLockLevel),
+  : envs_lock_("JVMTI Environment List Lock", art::LockLevel::kTopLockLevel),
     frame_pop_enabled(false) {
   alloc_listener_.reset(new JvmtiAllocationListener(this));
   ddm_listener_.reset(new JvmtiDdmChunkListener(this));
diff --git a/openjdkjvmti/events.h b/openjdkjvmti/events.h
index 2c3c7a0..e48772c 100644
--- a/openjdkjvmti/events.h
+++ b/openjdkjvmti/events.h
@@ -21,7 +21,6 @@
 #include <vector>
 
 #include <android-base/logging.h>
-#include <android-base/thread_annotations.h>
 
 #include "base/macros.h"
 #include "base/mutex.h"
@@ -324,9 +323,9 @@
   // need to be able to remove arbitrary elements from it.
   std::list<ArtJvmTiEnv*> envs GUARDED_BY(envs_lock_);
 
-  // Close to top level lock. Nothing should be held when we lock this (except for mutator_lock_
-  // which is needed when setting new events).
-  mutable art::ReaderWriterMutex envs_lock_ ACQUIRED_AFTER(art::Locks::mutator_lock_);
+  // Top level lock. Nothing at all should be held when we lock this.
+  mutable art::ReaderWriterMutex envs_lock_
+      ACQUIRED_BEFORE(art::Locks::instrument_entrypoints_lock_);
 
   // A union of all enabled events, anywhere.
   EventMask global_mask;
diff --git a/runtime/base/locks.cc b/runtime/base/locks.cc
index 4349be0..a7922a2 100644
--- a/runtime/base/locks.cc
+++ b/runtime/base/locks.cc
@@ -158,9 +158,9 @@
     DCHECK(runtime_thread_pool_lock_ != nullptr);
   } else {
     // Create global locks in level order from highest lock level to lowest.
-    LockLevel current_lock_level = kUserCodeSuspensionLock;
-    DCHECK(user_code_suspension_lock_ == nullptr);
-    user_code_suspension_lock_ = new Mutex("user code suspension lock", current_lock_level);
+    LockLevel current_lock_level = kInstrumentEntrypointsLock;
+    DCHECK(instrument_entrypoints_lock_ == nullptr);
+    instrument_entrypoints_lock_ = new Mutex("instrument entrypoint lock", current_lock_level);
 
     #define UPDATE_CURRENT_LOCK_LEVEL(new_level) \
       if ((new_level) >= current_lock_level) { \
@@ -171,9 +171,9 @@
       } \
       current_lock_level = new_level;
 
-    UPDATE_CURRENT_LOCK_LEVEL(kInstrumentEntrypointsLock);
-    DCHECK(instrument_entrypoints_lock_ == nullptr);
-    instrument_entrypoints_lock_ = new Mutex("instrument entrypoint lock", current_lock_level);
+    UPDATE_CURRENT_LOCK_LEVEL(kUserCodeSuspensionLock);
+    DCHECK(user_code_suspension_lock_ == nullptr);
+    user_code_suspension_lock_ = new Mutex("user code suspension lock", current_lock_level);
 
     UPDATE_CURRENT_LOCK_LEVEL(kMutatorLock);
     DCHECK(mutator_lock_ == nullptr);
diff --git a/runtime/base/locks.h b/runtime/base/locks.h
index b15fd32..b7d8e31 100644
--- a/runtime/base/locks.h
+++ b/runtime/base/locks.h
@@ -121,14 +121,9 @@
   kRuntimeShutdownLock,
   kTraceLock,
   kHeapBitmapLock,
-
-  // This is a generic lock level for a top-level lock meant to be gained after having the
-  // mutator_lock_.
-  kPostMutatorTopLockLevel,
-
   kMutatorLock,
-  kInstrumentEntrypointsLock,
   kUserCodeSuspensionLock,
+  kInstrumentEntrypointsLock,
   kZygoteCreationLock,
 
   // The highest valid lock level. Use this if there is code that should only be called with no
@@ -179,13 +174,13 @@
   // Check if the given mutex is in expected_mutexes_on_weak_ref_access_.
   static bool IsExpectedOnWeakRefAccess(BaseMutex* mutex);
 
+  // Guards allocation entrypoint instrumenting.
+  static Mutex* instrument_entrypoints_lock_;
+
   // Guards code that deals with user-code suspension. This mutex must be held when suspending or
   // resuming threads with SuspendReason::kForUserCode. It may be held by a suspended thread, but
   // only if the suspension is not due to SuspendReason::kForUserCode.
-  static Mutex* user_code_suspension_lock_;
-
-  // Guards allocation entrypoint instrumenting.
-  static Mutex* instrument_entrypoints_lock_ ACQUIRED_AFTER(user_code_suspension_lock_);
+  static Mutex* user_code_suspension_lock_ ACQUIRED_AFTER(instrument_entrypoints_lock_);
 
   // A barrier is used to synchronize the GC/Debugger thread with mutator threads. When GC/Debugger
   // thread wants to suspend all mutator threads, it needs to wait for all mutator threads to pass
@@ -222,7 +217,7 @@
   //    state is changed                           |  .. running ..
   //  - if the CAS operation fails then goto x     |  .. running ..
   //  .. running ..                                |  .. running ..
-  static MutatorMutex* mutator_lock_ ACQUIRED_AFTER(instrument_entrypoints_lock_);
+  static MutatorMutex* mutator_lock_ ACQUIRED_AFTER(user_code_suspension_lock_);
 
   // Allow reader-writer mutual exclusion on the mark and live bitmaps of the heap.
   static ReaderWriterMutex* heap_bitmap_lock_ ACQUIRED_AFTER(mutator_lock_);