WindowManager: Gate pending-transaction behind sync

Developers will reasonably assume that commands placed
in to sync or pending transaction in a given order will
reach the screen in that order. For example in this sequence:
	1. getSyncTransaction().hide(sc)
	2. Time passes
	3. getPendingTransaction.show(sc)
we would expect the SurfaceControl to end up visible. However
if the sync transaction were sent to SysUI but not yet applied
it could end up applied after, in which case the final state of
the SurfaceControl would be hidden. To solve this we listen
to the transaction committed callback for sync transactions
before sending the transaction to SysUI. In the interim
time period we gate the pending transaction on this callback.

Bug: 224438403
Bug: 227533765
Test: Existing tests pass
Change-Id: I5a99d68e134c3f10970f4635514c1a83456c4217

3 files changed, 143 insertions, 0 deletions

diff --git a/services/core/java/com/android/server/wm/BLASTSync.md b/services/core/java/com/android/server/wm/BLASTSync.md
index 2f39d6db385e..dbb28d4d5ae3 100644
--- a/services/core/java/com/android/server/wm/BLASTSync.md
+++ b/services/core/java/com/android/server/wm/BLASTSync.md
@@ -106,3 +106,88 @@ to the client via ClientTransaction), we haven't even incremented the seqId yet,
 at the same time as the state? We solve this by pushing all client communication through a handler thread that has to
 acquire the lock. This ensures we uphold requirement 2.
     
+= Transaction ordering =
+
+Applying transactions from different process, and in to different server side transaction queues
+raises various questions about transaction ordering. There are two tricky questions in this
+domain that we address here:
+    1. The ordering of Transactions from a single BLASTBufferQueue wrt to eachother
+    2. The ordering of non synced WM updates to syncable state, wrt a BLASTSyncEngine
+       transaction
+       
+== Ordering of Transactions in a single BBQ ==
+
+We can see if sync is never involved, there are never any real questions about ordering.
+Even using one-way setTransactionState, the calls are from a single thread to a single
+interface on another, and will be ordered. When we hand out transactions for sync
+is where issues can start to arise. Obviously if we apply another transaction
+immediately after handing out the sync transaction it could arrive first, and this would
+cause an ordering issue. It's also possible that the sync transaction arrives before the
+transaction applied before it (since setTransactionState is one way from BBQ). Even if
+the transactions are applied in the right order, it's possible for them to be
+commited out of sync, as the BBQ and SyncConsumer may be using different apply tokens
+resulting in the transaction being placed in different server side queues.
+
+To solve these issues, we use a scheme involving "barrier transactions". We show how
+this scheme handles every permutation of (Sync, NotSync).
+
+1. NotSync, NotSync: This is the trivial case. As long as both buffers are submitted from
+   the same thread, they will arrive in SF in order, and SF will place them in the same
+   queue (since there is a shared apply token), which it will process in order.
+2. Sync, NotSync: For sync transactions we register a commit callback, and require it to
+   be fired before applying the next transaction. Since the commit callback is only
+   fired when the transaction has latched on the server side, any transaction applied
+   later, must occur later.
+3. Sync, Sync: Ordering of consecutive sync transactions is delegated to the sync
+    consumer
+4. NotSync, Sync: This is the trickiest case, as we don't want to set a commit callback
+   on not sync transactions (as this would incur an unacceptable performance overhead).
+   Further complicating matters, we want to apply them with one-way binder, since
+   this is the hot path for all graphical updates. To solve this we use a
+   "setBufferHasBarrier" system. Sync transactions (before they are handed out)
+   are tagged with a barrier, referring to the frame number of the last
+   non sync transaction. SurfaceFlinger will ensure the correct ordering
+   by stalling transactions in the queue until barriers are fulfilled. This barrier
+   primitive is dangerous, because it could result in deadlocking, but its ok in
+   this scenario since only BBQ uses its apply token.
+   
+We can see from this that all frames are in order.
+
+== Ordering of WM updates to syncable state ==
+
+A second interesting question, is about the ordering of WM updates to syncable-state. In
+the WM we frequently write code like
+    getPendingTransaction/getSyncTransaction().show(mSurfaceControl).
+In normal operation, getSyncTransaction and getPendingTransaction both refer to the same
+transaction which is the displaycontent pendingtransaction, applied by the WindowManager.
+During sync, each syncing container will instead use its mSyncTransaction, which will
+eventually be merged to be applied by the WindowManager. We can see that we have a congruent
+"problem" to BLASTBufferQueue transaction ordering. Namely, earlier state updates which were
+given to SysUI could end up being applied later than state updates which were made later but
+directly applied by the WM. We can break this down in to two cases:
+=== getPendingTransaction() and getSyncTransaction() ordering ===
+It's possible for code like this to occur:
+getSyncTransaction().OP1
+getPendingTransaction().OP2
+applyPendingTransaction
+applySyncTransaction
+
+in this case the change in OP2 was made later, but occurs first. We define this case as
+intended behavior, and say there is no ordering guarantee between the pending
+and sync transactions. This means the pending transaction is only useful in
+some marginal cases and should probably be considered a deprecated primitive.
+=== getSyncTransaction() and getSyncTransaction() ordering ===
+However, we take great care to ensure usage of getSyncTransaction() reflects
+the ordering the developer would expect. BLASTSyncEngine will register
+a commit callback on all transactions it hands out. In the interval between
+receiving this commit callback and sending out the transaction, we may have other
+data enter getSyncTransaction. If we returned the pending transaction
+(as we do in normal time), then we could create ordering issues, since the pending
+transactions ordering is undefined. Instead we continue to return the sync transaction
+during this interval. If no second sync has started by the time we receive
+the commit callback, then we directly apply this left over data in the sync transaction
+guaranteed it will be ordered correctly, and return to using the pending
+transaction. If a second sync has started, then we just allow the data
+to persist in the mSyncTransaction, potentially to be overwritten
+by the new sync. It will eventually apply with SysUI's apply token and
+ordering will be maintained.
diff --git a/services/core/java/com/android/server/wm/BLASTSyncEngine.java b/services/core/java/com/android/server/wm/BLASTSyncEngine.java
index 6e205be5b574..3f300bce54c4 100644
--- a/services/core/java/com/android/server/wm/BLASTSyncEngine.java
+++ b/services/core/java/com/android/server/wm/BLASTSyncEngine.java
@@ -19,6 +19,7 @@ package com.android.server.wm;
 import static android.os.Trace.TRACE_TAG_WINDOW_MANAGER;
 
 import static com.android.internal.protolog.ProtoLogGroup.WM_DEBUG_SYNC_ENGINE;
+import static com.android.server.wm.WindowManagerService.H.WINDOW_STATE_BLAST_SYNC_TIMEOUT;
 
 import android.annotation.NonNull;
 import android.os.Trace;
@@ -147,6 +148,33 @@ class BLASTSyncEngine {
             for (WindowContainer wc : mRootMembers) {
                 wc.finishSync(merged, false /* cancel */);
             }
+
+            final ArraySet<WindowContainer> wcAwaitingCommit = new ArraySet<>();
+            for (WindowContainer wc : mRootMembers) {
+                wc.waitForSyncTransactionCommit(wcAwaitingCommit);
+            }
+            final Runnable callback = new Runnable() {
+                // Can run a second time if the action completes after the timeout.
+                boolean ran = false;
+                public void run() {
+                    synchronized (mWm.mGlobalLock) {
+                        if (ran) {
+                            return;
+                        }
+                        mWm.mH.removeCallbacks(this);
+                        ran = true;
+                        SurfaceControl.Transaction t = new SurfaceControl.Transaction();
+                        for (WindowContainer wc : wcAwaitingCommit) {
+                            wc.onSyncTransactionCommitted(t);
+                        }
+                        t.apply();
+                        wcAwaitingCommit.clear();
+                    }
+                }
+            };
+            merged.addTransactionCommittedListener((r) -> { r.run(); }, callback::run);
+            mWm.mH.postDelayed(callback, WINDOW_STATE_BLAST_SYNC_TIMEOUT);
+
             Trace.traceBegin(TRACE_TAG_WINDOW_MANAGER, "onTransactionReady");
             mListener.onTransactionReady(mSyncId, merged);
             Trace.traceEnd(TRACE_TAG_WINDOW_MANAGER);
diff --git a/services/core/java/com/android/server/wm/WindowContainer.java b/services/core/java/com/android/server/wm/WindowContainer.java
index 5b87463d889e..715930fac3e6 100644
--- a/services/core/java/com/android/server/wm/WindowContainer.java
+++ b/services/core/java/com/android/server/wm/WindowContainer.java
@@ -236,6 +236,8 @@ class WindowContainer<E extends WindowContainer> extends ConfigurationContainer<
      */
     private boolean mCommittedReparentToAnimationLeash;
 
+    private int mSyncTransactionCommitCallbackDepth = 0;
+
     /** Interface for {@link #isAnimating} to check which cases for the container is animating. */
     public interface AnimationFlags {
         /**
@@ -2688,6 +2690,9 @@ class WindowContainer<E extends WindowContainer> extends ConfigurationContainer<
      * {@link #getPendingTransaction()}
      */
     public Transaction getSyncTransaction() {
+        if (mSyncTransactionCommitCallbackDepth > 0) {
+            return mSyncTransaction;
+        }
         if (mSyncState != SYNC_STATE_NONE) {
             return mSyncTransaction;
         }
@@ -3906,4 +3911,29 @@ class WindowContainer<E extends WindowContainer> extends ConfigurationContainer<
             p.updateOverlayInsetsState(originalChange);
         }
     }
+
+    void waitForSyncTransactionCommit(ArraySet<WindowContainer> wcAwaitingCommit) {
+        if (wcAwaitingCommit.contains(this)) {
+            return;
+        }
+        mSyncTransactionCommitCallbackDepth++;
+        wcAwaitingCommit.add(this);
+
+        for (int i = mChildren.size() - 1; i >= 0; --i) {
+            mChildren.get(i).waitForSyncTransactionCommit(wcAwaitingCommit);
+        }
+    }
+
+    void onSyncTransactionCommitted(SurfaceControl.Transaction t) {
+        mSyncTransactionCommitCallbackDepth--;
+        if (mSyncTransactionCommitCallbackDepth > 0) {
+            return;
+        }
+        if (mSyncState != SYNC_STATE_NONE) {
+            return;
+        }
+
+        t.merge(mSyncTransaction);
+    }
+
 }