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

Commit c88bdf8a authored by Ruslan Tkhakokhov's avatar Ruslan Tkhakokhov
Browse files

Offload teardown logic after BackupAgent crash to a worker thread 

Teardown logic introduced in ag/16247901 is called on a foreground
thread in ActvitiyManager. The teardown itself might be time-consuming,
causing system_server to crash due to a blocked foreground thread (see
linked bug for more context).

This CL limits the scope of the fix to just the new logic that caused
the regression. Follow-up improvements should be made to ensure
time-consuming operations don't block foreground threads in the future
(b/242307900 will track).

Fixes: 213860448
Test: 1. atest UserBackupManagerServiceTest
      2. atest CtsBackupTestCases
      3. Kill app during full backup -> ensure agent is properly torn
	 down.
      4. Modify BackupTransport::cancelFullBackup() impl. to block; kill
	 app during full backup -> ensure foreground thread is not
	 blocked and agent is still properly torn down.
Change-Id: I6c1cb1ff71814564d5884a362135edfad3b258f2
parent e58df67a

services/backup/java/com/android/server/backup/UserBackupManagerService.java

+23 −10
Original line number Diff line number Diff line
@@ -3722,6 +3722,10 @@ public class UserBackupManagerService { 
            Slog.w(TAG, "agentDisconnected: the backup agent for " + packageName

                    + " died: cancel current operations");



            // Offload operation cancellation off the main thread as the cancellation callbacks

            // might call out to BackupTransport. Other operations started on the same package

            // before the cancellation callback has executed will also be cancelled by the callback.

            Runnable cancellationRunnable = () -> {

                // handleCancel() causes the PerformFullTransportBackupTask to go on to

                // tearDownAgentAndKill: that will unbindBackupAgent in the Activity Manager, so

                // that the package being backed up doesn't get stuck in restricted mode until the
@@ -3733,10 +3737,19 @@ public class UserBackupManagerService { 
                    }

                    handleCancel(token, true /* cancelAll */);

                }

            };

            getThreadForAsyncOperation(/* operationName */ "agent-disconnected",

                    cancellationRunnable).start();



            mAgentConnectLock.notifyAll();

        }

    }



    @VisibleForTesting

    Thread getThreadForAsyncOperation(String operationName, Runnable operation) {

        return new Thread(operation, operationName);

    }



    /**

     * An application being installed will need a restore pass, then the {@link PackageManager} will

     * need to be told when the restore is finished.

services/tests/servicestests/src/com/android/server/backup/UserBackupManagerServiceTest.java

+23 −0
Original line number Diff line number Diff line
@@ -18,6 +18,7 @@ package com.android.server.backup; 


import static com.google.common.truth.Truth.assertThat;



import static org.junit.Assert.fail;

import static org.mockito.ArgumentMatchers.any;

import static org.mockito.ArgumentMatchers.anyBoolean;

import static org.mockito.ArgumentMatchers.anyInt;
@@ -61,6 +62,7 @@ import java.util.function.IntConsumer; 
public class UserBackupManagerServiceTest {

    private static final String TEST_PACKAGE = "package1";

    private static final String[] TEST_PACKAGES = new String[] { TEST_PACKAGE };

    private static final int WORKER_THREAD_TIMEOUT_MILLISECONDS = 1;



    @Mock Context mContext;

    @Mock IBackupManagerMonitor mBackupManagerMonitor;
@@ -179,6 +181,7 @@ public class UserBackupManagerServiceTest { 


        mService.agentDisconnected("com.android.foo");



        mService.waitForAsyncOperation();

        verify(mOperationStorage).cancelOperation(eq(123), eq(true), any(IntConsumer.class));

        verify(mOperationStorage).cancelOperation(eq(456), eq(true), any());

        verify(mOperationStorage).cancelOperation(eq(789), eq(true), any());
@@ -207,6 +210,8 @@ public class UserBackupManagerServiceTest { 
        boolean isEnabledStatePersisted = false;

        boolean shouldUseNewBackupEligibilityRules = false;



        private volatile Thread mWorkerThread = null;



        TestBackupService(Context context, PackageManager packageManager,

                LifecycleOperationStorage operationStorage) {

            super(context, packageManager, operationStorage);
@@ -229,5 +234,23 @@ public class UserBackupManagerServiceTest { 
        boolean shouldUseNewBackupEligibilityRules() {

            return shouldUseNewBackupEligibilityRules;

        }



        @Override

        Thread getThreadForAsyncOperation(String operationName, Runnable operation) {

            mWorkerThread = super.getThreadForAsyncOperation(operationName, operation);

            return mWorkerThread;

        }



        private void waitForAsyncOperation() {

            if (mWorkerThread == null) {

                return;

            }



            try {

                mWorkerThread.join(/* millis */ WORKER_THREAD_TIMEOUT_MILLISECONDS);

            } catch (InterruptedException e) {

                fail("Failed waiting for worker thread to complete: " + e.getMessage());

            }

        }

    }

}