Merge "Reduce lock interactions in backup transport management" into oc-mr1-dev

am: 96b9ddbcb2

Change-Id: I40b489172034036d112d367c183110ae9d9408db

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 edcd4b7ba99b..0dd518141cf6 100644
--- a/services/backup/java/com/android/server/backup/BackupManagerService.java
+++ b/services/backup/java/com/android/server/backup/BackupManagerService.java
@@ -1983,7 +1983,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 };
                 }
@@ -1991,7 +1991,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) {
@@ -2001,7 +2001,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
                 }
 
@@ -2033,7 +2034,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)
@@ -2050,7 +2051,8 @@ public class BackupManagerService implements BackupManagerServiceInterface {
                             writeFullBackupScheduleAsync();
                         }
 
-                        mTransportManager.onPackageAdded(packageName);
+                        mBackupHandler.post(
+                                () -> mTransportManager.onPackageAdded(packageName));
 
                     } catch (NameNotFoundException e) {
                         // doesn't really exist; ignore it
@@ -2074,8 +2076,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 74859777ae1e..23abd934334b 100644
--- a/services/backup/java/com/android/server/backup/RefactoredBackupManagerService.java
+++ b/services/backup/java/com/android/server/backup/RefactoredBackupManagerService.java
@@ -1197,7 +1197,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};
                 }
@@ -1205,7 +1205,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) {
@@ -1215,7 +1215,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
                 }
 
@@ -1247,7 +1248,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)
@@ -1265,7 +1266,8 @@ public class RefactoredBackupManagerService implements BackupManagerServiceInter
                             writeFullBackupScheduleAsync();
                         }
 
-                        mTransportManager.onPackageAdded(packageName);
+                        mBackupHandler.post(
+                                () -> mTransportManager.onPackageAdded(packageName));
 
                     } catch (NameNotFoundException e) {
                         // doesn't really exist; ignore it
@@ -1289,8 +1291,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 fb2982eb0baa..dab218db51b2 100644
--- a/services/backup/java/com/android/server/backup/TransportManager.java
+++ b/services/backup/java/com/android/server/backup/TransportManager.java
@@ -316,9 +316,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<SelectBackupTransportCallback> mListeners = new ArrayList<>();
-        private String mTransportName;
+        private volatile String mTransportName;
 
         private final ComponentName mTransportComponent;
 
@@ -401,25 +401,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);
             }
         }