From 38876847fecbf3e24ecdb4d9d7542aa6241ff0e0 Mon Sep 17 00:00:00 2001
From: Vishnu Nair <vishnun@google.com>
Date: Tue, 19 Jul 2022 08:14:11 -0700
Subject: [PATCH] SurfaceView: Ensure the Surface remains valid with
 SurfaceHolder#lockCanvas

Fixes an issue where the surface was destroyed while the caller had a
valid Surface. The SurfaceHolder#lockCanvas API states the
Surface remains valid until the caller unlocks the canvas. This cl
ensures the validity by acquiring the mSurfaceLock before changing
the state of the Surface.

Test: app in b/234006724 does not ANR
Test: app in b/235188096 does not crash
Test: atest SurfaceViewTest
Bug: 235188096
Change-Id: I67ae2b98b83cc148128bb7c70cf2180c927c0b04
---
 core/java/android/view/SurfaceView.java | 42 ++++++++++++++++++-------
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/core/java/android/view/SurfaceView.java b/core/java/android/view/SurfaceView.java
index 536a0ac6403b6..c7dbc07ef7c6c 100644
--- a/core/java/android/view/SurfaceView.java
+++ b/core/java/android/view/SurfaceView.java
@@ -119,6 +119,10 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
     final int[] mLocation = new int[2];
 
     @UnsupportedAppUsage
+    // Used to ensure the Surface remains valid between SurfaceHolder#lockCanvas and
+    // SurfaceHolder#unlockCanvasAndPost calls. This prevents SurfaceView from destroying or
+    // invalidating the Surface. This means this lock should be acquired when destroying the
+    // BlastBufferQueue.
     final ReentrantLock mSurfaceLock = new ReentrantLock();
     @UnsupportedAppUsage
     final Surface mSurface = new Surface();       // Current surface in use
@@ -724,14 +728,18 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
 
     private void releaseSurfaces(boolean releaseSurfacePackage) {
         mSurfaceAlpha = 1f;
-        mSurface.destroy();
-
-        synchronized (mSurfaceControlLock) {
+        try {
+            mSurfaceLock.lock();
+            mSurface.destroy();
             if (mBlastBufferQueue != null) {
                 mBlastBufferQueue.destroy();
                 mBlastBufferQueue = null;
             }
+        } finally {
+            mSurfaceLock.unlock();
+        }
 
+        synchronized (mSurfaceControlLock) {
             final Transaction transaction = new Transaction();
             if (mSurfaceControl != null) {
                 transaction.remove(mSurfaceControl);
@@ -1232,16 +1240,21 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
                     .build();
         }
 
-        // Always recreate the IGBP for compatibility. This can be optimized in the future but
-        // the behavior change will need to be gated by SDK version.
-        if (mBlastBufferQueue != null) {
-            mBlastBufferQueue.destroy();
-        }
         mTransformHint = viewRoot.getBufferTransformHint();
         mBlastSurfaceControl.setTransformHint(mTransformHint);
 
-        mBlastBufferQueue = new BLASTBufferQueue(name, false /* updateDestinationFrame */);
-        mBlastBufferQueue.update(mBlastSurfaceControl, mSurfaceWidth, mSurfaceHeight, mFormat);
+        // Always recreate the IGBP for compatibility. This can be optimized in the future but
+        // the behavior change will need to be gated by SDK version.
+        try {
+            mSurfaceLock.lock();
+            if (mBlastBufferQueue != null) {
+                mBlastBufferQueue.destroy();
+            }
+            mBlastBufferQueue = new BLASTBufferQueue(name, false /* updateDestinationFrame */);
+            mBlastBufferQueue.update(mBlastSurfaceControl, mSurfaceWidth, mSurfaceHeight, mFormat);
+        } finally {
+            mSurfaceLock.unlock();
+        }
         mBlastBufferQueue.setTransactionHangCallback(ViewRootImpl.sTransactionHangCallback);
     }
 
@@ -1814,7 +1827,14 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
             // so the next connect will always work if we end up reusing
             // the surface.
             if (mSurface.isValid()) {
-                mSurface.forceScopedDisconnect();
+                // We need to grab this lock since mSurface.forceScopedDisconnect
+                // will free buffers from the queue.
+                try {
+                    mSurfaceLock.lock();
+                    mSurface.forceScopedDisconnect();
+                } finally {
+                    mSurfaceLock.unlock();
+                }
             }
         }
     }