From 3cb3e9a49e1240e46ae306b1266ce2cb1bbf5eaa Mon Sep 17 00:00:00 2001
From: Derek Xu <derekx@google.com>
Date: Mon, 10 Jul 2023 20:33:33 +0000
Subject: [PATCH] [VM/Timeline] Delete unsafe versions of Clear() and
 ReclaimCachedBlocksFromThreads()

TEST=Recorded traces using the file recorder on ASAN and TSAN builds.

Change-Id: I2251a8373d952d6e3ef1803e9a79de85522f518a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/312720
Reviewed-by: Ben Konyi <bkonyi@google.com>
---
 runtime/vm/timeline.cc | 24 +++++-------------------
 runtime/vm/timeline.h  | 15 ++++++++++-----
 2 files changed, 15 insertions(+), 24 deletions(-)

diff --git a/runtime/vm/timeline.cc b/runtime/vm/timeline.cc
index 5acfc8145a3..a9c212a4680 100644
--- a/runtime/vm/timeline.cc
+++ b/runtime/vm/timeline.cc
@@ -126,7 +126,7 @@ DEFINE_FLAG(charp,
 
 std::atomic<RecorderSynchronizationLock::RecorderState>
     RecorderSynchronizationLock::recorder_state_ = {
-        RecorderSynchronizationLock::kUnInitialized};
+        RecorderSynchronizationLock::kUninitialized};
 std::atomic<intptr_t> RecorderSynchronizationLock::outstanding_event_writes_ = {
     0};
 
@@ -316,11 +316,7 @@ void Timeline::Cleanup() {
   TIMELINE_STREAM_LIST(TIMELINE_STREAM_DISABLE)
 #undef TIMELINE_STREAM_DISABLE
   RecorderSynchronizationLock::WaitForShutdown();
-  // Timeline::Clear() is guarded by the recorder lock and will return
-  // immediately if we've started the shutdown sequence, leaking the recorder.
-  // All outstanding work has already been completed, so we're safe to call this
-  // without explicitly grabbing a recorder lock.
-  Timeline::ClearUnsafe();
+  Timeline::Clear();
   delete recorder_;
   recorder_ = nullptr;
   if (enabled_streams_ != nullptr) {
@@ -332,14 +328,9 @@ void Timeline::Cleanup() {
 void Timeline::ReclaimCachedBlocksFromThreads() {
   RecorderSynchronizationLockScope ls;
   TimelineEventRecorder* recorder = Timeline::recorder();
-  if (recorder == nullptr || !ls.IsActive()) {
+  if (recorder == nullptr || ls.IsUninitialized()) {
     return;
   }
-  ReclaimCachedBlocksFromThreadsUnsafe();
-}
-
-void Timeline::ReclaimCachedBlocksFromThreadsUnsafe() {
-  TimelineEventRecorder* recorder = Timeline::recorder();
   ASSERT(recorder != nullptr);
   // Iterate over threads.
   OSThreadIterator it;
@@ -397,16 +388,11 @@ void Timeline::PrintFlagsToJSON(JSONStream* js) {
 void Timeline::Clear() {
   RecorderSynchronizationLockScope ls;
   TimelineEventRecorder* recorder = Timeline::recorder();
-  if (recorder == nullptr || !ls.IsActive()) {
+  if (recorder == nullptr || ls.IsUninitialized()) {
     return;
   }
-  ClearUnsafe();
-}
-
-void Timeline::ClearUnsafe() {
-  TimelineEventRecorder* recorder = Timeline::recorder();
   ASSERT(recorder != nullptr);
-  ReclaimCachedBlocksFromThreadsUnsafe();
+  ReclaimCachedBlocksFromThreads();
   recorder->Clear();
 }
 
diff --git a/runtime/vm/timeline.h b/runtime/vm/timeline.h
index 7d9d91780ea..6c8711f94db 100644
--- a/runtime/vm/timeline.h
+++ b/runtime/vm/timeline.h
@@ -164,6 +164,10 @@ class RecorderSynchronizationLock : public AllStatic {
     ASSERT(count >= 0);
   }
 
+  static bool IsUninitialized() {
+    return (recorder_state_.load(std::memory_order_acquire) == kUninitialized);
+  }
+
   static bool IsActive() {
     return (recorder_state_.load(std::memory_order_acquire) == kActive);
   }
@@ -176,7 +180,7 @@ class RecorderSynchronizationLock : public AllStatic {
   }
 
  private:
-  typedef enum { kUnInitialized = 0, kActive, kShuttingDown } RecorderState;
+  typedef enum { kUninitialized = 0, kActive, kShuttingDown } RecorderState;
   static std::atomic<RecorderState> recorder_state_;
   static std::atomic<intptr_t> outstanding_event_writes_;
 
@@ -196,7 +200,11 @@ class RecorderSynchronizationLockScope {
     RecorderSynchronizationLock::ExitLock();
   }
 
-  bool IsActive() { return RecorderSynchronizationLock::IsActive(); }
+  bool IsUninitialized() const {
+    return RecorderSynchronizationLock::IsUninitialized();
+  }
+
+  bool IsActive() const { return RecorderSynchronizationLock::IsActive(); }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(RecorderSynchronizationLockScope);
@@ -251,9 +259,6 @@ class Timeline : public AllStatic {
 #undef TIMELINE_STREAM_FLAGS
 
  private:
-  static void ClearUnsafe();
-  static void ReclaimCachedBlocksFromThreadsUnsafe();
-
   static TimelineEventRecorder* recorder_;
   static Dart_TimelineRecorderCallback callback_;
   static MallocGrowableArray<char*>* enabled_streams_;