Merge "SurfaceView: Fix unsafe transaction accesses" into sc-v2-dev am: 830bf8bd34

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

Change-Id: I83169eaa386d4a4620b838832852bec4b14d7489

core/java/android/view/SurfaceView.java

@@ -138,20 +138,9 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
     private boolean mDisableBackgroundLayer = false;
 
     /**
-     * We use this lock in SOME cases when reading or writing SurfaceControl,
+     * We use this lock to protect access to mSurfaceControl and 
-     * but use the following model so that the RenderThread can run locklessly
+     * SurfaceViewPositionUpdateListener#mPositionChangedTransaction. Both are accessed on the UI
-     * in the position up-date case.
+     * thread and the render thread.
-     *
-     * 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();
@@ -945,8 +934,10 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
             Transaction geometryTransaction) {
         if (mPositionListener != null) {
             mRenderNode.removePositionUpdateListener(mPositionListener);
+            synchronized (mSurfaceControlLock) {
                 geometryTransaction = mPositionListener.getTransaction().merge(geometryTransaction);
             }
+        }
         mPositionListener = new SurfaceViewPositionUpdateListener(surfaceWidth, surfaceHeight,
                 geometryTransaction);
         mRenderNode.addPositionUpdateListener(mPositionListener);
@@ -1467,7 +1458,6 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
                 if (mSurfaceControl == null) {
                     return;
                 }
-            }
                 if (mRTLastReportedPosition.left == left
                         && mRTLastReportedPosition.top == top
                         && mRTLastReportedPosition.right == right
@@ -1490,8 +1480,10 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
                     onSetSurfacePositionAndScaleRT(mPositionChangedTransaction, mSurfaceControl,
                             mRTLastReportedPosition.left /*positionLeft*/,
                             mRTLastReportedPosition.top /*positionTop*/,
-                        mRTLastReportedPosition.width() / (float) mRtSurfaceWidth /*postScaleX*/,
+                            mRTLastReportedPosition.width()
-                        mRTLastReportedPosition.height() / (float) mRtSurfaceHeight /*postScaleY*/);
+                                    / (float) mRtSurfaceWidth /*postScaleX*/,
+                            mRTLastReportedPosition.height()
+                                    / (float) mRtSurfaceHeight /*postScaleY*/);
                     if (mViewVisibility) {
                         mPositionChangedTransaction.show(mSurfaceControl);
                     }
@@ -1506,6 +1498,7 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
                     Log.e(TAG, "Exception from repositionChild", ex);
                 }
             }
+        }
 
         @Override
         public void applyStretch(long frameNumber, float width, float height,
@@ -1526,18 +1519,18 @@ public class SurfaceView extends View implements ViewRootImpl.SurfaceChangedCall
             }
             mRTLastReportedPosition.setEmpty();
             mRTLastReportedSurfaceSize.set(-1, -1);
-            if (mPendingTransaction) {
-                Log.w(TAG, System.identityHashCode(SurfaceView.this)
-                        + "Pending transaction cleared.");
-                mPositionChangedTransaction.clear();
-                mPendingTransaction = false;
-            }
 
             /**
              * positionLost can be called while UI thread is un-paused so we
              * need to hold the lock here.
              */
             synchronized (mSurfaceControlLock) {
+                if (mPendingTransaction) {
+                    Log.w(TAG, System.identityHashCode(SurfaceView.this)
+                            + "Pending transaction cleared.");
+                    mPositionChangedTransaction.clear();
+                    mPendingTransaction = false;
+                }
                 if (mSurfaceControl == null) {
                     return;
                 }