[VM/Timeline] Prevent races between reading / modifying the value of Timeline::recorder_

TEST=Checked that the problem described in the GitHub issue has been resolved.

Fixes https://github.com/dart-lang/sdk/issues/51408
Change-Id: I688d32a7cdd81b4f877e212d4f1376735013175c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/283141
Commit-Queue: Derek Xu <derekx@google.com>
Reviewed-by: Ben Konyi <bkonyi@google.com>

runtime/vm/os_thread.cc

@@ -25,6 +25,7 @@ thread_local ThreadState* OSThread::current_vm_thread_ = NULL;
 #if defined(SUPPORT_TIMELINE)
 inline void UpdateTimelineTrackMetadata(const OSThread& thread) {
   RecorderShutdownSynchronizationLockScope ls;
+  MutexLocker ml(Timeline::recorder_lock());
   TimelineEventRecorder* recorder = Timeline::recorder();
   if (recorder != nullptr && !ls.IsShuttingDown()) {
     recorder->AddTrackMetadataBasedOnThread(

runtime/vm/timeline.cc

@@ -235,7 +235,19 @@ static bool HasStream(MallocGrowableArray<char*>* streams, const char* stream) {
 void Timeline::Init() {
   RecorderShutdownSynchronizationLock::Init();
   ASSERT(recorder_ == NULL);
-  recorder_ = CreateTimelineRecorder();
+  {
+    MutexLocker ml(Timeline::recorder_lock());
+    recorder_ = CreateTimelineRecorder();
+  }
+  // The following is needed to backfill information about any |OSThread|s that
+  // were initialized before this point.
+  OSThreadIterator it;
+  while (it.HasNext()) {
+    OSThread& thread = *it.Next();
+    recorder_->AddTrackMetadataBasedOnThread(
+        OS::ProcessId(), OSThread::ThreadIdToIntPtr(thread.trace_id()),
+        thread.name());
+  }
   if (FLAG_trace_timeline) {
     OS::PrintErr("Using the %s timeline recorder.\n", recorder_->name());
   }
@@ -438,6 +450,7 @@ void TimelineEventArguments::Free() {
   length_ = 0;
 }
 
+Mutex* Timeline::recorder_lock_ = new Mutex();
 TimelineEventRecorder* Timeline::recorder_ = NULL;
 Dart_TimelineRecorderCallback Timeline::callback_ = NULL;
 MallocGrowableArray<char*>* Timeline::enabled_streams_ = NULL;
@@ -1071,17 +1084,7 @@ TimelineEventRecorder::TimelineEventRecorder()
       track_uuid_to_track_metadata_(
           &SimpleHashMap::SamePointerValue,
           TimelineEventRecorder::kTrackUuidToTrackMetadataInitialCapacity),
-      track_uuid_to_track_metadata_lock_() {
-  // The following is needed to backfill information about any |OSThread|s that
-  // were initialized before this point.
-  OSThreadIterator it;
-  while (it.HasNext()) {
-    OSThread& thread = *it.Next();
-    AddTrackMetadataBasedOnThread(OS::ProcessId(),
-                                  OSThread::ThreadIdToIntPtr(thread.trace_id()),
-                                  thread.name());
-  }
-}
+      track_uuid_to_track_metadata_lock_() {}
 
 TimelineEventRecorder::~TimelineEventRecorder() {
   for (SimpleHashMap::Entry* entry = track_uuid_to_track_metadata_.Start();

runtime/vm/timeline.h

@@ -193,6 +193,10 @@ class Timeline : public AllStatic {
   // Cleanup timeline system. Not thread safe.
   static void Cleanup();
 
+  // Used to prevent races between reading / modifying the value of
+  // |Timeline::recorder_|.
+  static Mutex* recorder_lock() { return recorder_lock_; }
+
   // Access the global recorder. Not thread safe.
   static TimelineEventRecorder* recorder() { return recorder_; }
 
@@ -237,6 +241,12 @@ class Timeline : public AllStatic {
   static void ClearUnsafe();
   static void ReclaimCachedBlocksFromThreadsUnsafe();
 
+  // |recorder_lock_| is used in |OSThread|'s constructor, so it must not be
+  // destroyed until |OSThread|s can no longer be created. Therefore, there is
+  // currently no opportunity to delete this lock. If a thread only begins to
+  // run after we have started to run TLS destructors for a call to |exit()|,
+  // there will be a race involving this lock's deletion.
+  static Mutex* recorder_lock_;
   static TimelineEventRecorder* recorder_;
   static Dart_TimelineRecorderCallback callback_;
   static MallocGrowableArray<char*>* enabled_streams_;