From 6c217f7b2ed05c1cd77f508d077ddccb607d28c1 Mon Sep 17 00:00:00 2001
From: Chavi Weingarten <chaviw@google.com>
Date: Thu, 15 Dec 2022 17:04:23 +0000
Subject: [PATCH] Don't re-add SurfaceSyncGroup to same parent

The scenario that caused the crash was the following:
1. SyncGroup C adds A so A's parent is now C
2. C adds B so B's parent is now C
3. New SyncGroup D adds A
4. A's parent is C so D needs to add C.
   C's old parent was null, new parent is now D.
5. D adds B
6. B's parent is C so D needs to add C. C's old parent was D, new parent
   is D. This causes the stack overflow.

Ensure that the old parent and new parent aren't the same when trying to
call newParent.addToSync(oldParent)

Test: Device that was crashing no longer does.
Test: SurfaceSyncGroupTest#testAddToSameParentNoCrash
Fixes: 262662555
Change-Id: Ifce471068c01711260042aee954f1e7a523d386e
---
 .../java/android/window/SurfaceSyncGroup.java | 15 ++++++++++---
 .../server/wm/SurfaceSyncGroupTest.java       | 21 +++++++++++++++++++
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/core/java/android/window/SurfaceSyncGroup.java b/core/java/android/window/SurfaceSyncGroup.java
index 250652a536779..c0686fa4164d4 100644
--- a/core/java/android/window/SurfaceSyncGroup.java
+++ b/core/java/android/window/SurfaceSyncGroup.java
@@ -18,6 +18,7 @@ package android.window;
 
 import android.annotation.Nullable;
 import android.annotation.UiThread;
+import android.os.Debug;
 import android.util.ArraySet;
 import android.util.Log;
 import android.util.Pair;
@@ -141,7 +142,7 @@ public class SurfaceSyncGroup {
         };
 
         if (DEBUG) {
-            Log.d(TAG, "setupSync");
+            Log.d(TAG, "setupSync " + this + " " + Debug.getCallers(2));
         }
     }
 
@@ -313,10 +314,18 @@ public class SurfaceSyncGroup {
                 // Additionally, the old parent will not get the final transaction object and
                 // instead will send it to the new parent, ensuring that any other SurfaceSyncGroups
                 // from the original parent are also combined with the new parent SurfaceSyncGroup.
-                if (mParentSyncGroup != null) {
-                    Log.d(TAG, "Already part of sync group " + mParentSyncGroup + " " + this);
+                if (mParentSyncGroup != null && mParentSyncGroup != parentSyncGroup) {
+                    if (DEBUG) {
+                        Log.d(TAG, "Already part of sync group " + mParentSyncGroup + " " + this);
+                    }
                     parentSyncGroup.addToSync(mParentSyncGroup, true /* parentSyncGroupMerge */);
                 }
+
+                if (mParentSyncGroup == parentSyncGroup) {
+                    if (DEBUG) {
+                        Log.d(TAG, "Added to parent that was already the parent");
+                    }
+                }
                 mParentSyncGroup = parentSyncGroup;
                 final TransactionReadyCallback lastCallback = mTransactionReadyCallback;
                 mTransactionReadyCallback = t -> {
diff --git a/services/tests/wmtests/src/com/android/server/wm/SurfaceSyncGroupTest.java b/services/tests/wmtests/src/com/android/server/wm/SurfaceSyncGroupTest.java
index 7d9f29c0a63db..266565946d93a 100644
--- a/services/tests/wmtests/src/com/android/server/wm/SurfaceSyncGroupTest.java
+++ b/services/tests/wmtests/src/com/android/server/wm/SurfaceSyncGroupTest.java
@@ -336,6 +336,27 @@ public class SurfaceSyncGroupTest {
         verify(parentTransaction).merge(targetTransaction);
     }
 
+    @Test
+    public void testAddToSameParentNoCrash() {
+        final CountDownLatch finishedLatch = new CountDownLatch(1);
+        SurfaceSyncGroup syncGroup = new SurfaceSyncGroup();
+        syncGroup.addSyncCompleteCallback(mExecutor, finishedLatch::countDown);
+        SyncTarget syncTarget = new SyncTarget();
+        syncGroup.addToSync(syncTarget, false /* parentSyncGroupMerge */);
+        // Add the syncTarget to the same syncGroup and ensure it doesn't crash.
+        syncGroup.addToSync(syncTarget, false /* parentSyncGroupMerge */);
+        syncGroup.onTransactionReady(null);
+
+        syncTarget.onBufferReady();
+
+        try {
+            finishedLatch.await(5, TimeUnit.SECONDS);
+        } catch (InterruptedException e) {
+            throw new RuntimeException(e);
+        }
+        assertEquals(0, finishedLatch.getCount());
+    }
+
     private static class SyncTarget extends SurfaceSyncGroup {
         void onBufferReady() {
             SurfaceControl.Transaction t = new StubTransaction();