From ebaaca1a46b792d4d79f87d02059686ad30b4fa2 Mon Sep 17 00:00:00 2001
From: Robert Carr <racarr@google.com>
Date: Wed, 13 May 2020 11:23:43 -0700
Subject: [PATCH] SurfaceView: positionLost locking fix

positionLost can be called from CanvasContext::destroyHardwareResources
which runs asynchronously to the UI thread. This means we could be
simultaneously executing releaseSurfaces on the UI thread. We need
to expand the scope of mSurfaceControl lock in positionLost. While
we are here we add a block comment explaining the previously
undocumented locking strategy.

Bug: 156264048
Test: Existing tests pass
Change-Id: I9cdb6a0f7aeffd878f1755f240e8896f0fb8bf01
---
 core/java/android/view/SurfaceView.java | 37 +++++++++++++++++++------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/core/java/android/view/SurfaceView.java b/core/java/android/view/SurfaceView.java
index db6fe0f57d067..bd811fc1f0528 100644
--- a/core/java/android/view/SurfaceView.java
+++ b/core/java/android/view/SurfaceView.java
@@ -134,6 +134,23 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
     // we need to preserve the old one until the new one has drawn.
     SurfaceControl mDeferredDestroySurfaceControl;
     SurfaceControl mBackgroundControl;
+
+    /**
+     * We use this lock in SOME cases when reading or writing SurfaceControl,
+     * but use the following model so that the RenderThread can run locklessly
+     * in the position up-date case.
+     *
+     * 1. UI Thread can read from mSurfaceControl (use in Transactions) without
+     * holding the lock.
+     * 2. UI Thread will hold the lock when writing to mSurfaceControl (calling release
+     * or remove).
+     * 3. Render thread will also hold the lock when writing to mSurfaceControl (e.g.
+     * calling release from positionLost).
+     * 3. RenderNode.PositionUpdateListener::positionChanged will only be called
+     * when the UI thread is paused (blocked on the Render thread).
+     * 4. positionChanged thus will not be required to hold the lock as the
+     * UI thread is blocked, and the other writer is the RT itself.
+     */
     final Object mSurfaceControlLock = new Object();
     final Rect mTmpRect = new Rect();
 
@@ -1297,15 +1314,19 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
                 (viewRoot != null ? viewRoot.getBLASTSyncTransaction() : mRtTransaction) :
                 mRtTransaction;
 
-            if (frameNumber > 0 && viewRoot !=  null && !useBLAST) {
-                if (viewRoot.mSurface.isValid()) {
-                    mRtTransaction.deferTransactionUntil(mSurfaceControl,
-                            viewRoot.getRenderSurfaceControl(), frameNumber);
-                }
-            }
-            t.hide(mSurfaceControl);
-
+            /**
+             * positionLost can be called while UI thread is un-paused so we
+             * need to hold the lock here.
+             */
             synchronized (mSurfaceControlLock) {
+                if (frameNumber > 0 && viewRoot !=  null && !useBLAST) {
+                    if (viewRoot.mSurface.isValid()) {
+                        mRtTransaction.deferTransactionUntil(mSurfaceControl,
+                                viewRoot.getRenderSurfaceControl(), frameNumber);
+                    }
+                }
+                t.hide(mSurfaceControl);
+
                 if (mRtReleaseSurfaces) {
                     mRtReleaseSurfaces = false;
                     mRtTransaction.remove(mSurfaceControl);