Revert "[ VM ] Ensure shared timeline objects are protected by locks"

This reverts commit a62e144.

Reason for revert: Flutter engine shell tests are timing out because of deadlock.

Original change's description:
> [ VM ] Ensure shared timeline objects are protected by locks
>
> Timeline::Clear() could cause an user after free error if trying to allocate a timeline
> event immediately after clearing the timeline buffer.
>
> Timeline::Cleanup() could result in recorder_ being freed while timeline events are still
> being processed.
>
> Should fix flutter/flutter#78076
>
> TEST=tests/lib/developer/timeline_recorders_test.dart
>
> Change-Id: Ia61b9c93986788c79ef6f2ff1907625b66fb4ea1
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/191802
> Commit-Queue: Ben Konyi <[email protected]>
> Reviewed-by: Siva Annamalai <[email protected]>

# Not skipping CQ checks because original CL landed > 1 day ago.

Change-Id: Iac2c19147a3e73df37418e11106f0c5132a35302
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/192482
Reviewed-by: Siva Annamalai <[email protected]>
Reviewed-by: Ben Konyi <[email protected]>
Commit-Queue: Siva Annamalai <[email protected]>

Author: a-siva

runtime/vm/timeline.cc

@@ -86,8 +86,6 @@ DEFINE_FLAG(charp,
 //
 // Locking notes:
 // The following locks are used by the timeline system:
-// - |Timeline::recorder_lock_| This lock is held whenever Timeline::recorder()
-// is accessed or modified.
 // - |TimelineEventRecorder::lock_| This lock is held whenever a
 // |TimelineEventBlock| is being requested or reclaimed.
 // - |Thread::timeline_block_lock_| This lock is held whenever a |Thread|'s
@@ -96,10 +94,9 @@ DEFINE_FLAG(charp,
 // |Thread|s.
 //
 // Locks must always be taken in the following order:
-//   |Timeline::recorder_lock_|
-//     |Thread::thread_list_lock_|
-//       |Thread::timeline_block_lock_|
-//         |TimelineEventRecorder::lock_|
+//   |Thread::thread_list_lock_|
+//     |Thread::timeline_block_lock_|
+//       |TimelineEventRecorder::lock_|
 //
 
 static TimelineEventRecorder* CreateTimelineRecorder() {
@@ -208,10 +205,6 @@ static bool HasStream(MallocGrowableArray<char*>* streams, const char* stream) {
 }
 
 void Timeline::Init() {
-  if (recorder_lock_ == nullptr) {
-    recorder_lock_ = new Mutex();
-  }
-  MutexLocker ml(recorder_lock_);
   ASSERT(recorder_ == NULL);
   recorder_ = CreateTimelineRecorder();
   ASSERT(recorder_ != NULL);
@@ -224,8 +217,6 @@ void Timeline::Init() {
 }
 
 void Timeline::Cleanup() {
-  ASSERT(recorder_lock_ != nullptr);
-  MutexLocker ml(recorder_lock_);
   ASSERT(recorder_ != NULL);
 
 #ifndef PRODUCT
@@ -252,12 +243,6 @@ TimelineEventRecorder* Timeline::recorder() {
 }
 
 void Timeline::ReclaimCachedBlocksFromThreads() {
-  ASSERT(recorder_lock_ != nullptr);
-  MutexLocker ml(recorder_lock_);
-  ReclaimCachedBlocksFromThreadsLocked();
-}
-
-void Timeline::ReclaimCachedBlocksFromThreadsLocked() {
   TimelineEventRecorder* recorder = Timeline::recorder();
   if (recorder == NULL) {
     return;
@@ -289,8 +274,6 @@ void Timeline::PrintFlagsToJSONArray(JSONArray* arr) {
 }
 
 void Timeline::PrintFlagsToJSON(JSONStream* js) {
-  ASSERT(recorder_lock_ != nullptr);
-  MutexLocker ml(recorder_lock_);
   JSONObject obj(js);
   obj.AddProperty("type", "TimelineFlags");
   TimelineEventRecorder* recorder = Timeline::recorder();
@@ -318,13 +301,11 @@ void Timeline::PrintFlagsToJSON(JSONStream* js) {
 #endif
 
 void Timeline::Clear() {
-  ASSERT(recorder_lock_ != nullptr);
-  MutexLocker ml(recorder_lock_);
   TimelineEventRecorder* recorder = Timeline::recorder();
   if (recorder == NULL) {
     return;
   }
-  ReclaimCachedBlocksFromThreadsLocked();
+  ReclaimCachedBlocksFromThreads();
   recorder->Clear();
 }
 
@@ -409,7 +390,6 @@ void TimelineEventArguments::Free() {
 
 TimelineEventRecorder* Timeline::recorder_ = NULL;
 MallocGrowableArray<char*>* Timeline::enabled_streams_ = NULL;
-Mutex* Timeline::recorder_lock_ = nullptr;
 
 #define TIMELINE_STREAM_DEFINE(name, fuchsia_name)                             \
   TimelineStream Timeline::stream_##name##_(#name, fuchsia_name, false);
@@ -576,8 +556,6 @@ void TimelineEvent::FormatArgument(intptr_t i,
 }
 
 void TimelineEvent::Complete() {
-  ASSERT(Timeline::recorder_lock() != nullptr);
-  MutexLocker ml(Timeline::recorder_lock());
   TimelineEventRecorder* recorder = Timeline::recorder();
   if (recorder != NULL) {
     recorder->CompleteEvent(this);
@@ -797,10 +775,6 @@ TimelineStream::TimelineStream(const char* name,
 }
 
 TimelineEvent* TimelineStream::StartEvent() {
-  if (Timeline::recorder_lock() == nullptr) {
-    return nullptr;
-  }
-  MutexLocker ml(Timeline::recorder_lock());
   TimelineEventRecorder* recorder = Timeline::recorder();
   if (!enabled() || (recorder == NULL)) {
     return NULL;
@@ -1163,7 +1137,6 @@ TimelineEventFixedBufferRecorder::TimelineEventFixedBufferRecorder(
 }
 
 TimelineEventFixedBufferRecorder::~TimelineEventFixedBufferRecorder() {
-  MutexLocker ml(&lock_);
   // Delete all blocks.
   for (intptr_t i = 0; i < num_blocks_; i++) {
     blocks_[i].Reset();
@@ -1361,7 +1334,6 @@ TimelineEventEndlessRecorder::TimelineEventEndlessRecorder()
     : head_(nullptr), tail_(nullptr), block_index_(0) {}
 
 TimelineEventEndlessRecorder::~TimelineEventEndlessRecorder() {
-  MutexLocker ml(&lock_);
   TimelineEventBlock* current = head_;
   head_ = tail_ = nullptr;
 
@@ -1452,18 +1424,15 @@ void TimelineEventEndlessRecorder::PrintJSONEvents(
 #endif
 
 void TimelineEventEndlessRecorder::Clear() {
-  OSThread* thread = OSThread::Current();
-  MutexLocker ml(thread->timeline_block_lock());
-  MutexLocker ml2(&lock_);
   TimelineEventBlock* current = head_;
   while (current != NULL) {
     TimelineEventBlock* next = current->next();
     delete current;
     current = next;
   }
   head_ = NULL;
-  tail_ = NULL;
   block_index_ = 0;
+  OSThread* thread = OSThread::Current();
   thread->set_timeline_block(NULL);
 }
 

runtime/vm/timeline.h

@@ -23,7 +23,7 @@
 #if defined(__MAC_10_14) || defined (__IPHONE_12_0)
 #define HOST_OS_SUPPORTS_SIGNPOST 1
 #endif
-// signpost.h exists in macOS 10.14, iOS 12 or above
+//signpost.h exists in macOS 10.14, iOS 12 or above
 #if defined(HOST_OS_SUPPORTS_SIGNPOST)
 #include <os/signpost.h>
 #else
@@ -123,18 +123,15 @@ class TimelineStream {
 
 class Timeline : public AllStatic {
  public:
-  // Initialize timeline system.
+  // Initialize timeline system. Not thread safe.
   static void Init();
 
-  // Cleanup timeline system.
+  // Cleanup timeline system. Not thread safe.
   static void Cleanup();
 
-  // Access the global recorder. recorder_lock() must be held when accessing
-  // the returned |TimelineEventRecorder|.
+  // Access the global recorder. Not thread safe.
   static TimelineEventRecorder* recorder();
 
-  static Mutex* recorder_lock() { return recorder_lock_; }
-
   // Reclaim all |TimelineEventBlocks|s that are cached by threads.
   static void ReclaimCachedBlocksFromThreads();
 
@@ -161,13 +158,8 @@ class Timeline : public AllStatic {
 #undef TIMELINE_STREAM_FLAGS
 
  private:
-  // Reclaims all |TimelineEventBlocks|s that are cached by threads, assuming
-  // that lock_ is held.
-  static void ReclaimCachedBlocksFromThreadsLocked();
-
   static TimelineEventRecorder* recorder_;
   static MallocGrowableArray<char*>* enabled_streams_;
-  static Mutex* recorder_lock_;
 
 #define TIMELINE_STREAM_DECLARE(name, fuchsia_name)                            \
   static TimelineStream stream_##name##_;