commit 2cafd8b44d0b4c13b16fdcd2557352ec5496fcc3
author Vishnu Nair <vishnun@google.com> Thu Sep 09 19:56:40 2021 -0700
committer Vishnu Nair <vishnun@google.com> Thu Sep 09 19:57:24 2021 -0700
tree 69af91c2c5deb0bc5393cecc315a53bd54d76e67
parent 8818f98f336edd7568eaa0404a7081b9f7c24a4b

SurfaceView: Fix SurfaceControl synchronization issues

Hold the surface control lock in PositionUpdateListener
callbacks before checking if the SurfaceControl will
be null. UI tread might release it after the checks
causing crashes.

Also fix a case where the ViewRootImpl may be null
in the positionChanged callback.

Bug: 199261027
Test: run steps in bug
Test: go/wm-smoke
Change-Id: I5a4cac35fe14356389b29268fddf9703b25c03aa

core/java/android/view/SurfaceView.java

diff --git a/core/java/android/view/SurfaceView.java b/core/java/android/view/SurfaceView.java
index a4e7a43..841557b 100644
--- a/core/java/android/view/SurfaceView.java
+++ b/core/java/android/view/SurfaceView.java
@@ -1464,19 +1464,18 @@
 
         @Override
         public void positionChanged(long frameNumber, int left, int top, int right, int bottom) {
-            if (mSurfaceControl == null) {
-                return;
-            }
-
-            // TODO: This is teensy bit racey in that a brand new SurfaceView moving on
-            // its 2nd frame if RenderThread is running slowly could potentially see
-            // this as false, enter the branch, get pre-empted, then this comes along
-            // and reports a new position, then the UI thread resumes and reports
-            // its position. This could therefore be de-sync'd in that interval, but
-            // the synchronization would violate the rule that RT must never block
-            // on the UI thread which would open up potential deadlocks. The risk of
-            // a single-frame desync is therefore preferable for now.
             synchronized(mSurfaceControlLock) {
+                if (mSurfaceControl == null) {
+                    return;
+                }
+                // TODO: This is teensy bit racey in that a brand new SurfaceView moving on
+                // its 2nd frame if RenderThread is running slowly could potentially see
+                // this as false, enter the branch, get pre-empted, then this comes along
+                // and reports a new position, then the UI thread resumes and reports
+                // its position. This could therefore be de-sync'd in that interval, but
+                // the synchronization would violate the rule that RT must never block
+                // on the UI thread which would open up potential deadlocks. The risk of
+                // a single-frame desync is therefore preferable for now.
                 mRtHandlingPositionUpdates = true;
             }
             if (mRTLastReportedPosition.left == left
@@ -1506,8 +1505,11 @@
                 if (mViewVisibility) {
                     mPositionChangedTransaction.show(mSurfaceControl);
                 }
-                applyChildSurfaceTransaction_renderWorker(mPositionChangedTransaction,
-                        getViewRootImpl().mSurface, frameNumber);
+                final ViewRootImpl viewRoot = getViewRootImpl();
+                if (viewRoot != null) {
+                    applyChildSurfaceTransaction_renderWorker(mPositionChangedTransaction,
+                            viewRoot.mSurface, frameNumber);
+                }
                 applyOrMergeTransaction(mPositionChangedTransaction, frameNumber);
                 mPendingTransaction = false;
             } catch (Exception ex) {
@@ -1528,7 +1530,7 @@
 
         @Override
         public void positionLost(long frameNumber) {
-            if (DEBUG) {
+            if (DEBUG_POSITION) {
                 Log.d(TAG, String.format("%d windowPositionLost, frameNr = %d",
                         System.identityHashCode(this), frameNumber));
             }
@@ -1540,15 +1542,15 @@
                 mPositionChangedTransaction.clear();
                 mPendingTransaction = false;
             }
-            if (mSurfaceControl == null) {
-                return;
-            }
 
             /**
              * positionLost can be called while UI thread is un-paused so we
              * need to hold the lock here.
              */
             synchronized (mSurfaceControlLock) {
+                if (mSurfaceControl == null) {
+                    return;
+                }
                 mRtTransaction.hide(mSurfaceControl);
                 if (mRtReleaseSurfaces) {
                     mRtReleaseSurfaces = false;