Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 6c217f7b authored by Chavi Weingarten's avatar Chavi Weingarten
Browse files

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
parent 912f380c

core/java/android/window/SurfaceSyncGroup.java

+12 −3
Original line number Diff line number Diff line
@@ -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) {

                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 -> {

services/tests/wmtests/src/com/android/server/wm/SurfaceSyncGroupTest.java

+21 −0
Original line number Diff line number Diff line
@@ -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();