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

Commit 43f2d3d3 authored by Christopher Tate's avatar Christopher Tate
Browse files

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
parent 2f3072bc

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

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















                }















            }















        }

































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



+10
−7




























Original line number
Diff line number
Diff line








@@ -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));















                }















            }















        }

































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



+13
−14




























Original line number
Diff line number
Diff line








@@ -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,27 +426,26 @@ 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;















        }













        }





























        // Intentionally not synchronized; same as getBinder()















        private String getName() {













            synchronized (mTransportLock) {















            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);















            }















        }













        }































        private void addListener(TransportReadyCallback listener) {















            synchronized (mTransportLock) {