From e9ea91f47d14920a0a39f1d2cd6e63da7cc2b5d9 Mon Sep 17 00:00:00 2001
From: Robert Carr <racarr@google.com>
Date: Wed, 5 May 2021 17:36:47 -0700
Subject: [PATCH] SyncRtSurfaceTransactionApplier: Improve thread safety

If the calling thread releases the SurfaceControl passed
to SyncRtSurfaceTransactionApplier concurrently with the
applier preparing the transaction, this can lead to a
synchronization error and a crash. Once the state is
inside the transaction no further synchronization is required
as the native transaction will hold its own sp<SurfaceControl>
reference. By constructing the Transaction on the calling thread
and deferring application to the RenderThread we enable the calling
thread to not have any release synchronization requirements with
RenderThread.

Bug: 186391509
Change-Id: I585e1a9d3baf9ea384b00408b6253f34487d5037
---
 .../view/SyncRtSurfaceTransactionApplier.java   | 17 ++++++++++-------
 .../view/ViewRootInsetsControllerHost.java      |  4 +++-
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/core/java/android/view/SyncRtSurfaceTransactionApplier.java b/core/java/android/view/SyncRtSurfaceTransactionApplier.java
index 162c71bf1c16f..3e2110341693d 100644
--- a/core/java/android/view/SyncRtSurfaceTransactionApplier.java
+++ b/core/java/android/view/SyncRtSurfaceTransactionApplier.java
@@ -54,20 +54,21 @@ public class SyncRtSurfaceTransactionApplier {
     /**
      * Schedules applying surface parameters on the next frame.
      *
-     * @param params The surface parameters to apply. DO NOT MODIFY the list after passing into
-     *               this method to avoid synchronization issues.
+     * @param params The surface parameters to apply.
      */
     public void scheduleApply(final SurfaceParams... params) {
         if (mTargetViewRootImpl == null) {
             return;
         }
         mTargetSc = mTargetViewRootImpl.getSurfaceControl();
+        final Transaction t = new Transaction();
+        applyParams(t, params);
+
         mTargetViewRootImpl.registerRtFrameCallback(frame -> {
             if (mTargetSc == null || !mTargetSc.isValid()) {
                 return;
             }
-            Transaction t = new Transaction();
-            applyParams(t, frame, params);
+            applyTransaction(t, frame);
         });
 
         // Make sure a frame gets scheduled.
@@ -78,15 +79,17 @@ public class SyncRtSurfaceTransactionApplier {
      * Applies surface parameters on the next frame.
      * @param t transaction to apply all parameters in.
      * @param frame frame to synchronize to. Set -1 when sync is not required.
-     * @param params The surface parameters to apply. DO NOT MODIFY the list after passing into
-     *               this method to avoid synchronization issues.
+     * @param params The surface parameters to apply.
      */
-     void applyParams(Transaction t, long frame, final SurfaceParams... params) {
+     void applyParams(Transaction t, final SurfaceParams... params) {
         for (int i = params.length - 1; i >= 0; i--) {
             SurfaceParams surfaceParams = params[i];
             SurfaceControl surface = surfaceParams.surface;
             applyParams(t, surfaceParams, mTmpFloat9);
         }
+    }
+
+    void applyTransaction(Transaction t, long frame) {
         if (mTargetViewRootImpl != null) {
             mTargetViewRootImpl.mergeWithNextTransaction(t, frame);
         } else {
diff --git a/core/java/android/view/ViewRootInsetsControllerHost.java b/core/java/android/view/ViewRootInsetsControllerHost.java
index 514fb29029d08..d8cd6056de904 100644
--- a/core/java/android/view/ViewRootInsetsControllerHost.java
+++ b/core/java/android/view/ViewRootInsetsControllerHost.java
@@ -127,7 +127,9 @@ public class ViewRootInsetsControllerHost implements InsetsController.Host {
             // Window doesn't support hardware acceleration, no synchronization for now.
             // TODO(b/149342281): use mViewRoot.mSurface.getNextFrameNumber() to sync on every
             //  frame instead.
-            mApplier.applyParams(new SurfaceControl.Transaction(), -1 /* frame */, params);
+            final SurfaceControl.Transaction t = new SurfaceControl.Transaction();
+            mApplier.applyParams(t, params);
+            mApplier.applyTransaction(t, -1);
         }
     }