Merge "Ensure reportFrameMetrics not being called on deleted instance" into sc-dev

2 files changed, 20 insertions, 11 deletions

diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp
index 8af21d3c5d..660c5bd97d 100644
--- a/libs/gui/SurfaceComposerClient.cpp
+++ b/libs/gui/SurfaceComposerClient.cpp
@@ -210,13 +210,13 @@ void TransactionCompletedListener::removeReleaseBufferCallback(uint64_t graphicB
 
 void TransactionCompletedListener::addSurfaceStatsListener(void* context, void* cookie,
         sp<SurfaceControl> surfaceControl, SurfaceStatsCallback listener) {
-    std::lock_guard<std::mutex> lock(mMutex);
+    std::scoped_lock<std::recursive_mutex> lock(mSurfaceStatsListenerMutex);
     mSurfaceStatsListeners.insert({surfaceControl->getHandle(),
             SurfaceStatsCallbackEntry(context, cookie, listener)});
 }
 
 void TransactionCompletedListener::removeSurfaceStatsListener(void* context, void* cookie) {
-    std::lock_guard<std::mutex> lock(mMutex);
+    std::scoped_lock<std::recursive_mutex> lock(mSurfaceStatsListenerMutex);
     for (auto it = mSurfaceStatsListeners.begin(); it != mSurfaceStatsListeners.end();) {
         auto [itContext, itCookie, itListener] = it->second;
         if (itContext == context && itCookie == cookie) {
@@ -242,7 +242,6 @@ void TransactionCompletedListener::addSurfaceControlToCallbacks(
 
 void TransactionCompletedListener::onTransactionCompleted(ListenerStats listenerStats) {
     std::unordered_map<CallbackId, CallbackTranslation, CallbackIdHash> callbacksMap;
-    std::multimap<sp<IBinder>, SurfaceStatsCallbackEntry> surfaceListeners;
     {
         std::lock_guard<std::mutex> lock(mMutex);
 
@@ -258,7 +257,6 @@ void TransactionCompletedListener::onTransactionCompleted(ListenerStats listener
          * sp<SurfaceControl> that could possibly exist for the callbacks.
          */
         callbacksMap = mCallbacks;
-        surfaceListeners = mSurfaceStatsListeners;
         for (const auto& transactionStats : listenerStats.transactionStats) {
             for (auto& callbackId : transactionStats.callbackIds) {
                 mCallbacks.erase(callbackId);
@@ -339,11 +337,17 @@ void TransactionCompletedListener::onTransactionCompleted(ListenerStats listener
                              surfaceControlStats);
         }
         for (const auto& surfaceStats : transactionStats.surfaceStats) {
-            auto listenerRange = surfaceListeners.equal_range(surfaceStats.surfaceControl);
-            for (auto it = listenerRange.first; it != listenerRange.second; it++) {
-                auto entry = it->second;
-                entry.callback(entry.context, transactionStats.latchTime,
-                    transactionStats.presentFence, surfaceStats);
+            {
+                // Acquire surface stats listener lock such that we guarantee that after calling
+                // unregister, there won't be any further callback.
+                std::scoped_lock<std::recursive_mutex> lock(mSurfaceStatsListenerMutex);
+                auto listenerRange = mSurfaceStatsListeners.equal_range(
+                        surfaceStats.surfaceControl);
+                for (auto it = listenerRange.first; it != listenerRange.second; it++) {
+                    auto entry = it->second;
+                    entry.callback(entry.context, transactionStats.latchTime,
+                        transactionStats.presentFence, surfaceStats);
+                }
             }
 
             if (surfaceStats.jankData.empty()) continue;
diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h
index 28b4a3724e..5aa132cd92 100644
--- a/libs/gui/include/gui/SurfaceComposerClient.h
+++ b/libs/gui/include/gui/SurfaceComposerClient.h
@@ -655,6 +655,9 @@ class TransactionCompletedListener : public BnTransactionCompletedListener {
     // This lock needs to be recursive so we can unregister a callback from within that callback.
     std::recursive_mutex mJankListenerMutex;
 
+    // This lock needs to be recursive so we can unregister a callback from within that callback.
+    std::recursive_mutex mSurfaceStatsListenerMutex;
+
     bool mListening GUARDED_BY(mMutex) = false;
 
     int64_t mCallbackIdCounter GUARDED_BY(mMutex) = 1;
@@ -683,8 +686,10 @@ class TransactionCompletedListener : public BnTransactionCompletedListener {
     std::multimap<sp<IBinder>, sp<JankDataListener>> mJankListeners;
     std::unordered_map<uint64_t /* graphicsBufferId */, ReleaseBufferCallback>
             mReleaseBufferCallbacks GUARDED_BY(mMutex);
-    std::multimap<sp<IBinder>, SurfaceStatsCallbackEntry>
-                mSurfaceStatsListeners GUARDED_BY(mMutex);
+
+    // This is protected by mSurfaceStatsListenerMutex, but GUARDED_BY isn't supported for
+    // std::recursive_mutex
+    std::multimap<sp<IBinder>, SurfaceStatsCallbackEntry> mSurfaceStatsListeners;
 
 public:
     static sp<TransactionCompletedListener> getInstance();