Bug 1651102 - Safely delay handling of child profile buffer updates - r=canaltinova

Profile buffer updates could be triggered from a number of locations, including scopes where profiler and/or system locks are held, making deadlocks possible if profiler and/or system function are called.
So instead of dispatching updates to the main thread (which may use OS task queue functions), we fold updates into a static storage. The profiler sampler loop regularly triggers processing of these pending updates.

Differential Revision: https://phabricator.services.mozilla.com/D83747

tools/profiler/core/platform.cpp

@@ -34,6 +34,7 @@
 #include "ProfileBuffer.h"
 #include "ProfiledThreadData.h"
 #include "ProfilerBacktrace.h"
+#include "ProfilerChild.h"
 #include "ProfilerCodeAddressService.h"
 #include "ProfilerIOInterposeObserver.h"
 #include "ProfilerMarkerPayload.h"
@@ -3572,6 +3573,8 @@ void SamplerThread::Run() {
     InvokePostSamplingCallbacks(std::move(postSamplingCallbacks),
                                 samplingState);
 
+    ProfilerChild::ProcessPendingUpdate();
+
     // Calculate how long a sleep to request.  After the sleep, measure how
     // long we actually slept and take the difference into account when
     // calculating the sleep interval for the next iteration.  This is an

tools/profiler/gecko/ProfilerChild.cpp

@@ -14,6 +14,10 @@
 
 namespace mozilla {
 
+/* static */ StaticDataMutex<ProfilerChild::ProfilerChildAndUpdate>
+    ProfilerChild::sPendingChunkManagerUpdate{
+        "ProfilerChild::sPendingChunkManagerUpdate"};
+
 ProfilerChild::ProfilerChild()
     : mThread(NS_GetCurrentThread()), mDestroyed(false) {
   MOZ_COUNT_CTOR(ProfilerChild);
@@ -66,7 +70,7 @@ void ProfilerChild::ResolveChunkUpdate(
   aResolve = nullptr;
 }
 
-void ProfilerChild::ChunkManagerUpdateCallback(
+void ProfilerChild::ProcessChunkManagerUpdate(
     ProfileBufferControlledChunkManager::Update&& aUpdate) {
   if (mDestroyed) {
     return;
@@ -79,63 +83,68 @@ void ProfilerChild::ChunkManagerUpdateCallback(
   }
 }
 
+/* static */ void ProfilerChild::ProcessPendingUpdate() {
+  auto lockedUpdate = sPendingChunkManagerUpdate.Lock();
+  if (!lockedUpdate->mProfilerChild || lockedUpdate->mUpdate.IsNotUpdate()) {
+    return;
+  }
+  lockedUpdate->mProfilerChild->mThread->Dispatch(NS_NewRunnableFunction(
+      "ProfilerChild::ProcessPendingUpdate", []() mutable {
+        auto lockedUpdate = sPendingChunkManagerUpdate.Lock();
+        if (!lockedUpdate->mProfilerChild ||
+            lockedUpdate->mUpdate.IsNotUpdate()) {
+          return;
+        }
+        lockedUpdate->mProfilerChild->ProcessChunkManagerUpdate(
+            std::move(lockedUpdate->mUpdate));
+        lockedUpdate->mUpdate.Clear();
+      }));
+}
+
 void ProfilerChild::SetupChunkManager() {
   mChunkManager = profiler_get_controlled_chunk_manager();
   if (NS_WARN_IF(!mChunkManager)) {
     return;
   }
 
-  // The update may be in any state from a previous profiling session.
-  // In case there is already a task in-flight with an update from that previous
-  // session, we need to dispatch a task to clear the update afterwards, but
-  // before the first update which will be dispatched from SetUpdateCallback()
-  // below.
-  mThread->Dispatch(NS_NewRunnableFunction(
-      "ChunkManagerUpdate Callback", [profilerChild = RefPtr(this)]() mutable {
-        profilerChild->mChunkManagerUpdate.Clear();
-      }));
+  // Make sure there are no updates (from a previous run).
+  mChunkManagerUpdate.Clear();
+  {
+    auto lockedUpdate = sPendingChunkManagerUpdate.Lock();
+    lockedUpdate->mProfilerChild = this;
+    lockedUpdate->mUpdate.Clear();
+  }
 
-  // `ProfilerChild` should only be used on its `mThread`.
-  // But the chunk manager update callback may happen on any thread, so we need
-  // to manually keep the `ProfilerChild` alive until after the final update has
-  // been handled on `mThread`.
-  // Using manual AddRef/Release, because ref-counting is single-threaded, so we
-  // cannot have a RefPtr stored in the callback which will be destroyed in
-  // another thread. The callback (where `Release()` happens) is guaranteed to
-  // always be called, at the latest when the calback is reset during shutdown.
-  AddRef();
   mChunkManager->SetUpdateCallback(
-      // Cast to `void*` to evade refcounted security!
-      [profilerChildPtr = static_cast<void*>(this)](
-          ProfileBufferControlledChunkManager::Update&& aUpdate) {
-        // Always dispatch, even if we're already on the `mThread`, to avoid
-        // reentrancy issues.
-        ProfilerChild* profilerChild =
-            static_cast<ProfilerChild*>(profilerChildPtr);
-        profilerChild->mThread->Dispatch(NS_NewRunnableFunction(
-            "ChunkManagerUpdate Callback",
-            [profilerChildPtr, update = std::move(aUpdate)]() mutable {
-              ProfilerChild* profilerChild =
-                  static_cast<ProfilerChild*>(profilerChildPtr);
-              const bool isFinal = update.IsFinal();
-              profilerChild->ChunkManagerUpdateCallback(std::move(update));
-              if (isFinal) {
-                profilerChild->Release();
-              }
-            }));
+      [](ProfileBufferControlledChunkManager::Update&& aUpdate) {
+        // Updates from the chunk manager are stored for later processing.
+        // We avoid dispatching a task, as this could deadlock (if the queueing
+        // mutex is held elsewhere).
+        auto lockedUpdate = sPendingChunkManagerUpdate.Lock();
+        if (!lockedUpdate->mProfilerChild) {
+          return;
+        }
+        lockedUpdate->mUpdate.Fold(std::move(aUpdate));
       });
 }
 
 void ProfilerChild::ResetChunkManager() {
-  if (mChunkManager) {
-    // We have a chunk manager, reset the callback, which will send a final
-    // update.
-    mChunkManager->SetUpdateCallback({});
-  } else if (!mChunkManagerUpdate.IsFinal()) {
-    // No chunk manager, just make sure the update as final now.
-    mChunkManagerUpdate.Fold(
-        ProfileBufferControlledChunkManager::Update(nullptr));
+  if (!mChunkManager) {
+    return;
   }
+
+  // We have a chunk manager, reset the callback, which will add a final
+  // pending update.
+  mChunkManager->SetUpdateCallback({});
+
+  // Clear the pending update.
+  auto lockedUpdate = sPendingChunkManagerUpdate.Lock();
+  lockedUpdate->mProfilerChild = nullptr;
+  lockedUpdate->mUpdate.Clear();
+  // And process a final update right now.
+  ProcessChunkManagerUpdate(
+      ProfileBufferControlledChunkManager::Update(nullptr));
+
   mChunkManager = nullptr;
   mAwaitNextChunkManagerUpdateResolver = nullptr;
 }
@@ -222,6 +231,14 @@ mozilla::ipc::IPCResult ProfilerChild::RecvAwaitNextChunkManagerUpdate(
     AwaitNextChunkManagerUpdateResolver&& aResolve) {
   MOZ_ASSERT(!mDestroyed,
              "Recv... should not be called if the actor was destroyed");
+  // Pick up pending updates if any.
+  {
+    auto lockedUpdate = sPendingChunkManagerUpdate.Lock();
+    if (lockedUpdate->mProfilerChild && !lockedUpdate->mUpdate.IsNotUpdate()) {
+      mChunkManagerUpdate.Fold(std::move(lockedUpdate->mUpdate));
+      lockedUpdate->mUpdate.Clear();
+    }
+  }
   if (mChunkManagerUpdate.IsNotUpdate()) {
     // No data yet, store the resolver for later.
     mAwaitNextChunkManagerUpdateResolver = std::move(aResolve);

tools/profiler/public/ProfilerChild.h

@@ -7,6 +7,7 @@
 #ifndef ProfilerChild_h
 #define ProfilerChild_h
 
+#include "mozilla/DataMutex.h"
 #include "mozilla/PProfilerChild.h"
 #include "mozilla/ProfileBufferControlledChunkManager.h"
 #include "mozilla/RefPtr.h"
@@ -33,6 +34,9 @@ class ProfilerChild final : public PProfilerChild,
 
   void Destroy();
 
+  // This should be called regularly from outside of the profiler lock.
+  static void ProcessPendingUpdate();
+
  private:
   virtual ~ProfilerChild();
 
@@ -60,7 +64,7 @@ class ProfilerChild final : public PProfilerChild,
   void ResetChunkManager();
   void ResolveChunkUpdate(
       PProfilerChild::AwaitNextChunkManagerUpdateResolver& aResolve);
-  void ChunkManagerUpdateCallback(
+  void ProcessChunkManagerUpdate(
       ProfileBufferControlledChunkManager::Update&& aUpdate);
 
   nsCOMPtr<nsIThread> mThread;
@@ -69,6 +73,12 @@ class ProfilerChild final : public PProfilerChild,
   ProfileBufferControlledChunkManager* mChunkManager = nullptr;
   AwaitNextChunkManagerUpdateResolver mAwaitNextChunkManagerUpdateResolver;
   ProfileBufferControlledChunkManager::Update mChunkManagerUpdate;
+
+  struct ProfilerChildAndUpdate {
+    RefPtr<ProfilerChild> mProfilerChild;
+    ProfileBufferControlledChunkManager::Update mUpdate;
+  };
+  static StaticDataMutex<ProfilerChildAndUpdate> sPendingChunkManagerUpdate;
 };
 
 }  // namespace mozilla