Reduce lock interactions in backup transport management

1. process package update broadcasts on our background thread rather
   than on the main looper thread

2. don't synchronize unnecessarily around access to simple
   transport metadata

We mustn't block the main looper thread for anything that might wind
up interlocked with calls to the transport, because those might take
arbitrary amounts of time.  We were previously entering such an
implicitly interlocked code path during package-changed broadcast
handling, and in pathological cases were causing the watchdog to
restart the system.  This situation is addressed in a couple of ways:
first, by no longer performing package-update work on the main looper
thread at all; and second, by eliminating lock reliance entirely from
data-access paths that don't actually need it.

Bug: 65438129
Bug: 64133971
Test: manual + CTS
Change-Id: I361ad4a0729f319db7339bd341a6d33aa3b64fed

3 files changed, 33 insertions, 28 deletions

diff --git a/services/backup/java/com/android/server/backup/BackupManagerService.java b/services/backup/java/com/android/server/backup/BackupManagerService.java
index 2f31fc34a135..f4dbb5a9b0b2 100644
--- a/services/backup/java/com/android/server/backup/BackupManagerService.java
+++ b/services/backup/java/com/android/server/backup/BackupManagerService.java
@@ -1987,7 +1987,7 @@ public class BackupManagerService implements BackupManagerServiceInterface {
                 if (uri == null) {
                     return;
                 }
-                String pkgName = uri.getSchemeSpecificPart();
+                final String pkgName = uri.getSchemeSpecificPart();
                 if (pkgName != null) {
                     pkgList = new String[] { pkgName };
                 }
@@ -1995,7 +1995,7 @@ public class BackupManagerService implements BackupManagerServiceInterface {
 
                 // At package-changed we only care about looking at new transport states
                 if (changed) {
-                    String[] components =
+                    final String[] components =
                             intent.getStringArrayExtra(Intent.EXTRA_CHANGED_COMPONENT_NAME_LIST);
 
                     if (MORE_DEBUG) {
@@ -2005,7 +2005,8 @@ public class BackupManagerService implements BackupManagerServiceInterface {
                         }
                     }
 
-                    mTransportManager.onPackageChanged(pkgName, components);
+                    mBackupHandler.post(
+                            () -> mTransportManager.onPackageChanged(pkgName, components));
                     return; // nothing more to do in the PACKAGE_CHANGED case
                 }
 
@@ -2037,7 +2038,7 @@ public class BackupManagerService implements BackupManagerServiceInterface {
                 }
                 // If they're full-backup candidates, add them there instead
                 final long now = System.currentTimeMillis();
-                for (String packageName : pkgList) {
+                for (final String packageName : pkgList) {
                     try {
                         PackageInfo app = mPackageManager.getPackageInfo(packageName, 0);
                         if (appGetsFullBackup(app)
@@ -2054,7 +2055,8 @@ public class BackupManagerService implements BackupManagerServiceInterface {
                             writeFullBackupScheduleAsync();
                         }
 
-                        mTransportManager.onPackageAdded(packageName);
+                        mBackupHandler.post(
+                                () -> mTransportManager.onPackageAdded(packageName));
 
                     } catch (NameNotFoundException e) {
                         // doesn't really exist; ignore it
@@ -2078,8 +2080,9 @@ public class BackupManagerService implements BackupManagerServiceInterface {
                         removePackageParticipantsLocked(pkgList, uid);
                     }
                 }
-                for (String pkgName : pkgList) {
-                    mTransportManager.onPackageRemoved(pkgName);
+                for (final String pkgName : pkgList) {
+                    mBackupHandler.post(
+                            () -> mTransportManager.onPackageRemoved(pkgName));
                 }
             }
         }
diff --git a/services/backup/java/com/android/server/backup/RefactoredBackupManagerService.java b/services/backup/java/com/android/server/backup/RefactoredBackupManagerService.java
index 8b4cc7f6d22b..14a008e0852f 100644
--- a/services/backup/java/com/android/server/backup/RefactoredBackupManagerService.java
+++ b/services/backup/java/com/android/server/backup/RefactoredBackupManagerService.java
@@ -1188,7 +1188,7 @@ public class RefactoredBackupManagerService implements BackupManagerServiceInter
                 if (uri == null) {
                     return;
                 }
-                String pkgName = uri.getSchemeSpecificPart();
+                final String pkgName = uri.getSchemeSpecificPart();
                 if (pkgName != null) {
                     pkgList = new String[]{pkgName};
                 }
@@ -1196,7 +1196,7 @@ public class RefactoredBackupManagerService implements BackupManagerServiceInter
 
                 // At package-changed we only care about looking at new transport states
                 if (changed) {
-                    String[] components =
+                    final String[] components =
                             intent.getStringArrayExtra(Intent.EXTRA_CHANGED_COMPONENT_NAME_LIST);
 
                     if (MORE_DEBUG) {
@@ -1206,7 +1206,8 @@ public class RefactoredBackupManagerService implements BackupManagerServiceInter
                         }
                     }
 
-                    mTransportManager.onPackageChanged(pkgName, components);
+                    mBackupHandler.post(
+                            () -> mTransportManager.onPackageChanged(pkgName, components));
                     return; // nothing more to do in the PACKAGE_CHANGED case
                 }
 
@@ -1238,7 +1239,7 @@ public class RefactoredBackupManagerService implements BackupManagerServiceInter
                 }
                 // If they're full-backup candidates, add them there instead
                 final long now = System.currentTimeMillis();
-                for (String packageName : pkgList) {
+                for (final String packageName : pkgList) {
                     try {
                         PackageInfo app = mPackageManager.getPackageInfo(packageName, 0);
                         if (AppBackupUtils.appGetsFullBackup(app)
@@ -1256,7 +1257,8 @@ public class RefactoredBackupManagerService implements BackupManagerServiceInter
                             writeFullBackupScheduleAsync();
                         }
 
-                        mTransportManager.onPackageAdded(packageName);
+                        mBackupHandler.post(
+                                () -> mTransportManager.onPackageAdded(packageName));
 
                     } catch (NameNotFoundException e) {
                         // doesn't really exist; ignore it
@@ -1280,8 +1282,9 @@ public class RefactoredBackupManagerService implements BackupManagerServiceInter
                         removePackageParticipantsLocked(pkgList, uid);
                     }
                 }
-                for (String pkgName : pkgList) {
-                    mTransportManager.onPackageRemoved(pkgName);
+                for (final String pkgName : pkgList) {
+                    mBackupHandler.post(
+                            () -> mTransportManager.onPackageRemoved(pkgName));
                 }
             }
         }
diff --git a/services/backup/java/com/android/server/backup/TransportManager.java b/services/backup/java/com/android/server/backup/TransportManager.java
index 9aae38416bd1..7a0173f669af 100644
--- a/services/backup/java/com/android/server/backup/TransportManager.java
+++ b/services/backup/java/com/android/server/backup/TransportManager.java
@@ -341,9 +341,9 @@ public class TransportManager {
     private class TransportConnection implements ServiceConnection {
 
         // Hold mTransportsLock to access these fields so as to provide a consistent view of them.
-        private IBackupTransport mBinder;
+        private volatile IBackupTransport mBinder;
         private final List<TransportReadyCallback> mListeners = new ArrayList<>();
-        private String mTransportName;
+        private volatile String mTransportName;
 
         private final ComponentName mTransportComponent;
 
@@ -426,25 +426,24 @@ public class TransportManager {
                     + rebindTimeout + "ms");
         }
 
+        // Intentionally not synchronized -- the variable is volatile and changes to its value
+        // are inside synchronized blocks, providing a memory sync barrier; and this method
+        // does not touch any other state protected by that lock.
         private IBackupTransport getBinder() {
-            synchronized (mTransportLock) {
-                return mBinder;
-            }
+            return mBinder;
         }
 
+        // Intentionally not synchronized; same as getBinder()
         private String getName() {
-            synchronized (mTransportLock) {
-                return mTransportName;
-            }
+            return mTransportName;
         }
 
+        // Intentionally not synchronized; same as getBinder()
         private void bindIfUnbound() {
-            synchronized (mTransportLock) {
-                if (mBinder == null) {
-                    Slog.d(TAG,
-                            "Rebinding to transport " + mTransportComponent.flattenToShortString());
-                    bindToTransport(mTransportComponent, this);
-                }
+            if (mBinder == null) {
+                Slog.d(TAG,
+                        "Rebinding to transport " + mTransportComponent.flattenToShortString());
+                bindToTransport(mTransportComponent, this);
             }
         }