From 2f87cb2d4a2417a70486b117d57d7547d4605a58 Mon Sep 17 00:00:00 2001 From: Christopher Tate Date: Wed, 27 Sep 2017 15:39:54 -0700 Subject: [PATCH] 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 Merged-In: I361ad4a0729f319db7339bd341a6d33aa3b64fed --- .../server/backup/BackupManagerService.java | 17 +++++++----- .../RefactoredBackupManagerService.java | 17 +++++++----- .../server/backup/TransportManager.java | 27 +++++++++---------- 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 edcd4b7ba99b0..0dd518141cf63 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 74859777ae1e6..23abd934334bb 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 fb2982eb0baaa..dab218db51b2c 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 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); } }