From 4c769ee76a1f424b7b60c2e1c259649037ddcd5a Mon Sep 17 00:00:00 2001 From: Gerald Squelart Date: Fri, 17 Jul 2020 11:21:38 +0000 Subject: [PATCH] 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 | 3 + tools/profiler/gecko/ProfilerChild.cpp | 107 ++++++++++++++----------- tools/profiler/public/ProfilerChild.h | 12 ++- 3 files changed, 76 insertions(+), 46 deletions(-) diff --git a/tools/profiler/core/platform.cpp b/tools/profiler/core/platform.cpp index a887d3cf437a..e178346f3187 100644 --- a/tools/profiler/core/platform.cpp +++ b/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 diff --git a/tools/profiler/gecko/ProfilerChild.cpp b/tools/profiler/gecko/ProfilerChild.cpp index f64a64761e39..d7c8300ec712 100644 --- a/tools/profiler/gecko/ProfilerChild.cpp +++ b/tools/profiler/gecko/ProfilerChild.cpp @@ -14,6 +14,10 @@ namespace mozilla { +/* static */ StaticDataMutex + 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(this)]( - ProfileBufferControlledChunkManager::Update&& aUpdate) { - // Always dispatch, even if we're already on the `mThread`, to avoid - // reentrancy issues. - ProfilerChild* profilerChild = - static_cast(profilerChildPtr); - profilerChild->mThread->Dispatch(NS_NewRunnableFunction( - "ChunkManagerUpdate Callback", - [profilerChildPtr, update = std::move(aUpdate)]() mutable { - ProfilerChild* profilerChild = - static_cast(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); diff --git a/tools/profiler/public/ProfilerChild.h b/tools/profiler/public/ProfilerChild.h index 6002ab9575f2..39d467ac7dd2 100644 --- a/tools/profiler/public/ProfilerChild.h +++ b/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 mThread; @@ -69,6 +73,12 @@ class ProfilerChild final : public PProfilerChild, ProfileBufferControlledChunkManager* mChunkManager = nullptr; AwaitNextChunkManagerUpdateResolver mAwaitNextChunkManagerUpdateResolver; ProfileBufferControlledChunkManager::Update mChunkManagerUpdate; + + struct ProfilerChildAndUpdate { + RefPtr mProfilerChild; + ProfileBufferControlledChunkManager::Update mUpdate; + }; + static StaticDataMutex sPendingChunkManagerUpdate; }; } // namespace mozilla