Ack messages when WindowInfosListeners are removed am: 7da995f39e am: 87300af139

Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/native/+/24907058

Change-Id: I982d9199720039d277d06232444a7a8fe769dbec
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

3 files changed, 58 insertions, 15 deletions

diff --git a/services/surfaceflinger/WindowInfosListenerInvoker.cpp b/services/surfaceflinger/WindowInfosListenerInvoker.cpp
index 7062a4e3a7..effbfdb896 100644
--- a/services/surfaceflinger/WindowInfosListenerInvoker.cpp
+++ b/services/surfaceflinger/WindowInfosListenerInvoker.cpp
@@ -56,29 +56,33 @@ void WindowInfosListenerInvoker::removeWindowInfosListener(
         ATRACE_NAME("WindowInfosListenerInvoker::removeWindowInfosListener");
         sp<IBinder> asBinder = IInterface::asBinder(listener);
         asBinder->unlinkToDeath(sp<DeathRecipient>::fromExisting(this));
-        mWindowInfosListeners.erase(asBinder);
+        eraseListenerAndAckMessages(asBinder);
     }});
 }
 
 void WindowInfosListenerInvoker::binderDied(const wp<IBinder>& who) {
     BackgroundExecutor::getInstance().sendCallbacks({[this, who]() {
         ATRACE_NAME("WindowInfosListenerInvoker::binderDied");
-        auto it = mWindowInfosListeners.find(who);
-        int64_t listenerId = it->second.first;
-        mWindowInfosListeners.erase(who);
-
-        std::vector<int64_t> vsyncIds;
-        for (auto& [vsyncId, state] : mUnackedState) {
-            if (std::find(state.unackedListenerIds.begin(), state.unackedListenerIds.end(),
-                          listenerId) != state.unackedListenerIds.end()) {
-                vsyncIds.push_back(vsyncId);
-            }
-        }
+        eraseListenerAndAckMessages(who);
+    }});
+}
+
+void WindowInfosListenerInvoker::eraseListenerAndAckMessages(const wp<IBinder>& binder) {
+    auto it = mWindowInfosListeners.find(binder);
+    int64_t listenerId = it->second.first;
+    mWindowInfosListeners.erase(binder);
 
-        for (int64_t vsyncId : vsyncIds) {
-            ackWindowInfosReceived(vsyncId, listenerId);
+    std::vector<int64_t> vsyncIds;
+    for (auto& [vsyncId, state] : mUnackedState) {
+        if (std::find(state.unackedListenerIds.begin(), state.unackedListenerIds.end(),
+                      listenerId) != state.unackedListenerIds.end()) {
+            vsyncIds.push_back(vsyncId);
         }
-    }});
+    }
+
+    for (int64_t vsyncId : vsyncIds) {
+        ackWindowInfosReceived(vsyncId, listenerId);
+    }
 }
 
 void WindowInfosListenerInvoker::windowInfosChanged(
diff --git a/services/surfaceflinger/WindowInfosListenerInvoker.h b/services/surfaceflinger/WindowInfosListenerInvoker.h
index f36b0edd7d..261fd0ff3b 100644
--- a/services/surfaceflinger/WindowInfosListenerInvoker.h
+++ b/services/surfaceflinger/WindowInfosListenerInvoker.h
@@ -67,6 +67,7 @@ private:
 
     std::optional<gui::WindowInfosUpdate> mDelayedUpdate;
     WindowInfosReportedListenerSet mReportedListeners;
+    void eraseListenerAndAckMessages(const wp<IBinder>&);
 
     struct UnackedState {
         ftl::SmallVector<int64_t, kStaticCapacity> unackedListenerIds;
diff --git a/services/surfaceflinger/tests/unittests/WindowInfosListenerInvokerTest.cpp b/services/surfaceflinger/tests/unittests/WindowInfosListenerInvokerTest.cpp
index c7b845e668..cfb047c877 100644
--- a/services/surfaceflinger/tests/unittests/WindowInfosListenerInvokerTest.cpp
+++ b/services/surfaceflinger/tests/unittests/WindowInfosListenerInvokerTest.cpp
@@ -245,4 +245,42 @@ TEST_F(WindowInfosListenerInvokerTest, noListeners) {
     EXPECT_EQ(callCount, 1);
 }
 
+// Test that WindowInfosListenerInvoker#removeWindowInfosListener acks any unacked messages for
+// the removed listener.
+TEST_F(WindowInfosListenerInvokerTest, removeListenerAcks) {
+    // Don't ack in this listener to ensure there's an unacked message when the listener is later
+    // removed.
+    gui::WindowInfosListenerInfo listenerToBeRemovedInfo;
+    auto listenerToBeRemoved = sp<Listener>::make([](const gui::WindowInfosUpdate&) {});
+    mInvoker->addWindowInfosListener(listenerToBeRemoved, &listenerToBeRemovedInfo);
+
+    std::mutex mutex;
+    std::condition_variable cv;
+    int callCount = 0;
+    gui::WindowInfosListenerInfo listenerInfo;
+    mInvoker->addWindowInfosListener(sp<Listener>::make([&](const gui::WindowInfosUpdate& update) {
+                                         std::scoped_lock lock{mutex};
+                                         callCount++;
+                                         cv.notify_one();
+                                         listenerInfo.windowInfosPublisher
+                                                 ->ackWindowInfosReceived(update.vsyncId,
+                                                                          listenerInfo.listenerId);
+                                     }),
+                                     &listenerInfo);
+
+    BackgroundExecutor::getInstance().sendCallbacks(
+            {[&]() { mInvoker->windowInfosChanged({}, {}, false); }});
+    mInvoker->removeWindowInfosListener(listenerToBeRemoved);
+    BackgroundExecutor::getInstance().sendCallbacks(
+            {[&]() { mInvoker->windowInfosChanged({}, {}, false); }});
+
+    // Verify that the second listener is called twice. If unacked messages aren't removed when the
+    // first listener is removed, this will fail.
+    {
+        std::unique_lock lock{mutex};
+        cv.wait(lock, [&]() { return callCount == 2; });
+    }
+    EXPECT_EQ(callCount, 2);
+}
+
 } // namespace android