From 6608bfd58b26c1476caab3a0edfe11a125165807 Mon Sep 17 00:00:00 2001
From: Chavi Weingarten <chaviw@google.com>
Date: Wed, 19 Apr 2023 17:19:39 +0000
Subject: [PATCH] Ensure pendingTransactions are applied if the VRI is torn
 down

When callers invoke the applyTransactionOnDraw, the code previously set
up an RT frame callback and only merged the transaction once a draw
started. However, if VRI is being torn down before a traversal starts,
the callback will never be invoked and the transaction will be left in
memory until GC. The cleanest way is to immediately apply the
transaction when the VRI is getting torn down to avoid leaving a stale
transaction with reference to SCs in it.

This required a bit of cleanup to ensure the UI thread still had access
to the SurfaceControl until a traversal was called. The new behavior
merges all incoming transactions from applyTransactionOnDraw into a
single Transaction. Then in the traversal path, it will either
piggy-back on the frame callback from sync transaction or create a new
one. The Transaction and flag are cleared in the traversal path since if
draw has been invoked, we are guaranteed the callback even if VRI dies.
This is because HardwareRenderer is not destroyed until everything
in the queue completes.

Test: presubmit
Bug: 248162583
Change-Id: I4d28b917545f530a068b237bf95de431afd749cb
---
 core/java/android/view/ViewRootImpl.java | 36 ++++++++++++++++++------
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/core/java/android/view/ViewRootImpl.java b/core/java/android/view/ViewRootImpl.java
index 2b29e7878a5dc..5c6fe38227261 100644
--- a/core/java/android/view/ViewRootImpl.java
+++ b/core/java/android/view/ViewRootImpl.java
@@ -647,11 +647,18 @@ public final class ViewRootImpl implements ViewParent,
     boolean mForceNextWindowRelayout;
     CountDownLatch mWindowDrawCountDown;
 
-    // Whether we have used applyTransactionOnDraw to schedule an RT
-    // frame callback consuming a passed in transaction. In this case
-    // we also need to schedule a commit callback so we can observe
-    // if the draw was skipped, and the BBQ pending transactions.
+    /**
+     * Value to indicate whether someone has called {@link #applyTransactionOnDraw}before the
+     * traversal. This is used to determine whether a RT frame callback needs to be registered to
+     * merge the transaction with the next frame. The value is cleared after the VRI has run a
+     * traversal pass.
+     */
     boolean mHasPendingTransactions;
+    /**
+     * The combined transactions passed in from {@link #applyTransactionOnDraw}
+     */
+    private Transaction mPendingTransaction = new Transaction();
+
 
     boolean mIsDrawing;
     int mLastSystemUiVisibility;
@@ -4538,9 +4545,13 @@ public final class ViewRootImpl implements ViewParent,
     }
 
     private void registerCallbackForPendingTransactions() {
+        Transaction t = new Transaction();
+        t.merge(mPendingTransaction);
+
         registerRtFrameCallback(new FrameDrawingCallback() {
             @Override
             public HardwareRenderer.FrameCommitCallback onFrameDraw(int syncResult, long frame) {
+                mergeWithNextTransaction(t, frame);
                 if ((syncResult
                         & (SYNC_LOST_SURFACE_REWARD_IF_FOUND | SYNC_CONTEXT_IS_STOPPED)) != 0) {
                     mBlastBufferQueue.applyPendingTransactions(frame);
@@ -8770,6 +8781,9 @@ public final class ViewRootImpl implements ViewParent,
             mActiveSurfaceSyncGroup.markSyncReady();
             mActiveSurfaceSyncGroup = null;
         }
+        if (mHasPendingTransactions) {
+            mPendingTransaction.apply();
+        }
         WindowManagerGlobal.getInstance().doRemoveView(this);
     }
 
@@ -11104,12 +11118,11 @@ public final class ViewRootImpl implements ViewParent,
         } else {
             // Copy and clear the passed in transaction for thread safety. The new transaction is
             // accessed on the render thread.
-            var localTransaction = new Transaction();
-            localTransaction.merge(t);
+            mPendingTransaction.merge(t);
             mHasPendingTransactions = true;
-            registerRtFrameCallback(frame -> {
-                mergeWithNextTransaction(localTransaction, frame);
-            });
+            // Schedule the traversal to ensure there's an attempt to draw a frame and apply the
+            // pending transactions. This is also where the registerFrameCallback will be scheduled.
+            scheduleTraversals();
         }
         return true;
     }
@@ -11250,6 +11263,10 @@ public final class ViewRootImpl implements ViewParent,
         if (DEBUG_BLAST) {
             Log.d(mTag, "registerCallbacksForSync syncBuffer=" + syncBuffer);
         }
+
+        Transaction t = new Transaction();
+        t.merge(mPendingTransaction);
+
         mAttachInfo.mThreadedRenderer.registerRtFrameCallback(new FrameDrawingCallback() {
             @Override
             public void onFrameDraw(long frame) {
@@ -11263,6 +11280,7 @@ public final class ViewRootImpl implements ViewParent,
                                     + frame + ".");
                 }
 
+                mergeWithNextTransaction(t, frame);
                 // If the syncResults are SYNC_LOST_SURFACE_REWARD_IF_FOUND or
                 // SYNC_CONTEXT_IS_STOPPED it means nothing will draw. There's no need to set up
                 // any blast sync or commit callback, and the code should directly call