From c88bdf8a757be047938c3bb736491e3443f5519c Mon Sep 17 00:00:00 2001 From: Ruslan Tkhakokhov Date: Thu, 25 Aug 2022 15:31:08 +0100 Subject: [PATCH] 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 --- .../backup/UserBackupManagerService.java | 33 +++++++++++++------ .../backup/UserBackupManagerServiceTest.java | 23 +++++++++++++ 2 files changed, 46 insertions(+), 10 deletions(-) diff --git a/services/backup/java/com/android/server/backup/UserBackupManagerService.java b/services/backup/java/com/android/server/backup/UserBackupManagerService.java index ca7fe0c571d1a..14cfce7cc6792 100644 --- a/services/backup/java/com/android/server/backup/UserBackupManagerService.java +++ b/services/backup/java/com/android/server/backup/UserBackupManagerService.java @@ -3722,21 +3722,34 @@ public class UserBackupManagerService { Slog.w(TAG, "agentDisconnected: the backup agent for " + packageName + " died: cancel current operations"); - // 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 - // backup time-out elapses. - for (int token : mOperationStorage.operationTokensForPackage(packageName)) { - if (MORE_DEBUG) { - Slog.d(TAG, "agentDisconnected: will handleCancel(all) for token:" - + Integer.toHexString(token)); + // 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 + // backup time-out elapses. + for (int token : mOperationStorage.operationTokensForPackage(packageName)) { + if (MORE_DEBUG) { + Slog.d(TAG, "agentDisconnected: will handleCancel(all) for token:" + + Integer.toHexString(token)); + } + handleCancel(token, true /* cancelAll */); } - 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. diff --git a/services/tests/servicestests/src/com/android/server/backup/UserBackupManagerServiceTest.java b/services/tests/servicestests/src/com/android/server/backup/UserBackupManagerServiceTest.java index bccd8a0b14b48..9ae892286e55f 100644 --- a/services/tests/servicestests/src/com/android/server/backup/UserBackupManagerServiceTest.java +++ b/services/tests/servicestests/src/com/android/server/backup/UserBackupManagerServiceTest.java @@ -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()); + } + } } }