Fix crash from asynchronous GPU metrics

Making the scope more accurate that only acquire the lock when trying
to access frame info in FrameInfoVisualizer, then make it irrelevant
to the real draw operation.

Bug: 317995179
Test: 1.going to developer options
      2. swapping the "profile hwui" option from "none" to "bars"
          and back a couple times, no crash

Change-Id: I069a28a7e847c0c3fca94fd9c43e95382f501b80

7 files changed, 22 insertions, 22 deletions

diff --git a/libs/hwui/pipeline/skia/SkiaOpenGLPipeline.cpp b/libs/hwui/pipeline/skia/SkiaOpenGLPipeline.cpp
index c5ffbb70213e..c8d598702a7c 100644
--- a/libs/hwui/pipeline/skia/SkiaOpenGLPipeline.cpp
+++ b/libs/hwui/pipeline/skia/SkiaOpenGLPipeline.cpp
@@ -118,7 +118,7 @@ IRenderPipeline::DrawResult SkiaOpenGLPipeline::draw(
         const LightGeometry& lightGeometry, LayerUpdateQueue* layerUpdateQueue,
         const Rect& contentDrawBounds, bool opaque, const LightInfo& lightInfo,
         const std::vector<sp<RenderNode>>& renderNodes, FrameInfoVisualizer* profiler,
-        const HardwareBufferRenderParams& bufferParams) {
+        const HardwareBufferRenderParams& bufferParams, std::mutex& profilerLock) {
     if (!isCapturingSkp() && !mHardwareBuffer) {
         mEglManager.damageFrame(frame, dirty);
     }
@@ -171,6 +171,7 @@ IRenderPipeline::DrawResult SkiaOpenGLPipeline::draw(
     // Draw visual debugging features
     if (CC_UNLIKELY(Properties::showDirtyRegions ||
                     ProfileType::None != Properties::getProfileType())) {
+        std::scoped_lock lock(profilerLock);
         SkCanvas* profileCanvas = surface->getCanvas();
         SkiaProfileRenderer profileRenderer(profileCanvas, frame.width(), frame.height());
         profiler->draw(profileRenderer);
diff --git a/libs/hwui/pipeline/skia/SkiaOpenGLPipeline.h b/libs/hwui/pipeline/skia/SkiaOpenGLPipeline.h
index 098a74655831..ebe8b6e15d44 100644
--- a/libs/hwui/pipeline/skia/SkiaOpenGLPipeline.h
+++ b/libs/hwui/pipeline/skia/SkiaOpenGLPipeline.h
@@ -42,7 +42,8 @@ public:
             const LightGeometry& lightGeometry, LayerUpdateQueue* layerUpdateQueue,
             const Rect& contentDrawBounds, bool opaque, const LightInfo& lightInfo,
             const std::vector<sp<RenderNode> >& renderNodes, FrameInfoVisualizer* profiler,
-            const renderthread::HardwareBufferRenderParams& bufferParams) override;
+            const renderthread::HardwareBufferRenderParams& bufferParams,
+            std::mutex& profilerLock) override;
     GrSurfaceOrigin getSurfaceOrigin() override { return kBottomLeft_GrSurfaceOrigin; }
     bool swapBuffers(const renderthread::Frame& frame, IRenderPipeline::DrawResult& drawResult,
                      const SkRect& screenDirty, FrameInfo* currentFrameInfo,
diff --git a/libs/hwui/pipeline/skia/SkiaVulkanPipeline.cpp b/libs/hwui/pipeline/skia/SkiaVulkanPipeline.cpp
index d74748936d15..fd0a8e06f39c 100644
--- a/libs/hwui/pipeline/skia/SkiaVulkanPipeline.cpp
+++ b/libs/hwui/pipeline/skia/SkiaVulkanPipeline.cpp
@@ -75,7 +75,7 @@ IRenderPipeline::DrawResult SkiaVulkanPipeline::draw(
         const LightGeometry& lightGeometry, LayerUpdateQueue* layerUpdateQueue,
         const Rect& contentDrawBounds, bool opaque, const LightInfo& lightInfo,
         const std::vector<sp<RenderNode>>& renderNodes, FrameInfoVisualizer* profiler,
-        const HardwareBufferRenderParams& bufferParams) {
+        const HardwareBufferRenderParams& bufferParams, std::mutex& profilerLock) {
     sk_sp<SkSurface> backBuffer;
     SkMatrix preTransform;
     if (mHardwareBuffer) {
@@ -103,6 +103,7 @@ IRenderPipeline::DrawResult SkiaVulkanPipeline::draw(
     // Draw visual debugging features
     if (CC_UNLIKELY(Properties::showDirtyRegions ||
                     ProfileType::None != Properties::getProfileType())) {
+        std::scoped_lock lock(profilerLock);
         SkCanvas* profileCanvas = backBuffer->getCanvas();
         SkAutoCanvasRestore saver(profileCanvas, true);
         profileCanvas->concat(preTransform);
diff --git a/libs/hwui/pipeline/skia/SkiaVulkanPipeline.h b/libs/hwui/pipeline/skia/SkiaVulkanPipeline.h
index e2ea57d32cd5..624eaa51a584 100644
--- a/libs/hwui/pipeline/skia/SkiaVulkanPipeline.h
+++ b/libs/hwui/pipeline/skia/SkiaVulkanPipeline.h
@@ -42,7 +42,8 @@ public:
             const LightGeometry& lightGeometry, LayerUpdateQueue* layerUpdateQueue,
             const Rect& contentDrawBounds, bool opaque, const LightInfo& lightInfo,
             const std::vector<sp<RenderNode> >& renderNodes, FrameInfoVisualizer* profiler,
-            const renderthread::HardwareBufferRenderParams& bufferParams) override;
+            const renderthread::HardwareBufferRenderParams& bufferParams,
+            std::mutex& profilerLock) override;
     GrSurfaceOrigin getSurfaceOrigin() override { return kTopLeft_GrSurfaceOrigin; }
     bool swapBuffers(const renderthread::Frame& frame, IRenderPipeline::DrawResult& drawResult,
                      const SkRect& screenDirty, FrameInfo* currentFrameInfo,
diff --git a/libs/hwui/renderthread/CanvasContext.cpp b/libs/hwui/renderthread/CanvasContext.cpp
index 56b52dc2abe7..9c7f7cc24266 100644
--- a/libs/hwui/renderthread/CanvasContext.cpp
+++ b/libs/hwui/renderthread/CanvasContext.cpp
@@ -625,14 +625,9 @@ void CanvasContext::draw(bool solelyTextureViewUpdates) {
     {
         // FrameInfoVisualizer accesses the frame events, which cannot be mutated mid-draw
         // or it can lead to memory corruption.
-        // This lock is overly broad, but it's the quickest fix since this mutex is otherwise
-        // not visible to IRenderPipeline much less FrameInfoVisualizer. And since this is
-        // the thread we're primarily concerned about being responsive, this being too broad
-        // shouldn't pose a performance issue.
-        std::scoped_lock lock(mFrameMetricsReporterMutex);
-        drawResult = mRenderPipeline->draw(frame, windowDirty, dirty, mLightGeometry,
-                                           &mLayerUpdateQueue, mContentDrawBounds, mOpaque,
-                                           mLightInfo, mRenderNodes, &(profiler()), mBufferParams);
+        drawResult = mRenderPipeline->draw(
+                frame, windowDirty, dirty, mLightGeometry, &mLayerUpdateQueue, mContentDrawBounds,
+                mOpaque, mLightInfo, mRenderNodes, &(profiler()), mBufferParams, profilerLock());
     }
 
     uint64_t frameCompleteNr = getFrameNumber();
@@ -762,7 +757,7 @@ void CanvasContext::draw(bool solelyTextureViewUpdates) {
             mCurrentFrameInfo->markFrameCompleted();
             mCurrentFrameInfo->set(FrameInfoIndex::GpuCompleted)
                     = mCurrentFrameInfo->get(FrameInfoIndex::FrameCompleted);
-            std::scoped_lock lock(mFrameMetricsReporterMutex);
+            std::scoped_lock lock(mFrameInfoMutex);
             mJankTracker.finishFrame(*mCurrentFrameInfo, mFrameMetricsReporter, frameCompleteNr,
                                      mSurfaceControlGenerationId);
         }
@@ -791,7 +786,7 @@ void CanvasContext::draw(bool solelyTextureViewUpdates) {
 
 void CanvasContext::reportMetricsWithPresentTime() {
     {  // acquire lock
-        std::scoped_lock lock(mFrameMetricsReporterMutex);
+        std::scoped_lock lock(mFrameInfoMutex);
         if (mFrameMetricsReporter == nullptr) {
             return;
         }
@@ -826,7 +821,7 @@ void CanvasContext::reportMetricsWithPresentTime() {
 
     forthBehind->set(FrameInfoIndex::DisplayPresentTime) = presentTime;
     {  // acquire lock
-        std::scoped_lock lock(mFrameMetricsReporterMutex);
+        std::scoped_lock lock(mFrameInfoMutex);
         if (mFrameMetricsReporter != nullptr) {
             mFrameMetricsReporter->reportFrameMetrics(forthBehind->data(), true /*hasPresentTime*/,
                                                       frameNumber, surfaceControlId);
@@ -835,7 +830,7 @@ void CanvasContext::reportMetricsWithPresentTime() {
 }
 
 void CanvasContext::addFrameMetricsObserver(FrameMetricsObserver* observer) {
-    std::scoped_lock lock(mFrameMetricsReporterMutex);
+    std::scoped_lock lock(mFrameInfoMutex);
     if (mFrameMetricsReporter.get() == nullptr) {
         mFrameMetricsReporter.reset(new FrameMetricsReporter());
     }
@@ -849,7 +844,7 @@ void CanvasContext::addFrameMetricsObserver(FrameMetricsObserver* observer) {
 }
 
 void CanvasContext::removeFrameMetricsObserver(FrameMetricsObserver* observer) {
-    std::scoped_lock lock(mFrameMetricsReporterMutex);
+    std::scoped_lock lock(mFrameInfoMutex);
     if (mFrameMetricsReporter.get() != nullptr) {
         mFrameMetricsReporter->removeObserver(observer);
         if (!mFrameMetricsReporter->hasObservers()) {
@@ -886,7 +881,7 @@ void CanvasContext::onSurfaceStatsAvailable(void* context, int32_t surfaceContro
     FrameInfo* frameInfo = instance->getFrameInfoFromLast4(frameNumber, surfaceControlId);
 
     if (frameInfo != nullptr) {
-        std::scoped_lock lock(instance->mFrameMetricsReporterMutex);
+        std::scoped_lock lock(instance->mFrameInfoMutex);
         frameInfo->set(FrameInfoIndex::FrameCompleted) = std::max(gpuCompleteTime,
                 frameInfo->get(FrameInfoIndex::SwapBuffersCompleted));
         frameInfo->set(FrameInfoIndex::GpuCompleted) = std::max(
diff --git a/libs/hwui/renderthread/CanvasContext.h b/libs/hwui/renderthread/CanvasContext.h
index be9b649030ae..e2e3fa35b9b0 100644
--- a/libs/hwui/renderthread/CanvasContext.h
+++ b/libs/hwui/renderthread/CanvasContext.h
@@ -164,6 +164,7 @@ public:
     void notifyFramePending();
 
     FrameInfoVisualizer& profiler() { return mProfiler; }
+    std::mutex& profilerLock() { return mFrameInfoMutex; }
 
     void dumpFrames(int fd);
     void resetFrameStats();
@@ -342,9 +343,8 @@ private:
     std::string mName;
     JankTracker mJankTracker;
     FrameInfoVisualizer mProfiler;
-    std::unique_ptr<FrameMetricsReporter> mFrameMetricsReporter
-            GUARDED_BY(mFrameMetricsReporterMutex);
-    std::mutex mFrameMetricsReporterMutex;
+    std::unique_ptr<FrameMetricsReporter> mFrameMetricsReporter GUARDED_BY(mFrameInfoMutex);
+    std::mutex mFrameInfoMutex;
 
     std::set<RenderNode*> mPrefetchedLayers;
 
diff --git a/libs/hwui/renderthread/IRenderPipeline.h b/libs/hwui/renderthread/IRenderPipeline.h
index 9c879d5a58d1..b8c3a4de2bd4 100644
--- a/libs/hwui/renderthread/IRenderPipeline.h
+++ b/libs/hwui/renderthread/IRenderPipeline.h
@@ -68,7 +68,8 @@ public:
                             const Rect& contentDrawBounds, bool opaque, const LightInfo& lightInfo,
                             const std::vector<sp<RenderNode>>& renderNodes,
                             FrameInfoVisualizer* profiler,
-                            const HardwareBufferRenderParams& bufferParams) = 0;
+                            const HardwareBufferRenderParams& bufferParams,
+                            std::mutex& profilerLock) = 0;
     virtual bool swapBuffers(const Frame& frame, IRenderPipeline::DrawResult&,
                              const SkRect& screenDirty, FrameInfo* currentFrameInfo,
                              bool* requireSwap) = 0;