Destroy DisplayDevice without holding the state lock

This is a temporary solution until we can manage transaction queues
without holding the mStateLock.

With blast, the IGBP that is passed to the VirtualDisplaySurface is
owned by the client. When the IGBP is disconnected, its buffer cache
in SF will be cleared via
SurfaceComposerClient::doUncacheBufferTransaction.

This call from the client ends up running on the main thread causing
a deadlock since setTransactionstate will try to acquire the mStateLock.
Instead we extend the lifetime of DisplayDevice and destroy it in the
main thread without holding the mStateLock. The display will be
disconnected and removed from the mDisplays list so it will not be
accessible.

Bug: 168917217
Test: adb shell settings put global use_blast_adapter_sv 1 && atest android.server.wm.MultiDisplayActivityLaunchTests
Change-Id: I8b94277cf2e807d47d6a61e4aafd8c2f183e6e7c

2 files changed, 20 insertions, 1 deletions

diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 95c99829ca..e7bee49911 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -2559,7 +2559,8 @@ void SurfaceFlinger::processDisplayAdded(const wp<IBinder>& displayToken,
 }
 
 void SurfaceFlinger::processDisplayRemoved(const wp<IBinder>& displayToken) {
-    if (const auto display = getDisplayDeviceLocked(displayToken)) {
+    auto display = getDisplayDeviceLocked(displayToken);
+    if (display) {
         display->disconnect();
         if (!display->isVirtual()) {
             dispatchDisplayHotplugEvent(display->getPhysicalId(), false);
@@ -2567,6 +2568,22 @@ void SurfaceFlinger::processDisplayRemoved(const wp<IBinder>& displayToken) {
     }
 
     mDisplays.erase(displayToken);
+
+    if (display && display->isVirtual()) {
+        static_cast<void>(schedule([display = std::move(display)] {
+            // Destroy the display without holding the mStateLock.
+            // This is a temporary solution until we can manage transaction queues without
+            // holding the mStateLock.
+            // With blast, the IGBP that is passed to the VirtualDisplaySurface is owned by the
+            // client. When the IGBP is disconnected, its buffer cache in SF will be cleared
+            // via SurfaceComposerClient::doUncacheBufferTransaction. This call from the client
+            // ends up running on the main thread causing a deadlock since setTransactionstate
+            // will try to acquire the mStateLock. Instead we extend the lifetime of
+            // DisplayDevice and destroy it in the main thread without holding the mStateLock.
+            // The display will be disconnected and removed from the mDisplays list so it will
+            // not be accessible.
+        }));
+    }
 }
 
 void SurfaceFlinger::processDisplayChanged(const wp<IBinder>& displayToken,
diff --git a/services/surfaceflinger/tests/VirtualDisplay_test.cpp b/services/surfaceflinger/tests/VirtualDisplay_test.cpp
index 9fd2227bba..18e08062c0 100644
--- a/services/surfaceflinger/tests/VirtualDisplay_test.cpp
+++ b/services/surfaceflinger/tests/VirtualDisplay_test.cpp
@@ -52,6 +52,8 @@ TEST_F(VirtualDisplayTest, VirtualDisplayDestroyedSurfaceReuse) {
     virtualDisplay.clear();
     // Sync here to ensure the display was completely destroyed in SF
     t.apply(true);
+    // add another sync since we are deferring the display destruction
+    t.apply(true);
 
     sp<Surface> surface = new Surface(mProducer);
     sp<ANativeWindow> window(surface);