Bug 1666246 - Allow duplicate ThreadIds in the profiler's registered thread list. r=gerald, a=RyanVM DEVEDITION_82_0b2_BUILD1 DEVEDITION_82_0b2_RELEASE FIREFOX_82_0b2_BUILD1 FIREFOX_82_0b2_RELEASE
authorMarkus Stange <mstange.moz@gmail.com>
Mon, 21 Sep 2020 23:36:12 +0000 (2020-09-21)
changeset 614719 fe10fb7d3f3be59080d44226fb630bee503567c0
parent 614718 c8af1cf55dcdc33a220566d6d17c1de0f8a4aa7c
child 614720 a4feec2f431a0c448915455995838e9bedc40ee0
push id13972
push userryanvm@gmail.com
push dateTue, 22 Sep 2020 18:37:49 +0000 (2020-09-22)
treeherdermozilla-beta@fe10fb7d3f3b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgerald, RyanVM
bugs1666246
milestone82.0
Bug 1666246 - Allow duplicate ThreadIds in the profiler's registered thread list. r=gerald, a=RyanVM This patch makes us trust the TLS whenever we try to determine whether the current thread is already registered. It also removes assertions that assume that thread IDs can never be re-used without a proper unregistration of the old thread. Differential Revision: https://phabricator.services.mozilla.com/D90915
tools/profiler/core/platform.cpp
--- a/tools/profiler/core/platform.cpp
+++ b/tools/profiler/core/platform.cpp
@@ -3781,38 +3781,33 @@ uint32_t ParseFeaturesFromStringArray(co
                                       bool aIsStartup /* = false */) {
   uint32_t features = 0;
   for (size_t i = 0; i < aFeatureCount; i++) {
     features |= ParseFeature(aFeatures[i], aIsStartup);
   }
   return features;
 }
 
-// Find the RegisteredThread for the current thread. This should only be called
-// in places where TLSRegisteredThread can't be used.
-static RegisteredThread* FindCurrentThreadRegisteredThread(PSLockRef aLock) {
-  int id = profiler_current_thread_id();
-  const Vector<UniquePtr<RegisteredThread>>& registeredThreads =
-      CorePS::RegisteredThreads(aLock);
-  for (auto& registeredThread : registeredThreads) {
-    if (registeredThread->Info()->ThreadId() == id) {
-      return registeredThread.get();
+static bool IsRegisteredThreadInRegisteredThreadsList(
+    PSLockRef aLock, RegisteredThread* aThread) {
+  const auto& registeredThreads = CorePS::RegisteredThreads(aLock);
+  for (const auto& registeredThread : registeredThreads) {
+    if (registeredThread.get() == aThread) {
+      return true;
     }
   }
 
-  return nullptr;
+  return false;
 }
 
 static ProfilingStack* locked_register_thread(PSLockRef aLock,
                                               const char* aName,
                                               void* aStackTop) {
   MOZ_RELEASE_ASSERT(CorePS::Exists());
 
-  MOZ_ASSERT(!FindCurrentThreadRegisteredThread(aLock));
-
   VTUNE_REGISTER_THREAD(aName);
 
   if (!TLSRegisteredThread::IsTLSInited()) {
     return nullptr;
   }
 
   RefPtr<ThreadInfo> info =
       new ThreadInfo(aName, profiler_current_thread_id(), NS_IsMainThread());
@@ -5057,20 +5052,29 @@ ProfilingStack* profiler_register_thread
 
   MOZ_RELEASE_ASSERT(CorePS::Exists());
 
   // Make sure we have a nsThread wrapper for the current thread, and that NSPR
   // knows its name.
   (void)NS_GetCurrentThread();
   NS_SetCurrentThreadName(aName);
 
+  if (!TLSRegisteredThread::IsTLSInited()) {
+    return nullptr;
+  }
+
   PSAutoLock lock(gPSMutex);
 
-  if (RegisteredThread* thread = FindCurrentThreadRegisteredThread(lock);
-      thread) {
+  if (RegisteredThread* thread = TLSRegisteredThread::RegisteredThread(lock)) {
+    MOZ_RELEASE_ASSERT(IsRegisteredThreadInRegisteredThreadsList(lock, thread),
+                       "Thread being re-registered is not in registered thread "
+                       "list even though its TLS is non-null");
+    MOZ_RELEASE_ASSERT(
+        thread->Info()->ThreadId() == profiler_current_thread_id(),
+        "Thread being re-registered has changed its TID");
     LOG("profiler_register_thread(%s) - thread %d already registered as %s",
         aName, profiler_current_thread_id(), thread->Info()->Name());
     // TODO: Use new name. This is currently not possible because the
     // RegisteredThread's ThreadInfo cannot be changed.
     // In the meantime, we record a marker that could be used in the frontend.
     nsCString text("Thread ");
     text.AppendInt(profiler_current_thread_id());
     text.AppendLiteral(" \"");
@@ -5078,58 +5082,51 @@ ProfilingStack* profiler_register_thread
     text.AppendLiteral("\" attempted to re-register as \"");
     text.AppendASCII(aName);
     text.AppendLiteral("\"");
     maybelocked_profiler_add_marker_for_thread(
         CorePS::MainThreadId(), JS::ProfilingCategoryPair::OTHER_Profiling,
         "profiler_register_thread again",
         TextMarkerPayload(text, TimeStamp::NowUnfuzzed()), &lock);
 
-    MOZ_RELEASE_ASSERT(
-        TLSRegisteredThread::IsTLSInited(),
-        "Thread should not have already been registered without TLS::Init()");
-    MOZ_RELEASE_ASSERT(TLSRegisteredThread::RegisteredThread(lock),
-                       "TLS should be set when re-registering thread");
-    MOZ_RELEASE_ASSERT(
-        thread == TLSRegisteredThread::RegisteredThread(lock),
-        "TLS should be set as expected when re-registering thread");
-
     return &thread->RacyRegisteredThread().ProfilingStack();
   }
 
   void* stackTop = GetStackTop(aGuessStackTop);
   return locked_register_thread(lock, aName, stackTop);
 }
 
 void profiler_unregister_thread() {
   PSAutoLock lock(gPSMutex);
 
+  if (!TLSRegisteredThread::IsTLSInited()) {
+    return;
+  }
+
   if (!CorePS::Exists()) {
     // This function can be called after the main thread has already shut down.
     // We want to reset the AutoProfilerLabel's ProfilingStack pointer (if
     // needed), because a thread could stay registered after the profiler has
     // shut down.
     TLSRegisteredThread::ResetAutoProfilerLabelProfilingStack(lock);
     return;
   }
 
   // We don't call RegisteredThread::StopJSSampling() here; there's no point
   // doing that for a JS thread that is in the process of disappearing.
 
   if (RegisteredThread* registeredThread =
-          FindCurrentThreadRegisteredThread(lock);
-      registeredThread) {
+          TLSRegisteredThread::RegisteredThread(lock)) {
     MOZ_RELEASE_ASSERT(
-        TLSRegisteredThread::IsTLSInited(),
-        "Thread should not have been registered without TLS::Init()");
-    MOZ_RELEASE_ASSERT(TLSRegisteredThread::RegisteredThread(lock),
-                       "TLS should be set when un-registering thread");
+        IsRegisteredThreadInRegisteredThreadsList(lock, registeredThread),
+        "Thread being unregistered is not in registered thread list even "
+        "though its TLS is non-null");
     MOZ_RELEASE_ASSERT(
-        registeredThread == TLSRegisteredThread::RegisteredThread(lock),
-        "TLS should be set as expected when un-registering thread");
+        registeredThread->Info()->ThreadId() == profiler_current_thread_id(),
+        "Thread being unregistered has changed its TID");
     RefPtr<ThreadInfo> info = registeredThread->Info();
 
     DEBUG_LOG("profiler_unregister_thread: %s", info->Name());
 
     if (ActivePS::Exists(lock)) {
       ActivePS::UnregisterThread(lock, registeredThread);
     }
 
@@ -5137,46 +5134,46 @@ void profiler_unregister_thread() {
     // destroy, as well as the AutoProfilerLabel's ProfilingStack because the
     // thread is unregistering itself and won't need the ProfilingStack anymore.
     TLSRegisteredThread::ResetRegisteredThread(lock);
     TLSRegisteredThread::ResetAutoProfilerLabelProfilingStack(lock);
 
     // Remove the thread from the list of registered threads. This deletes the
     // registeredThread object.
     CorePS::RemoveRegisteredThread(lock, registeredThread);
-    MOZ_RELEASE_ASSERT(!FindCurrentThreadRegisteredThread(lock));
+
+    MOZ_RELEASE_ASSERT(
+        !IsRegisteredThreadInRegisteredThreadsList(lock, registeredThread),
+        "After unregistering, thread should no longer be in the registered "
+        "thread list");
     MOZ_RELEASE_ASSERT(
         !TLSRegisteredThread::RegisteredThread(lock),
         "TLS should have been reset after un-registering thread");
   } else {
+    // There are two ways TLSRegisteredThread::RegisteredThread() might be
+    // empty.
+    //
+    // - TLSRegisteredThread::Init() failed in locked_register_thread().
+    //
+    // - We've already called profiler_unregister_thread() for this thread.
+    //   (Whether or not it should, this does happen in practice.)
     LOG("profiler_unregister_thread() - thread %d already unregistered",
         profiler_current_thread_id());
     // We cannot record a marker on this thread because it was already
     // unregistered. Send it to the main thread (unless this *is* already the
     // main thread, which has been unregistered); this may be useful to catch
     // mismatched register/unregister pairs in Firefox.
     if (int tid = profiler_current_thread_id(); tid != CorePS::MainThreadId()) {
       nsCString threadIdString;
       threadIdString.AppendInt(tid);
       maybelocked_profiler_add_marker_for_thread(
           CorePS::MainThreadId(), JS::ProfilingCategoryPair::OTHER_Profiling,
           "profiler_unregister_thread again",
           TextMarkerPayload(threadIdString, TimeStamp::NowUnfuzzed()), &lock);
     }
-    // There are two ways FindCurrentThreadRegisteredThread() might have failed.
-    //
-    // - TLSRegisteredThread::Init() failed in locked_register_thread().
-    //
-    // - We've already called profiler_unregister_thread() for this thread.
-    //   (Whether or not it should, this does happen in practice.)
-    //
-    // Either way, TLSRegisteredThread should be empty.
-    MOZ_RELEASE_ASSERT(
-        !TLSRegisteredThread::RegisteredThread(lock),
-        "TLS should have been reset when thread was previously un-registered");
   }
 }
 
 void profiler_register_page(uint64_t aBrowsingContextID,
                             uint64_t aInnerWindowID, const nsCString& aUrl,
                             uint64_t aEmbedderInnerWindowID) {
   DEBUG_LOG("profiler_register_page(%" PRIu64 ", %" PRIu64 ", %s, %" PRIu64 ")",
             aBrowsingContextID, aInnerWindowID, aUrl.get(),