diff --git a/services/core/java/com/android/server/accounts/AccountManagerService.java b/services/core/java/com/android/server/accounts/AccountManagerService.java index 79be3e6cfa131..0ccaf8e914b8c 100644 --- a/services/core/java/com/android/server/accounts/AccountManagerService.java +++ b/services/core/java/com/android/server/accounts/AccountManagerService.java @@ -114,7 +114,6 @@ import java.util.Date; import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashMap; -import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Map.Entry; @@ -201,7 +200,7 @@ public class AccountManagerService private final HashMap signinRequiredNotificationIds = new HashMap(); final Object cacheLock = new Object(); - final Object dbLock = new Object(); + final Object dbLock = new Object(); // if needed, dbLock must be obtained before cacheLock /** protected by the {@link #cacheLock} */ final HashMap accountCache = new LinkedHashMap<>(); /** protected by the {@link #cacheLock} */ @@ -586,13 +585,11 @@ public class AccountManagerService */ private int getAccountVisibilityFromCache(Account account, String packageName, UserAccounts accounts) { - synchronized (accounts.dbLock) { - synchronized (accounts.cacheLock) { - Map accountVisibility = - getPackagesAndVisibilityForAccountLocked(account, accounts); - Integer visibility = accountVisibility.get(packageName); - return visibility != null ? visibility : AccountManager.VISIBILITY_UNDEFINED; - } + synchronized (accounts.cacheLock) { + Map accountVisibility = + getPackagesAndVisibilityForAccountLocked(account, accounts); + Integer visibility = accountVisibility.get(packageName); + return visibility != null ? visibility : AccountManager.VISIBILITY_UNDEFINED; } } @@ -2240,16 +2237,23 @@ public class AccountManagerService long identityToken = clearCallingIdentity(); try { UserAccounts accounts = getUserAccounts(userId); + List> deletedTokens; synchronized (accounts.dbLock) { + accounts.accountsDb.beginTransaction(); + try { + deletedTokens = invalidateAuthTokenLocked(accounts, accountType, authToken); + accounts.accountsDb.setTransactionSuccessful(); + } finally { + accounts.accountsDb.endTransaction(); + } synchronized (accounts.cacheLock) { - accounts.accountsDb.beginTransaction(); - try { - invalidateAuthTokenLocked(accounts, accountType, authToken); - invalidateCustomTokenLocked(accounts, accountType, authToken); - accounts.accountsDb.setTransactionSuccessful(); - } finally { - accounts.accountsDb.endTransaction(); + for (Pair tokenInfo : deletedTokens) { + Account act = tokenInfo.first; + String tokenType = tokenInfo.second; + writeAuthTokenIntoCacheLocked(accounts, act, tokenType, null); } + // wipe out cached token in memory. + accounts.accountTokenCaches.remove(accountType, authToken); } } } finally { @@ -2257,38 +2261,24 @@ public class AccountManagerService } } - private void invalidateCustomTokenLocked( - UserAccounts accounts, - String accountType, + private List> invalidateAuthTokenLocked(UserAccounts accounts, String accountType, String authToken) { - if (authToken == null || accountType == null) { - return; - } - // Also wipe out cached token in memory. - accounts.accountTokenCaches.remove(accountType, authToken); - } - - private void invalidateAuthTokenLocked(UserAccounts accounts, String accountType, - String authToken) { - if (authToken == null || accountType == null) { - return; - } + // TODO Move to AccountsDB + List> results = new ArrayList<>(); Cursor cursor = accounts.accountsDb.findAuthtokenForAllAccounts(accountType, authToken); + try { while (cursor.moveToNext()) { String authTokenId = cursor.getString(0); String accountName = cursor.getString(1); String authTokenType = cursor.getString(2); accounts.accountsDb.deleteAuthToken(authTokenId); - writeAuthTokenIntoCacheLocked( - accounts, - new Account(accountName, accountType), - authTokenType, - null); + results.add(Pair.create(new Account(accountName, accountType), authTokenType)); } } finally { cursor.close(); } + return results; } private void saveCachedToken( @@ -2305,11 +2295,9 @@ public class AccountManagerService } cancelNotification(getSigninRequiredNotificationId(accounts, account), UserHandle.of(accounts.userId)); - synchronized (accounts.dbLock) { - synchronized (accounts.cacheLock) { - accounts.accountTokenCaches.put( - account, token, tokenType, callerPkg, callerSigDigest, expiryMillis); - } + synchronized (accounts.cacheLock) { + accounts.accountTokenCaches.put( + account, token, tokenType, callerPkg, callerSigDigest, expiryMillis); } } @@ -2321,22 +2309,26 @@ public class AccountManagerService cancelNotification(getSigninRequiredNotificationId(accounts, account), UserHandle.of(accounts.userId)); synchronized (accounts.dbLock) { - synchronized (accounts.cacheLock) { - accounts.accountsDb.beginTransaction(); - try { - long accountId = accounts.accountsDb.findDeAccountId(account); - if (accountId < 0) { - return false; - } - accounts.accountsDb.deleteAuthtokensByAccountIdAndType(accountId, type); - if (accounts.accountsDb.insertAuthToken(accountId, type, authToken) >= 0) { - accounts.accountsDb.setTransactionSuccessful(); - writeAuthTokenIntoCacheLocked(accounts, account, type, authToken); - return true; - } + accounts.accountsDb.beginTransaction(); + boolean updateCache = false; + try { + long accountId = accounts.accountsDb.findDeAccountId(account); + if (accountId < 0) { return false; - } finally { - accounts.accountsDb.endTransaction(); + } + accounts.accountsDb.deleteAuthtokensByAccountIdAndType(accountId, type); + if (accounts.accountsDb.insertAuthToken(accountId, type, authToken) >= 0) { + accounts.accountsDb.setTransactionSuccessful(); + updateCache = true; + return true; + } + return false; + } finally { + accounts.accountsDb.endTransaction(); + if (updateCache) { + synchronized (accounts.cacheLock) { + writeAuthTokenIntoCacheLocked(accounts, account, type, authToken); + } } } } @@ -3947,13 +3939,8 @@ public class AccountManagerService @Override public void run() throws RemoteException { - synchronized (mAccounts.dbLock) { - synchronized (mAccounts.cacheLock) { - mAccountsOfType = getAccountsFromCacheLocked(mAccounts, mAccountType, - mCallingUid, - mPackageName, false /* include managed not visible*/); - } - } + mAccountsOfType = getAccountsFromCache(mAccounts, mAccountType, + mCallingUid, mPackageName, false /* include managed not visible*/); // check whether each account matches the requested features mAccountsWithFeatures = new ArrayList<>(mAccountsOfType.length); mCurrentAccount = 0; @@ -4097,18 +4084,14 @@ public class AccountManagerService for (int userId : userIds) { UserAccounts userAccounts = getUserAccounts(userId); if (userAccounts == null) continue; - synchronized (userAccounts.dbLock) { - synchronized (userAccounts.cacheLock) { - Account[] accounts = getAccountsFromCacheLocked( - userAccounts, - null /* type */, - Binder.getCallingUid(), - null /* packageName */, - false /* include managed not visible*/); - for (int a = 0; a < accounts.length; a++) { - runningAccounts.add(new AccountAndUser(accounts[a], userId)); - } - } + Account[] accounts = getAccountsFromCache( + userAccounts, + null /* type */, + Binder.getCallingUid(), + null /* packageName */, + false /* include managed not visible*/); + for (Account account : accounts) { + runningAccounts.add(new AccountAndUser(account, userId)); } } @@ -4194,24 +4177,20 @@ public class AccountManagerService String callingPackage, List visibleAccountTypes, boolean includeUserManagedNotVisible) { - synchronized (userAccounts.dbLock) { - synchronized (userAccounts.cacheLock) { - ArrayList visibleAccounts = new ArrayList<>(); - for (String visibleType : visibleAccountTypes) { - Account[] accountsForType = getAccountsFromCacheLocked( - userAccounts, visibleType, callingUid, callingPackage, - includeUserManagedNotVisible); - if (accountsForType != null) { - visibleAccounts.addAll(Arrays.asList(accountsForType)); - } - } - Account[] result = new Account[visibleAccounts.size()]; - for (int i = 0; i < visibleAccounts.size(); i++) { - result[i] = visibleAccounts.get(i); - } - return result; + ArrayList visibleAccounts = new ArrayList<>(); + for (String visibleType : visibleAccountTypes) { + Account[] accountsForType = getAccountsFromCache( + userAccounts, visibleType, callingUid, callingPackage, + includeUserManagedNotVisible); + if (accountsForType != null) { + visibleAccounts.addAll(Arrays.asList(accountsForType)); } } + Account[] result = new Account[visibleAccounts.size()]; + for (int i = 0; i < visibleAccounts.size(); i++) { + result[i] = visibleAccounts.get(i); + } + return result; } @Override @@ -4277,10 +4256,12 @@ public class AccountManagerService public Account[] getSharedAccountsAsUser(int userId) { userId = handleIncomingUser(userId); UserAccounts accounts = getUserAccounts(userId); - List accountList = accounts.accountsDb.getSharedAccounts(); - Account[] accountArray = new Account[accountList.size()]; - accountList.toArray(accountArray); - return accountArray; + synchronized (accounts.dbLock) { + List accountList = accounts.accountsDb.getSharedAccounts(); + Account[] accountArray = new Account[accountList.size()]; + accountList.toArray(accountArray); + return accountArray; + } } @Override @@ -4360,13 +4341,8 @@ public class AccountManagerService try { UserAccounts userAccounts = getUserAccounts(userId); if (features == null || features.length == 0) { - Account[] accounts; - synchronized (userAccounts.dbLock) { - synchronized (userAccounts.cacheLock) { - accounts = getAccountsFromCacheLocked( - userAccounts, type, callingUid, opPackageName, false); - } - } + Account[] accounts = getAccountsFromCache(userAccounts, type, callingUid, + opPackageName, false); Bundle result = new Bundle(); result.putParcelableArray(AccountManager.KEY_ACCOUNTS, accounts); onResult(response, result); @@ -4922,36 +4898,36 @@ public class AccountManagerService private void dumpUser(UserAccounts userAccounts, FileDescriptor fd, PrintWriter fout, String[] args, boolean isCheckinRequest) { - synchronized (userAccounts.dbLock) { - synchronized (userAccounts.cacheLock) { - if (isCheckinRequest) { - // This is a checkin request. *Only* upload the account types and the count of - // each. - userAccounts.accountsDb.dumpDeAccountsTable(fout); - } else { - Account[] accounts = getAccountsFromCacheLocked(userAccounts, null /* type */, - Process.SYSTEM_UID, null /* packageName */, false); - fout.println("Accounts: " + accounts.length); - for (Account account : accounts) { - fout.println(" " + account); - } + if (isCheckinRequest) { + // This is a checkin request. *Only* upload the account types and the count of + // each. + synchronized (userAccounts.dbLock) { + userAccounts.accountsDb.dumpDeAccountsTable(fout); + } + } else { + Account[] accounts = getAccountsFromCache(userAccounts, null /* type */, + Process.SYSTEM_UID, null /* packageName */, false); + fout.println("Accounts: " + accounts.length); + for (Account account : accounts) { + fout.println(" " + account); + } - // Add debug information. - fout.println(); - userAccounts.accountsDb.dumpDebugTable(fout); - fout.println(); - synchronized (mSessions) { - final long now = SystemClock.elapsedRealtime(); - fout.println("Active Sessions: " + mSessions.size()); - for (Session session : mSessions.values()) { - fout.println(" " + session.toDebugString(now)); - } - } - - fout.println(); - mAuthenticatorCache.dump(fd, fout, args, userAccounts.userId); + // Add debug information. + fout.println(); + synchronized (userAccounts.dbLock) { + userAccounts.accountsDb.dumpDebugTable(fout); + } + fout.println(); + synchronized (mSessions) { + final long now = SystemClock.elapsedRealtime(); + fout.println("Active Sessions: " + mSessions.size()); + for (Session session : mSessions.values()) { + fout.println(" " + session.toDebugString(now)); } } + + fout.println(); + mAuthenticatorCache.dump(fd, fout, args, userAccounts.userId); } } @@ -5609,12 +5585,20 @@ public class AccountManagerService /* * packageName can be null. If not null, it should be used to filter out restricted accounts * that the package is not allowed to access. + * + *

The method shouldn't be called with UserAccounts#cacheLock held, otherwise it will cause a + * deadlock */ @NonNull - protected Account[] getAccountsFromCacheLocked(UserAccounts userAccounts, String accountType, + protected Account[] getAccountsFromCache(UserAccounts userAccounts, String accountType, int callingUid, @Nullable String callingPackage, boolean includeManagedNotVisible) { + Preconditions.checkState(!Thread.holdsLock(userAccounts.cacheLock), + "Method should not be called with cacheLock"); if (accountType != null) { - final Account[] accounts = userAccounts.accountCache.get(accountType); + Account[] accounts; + synchronized (userAccounts.cacheLock) { + accounts = userAccounts.accountCache.get(accountType); + } if (accounts == null) { return EMPTY_ACCOUNT_ARRAY; } else { @@ -5623,20 +5607,23 @@ public class AccountManagerService } } else { int totalLength = 0; - for (Account[] accounts : userAccounts.accountCache.values()) { - totalLength += accounts.length; + Account[] accountsArray; + synchronized (userAccounts.cacheLock) { + for (Account[] accounts : userAccounts.accountCache.values()) { + totalLength += accounts.length; + } + if (totalLength == 0) { + return EMPTY_ACCOUNT_ARRAY; + } + accountsArray = new Account[totalLength]; + totalLength = 0; + for (Account[] accountsOfType : userAccounts.accountCache.values()) { + System.arraycopy(accountsOfType, 0, accountsArray, totalLength, + accountsOfType.length); + totalLength += accountsOfType.length; + } } - if (totalLength == 0) { - return EMPTY_ACCOUNT_ARRAY; - } - Account[] accounts = new Account[totalLength]; - totalLength = 0; - for (Account[] accountsOfType : userAccounts.accountCache.values()) { - System.arraycopy(accountsOfType, 0, accounts, totalLength, - accountsOfType.length); - totalLength += accountsOfType.length; - } - return filterAccounts(userAccounts, accounts, callingUid, callingPackage, + return filterAccounts(userAccounts, accountsArray, callingUid, callingPackage, includeManagedNotVisible); } } @@ -5669,6 +5656,7 @@ public class AccountManagerService } } + /** protected by the {@code dbLock}, {@code cacheLock} */ protected void writeAuthTokenIntoCacheLocked(UserAccounts accounts, Account account, String key, String value) { Map authTokensForAccount = accounts.authTokenCache.get(account); @@ -5685,6 +5673,14 @@ public class AccountManagerService protected String readAuthTokenInternal(UserAccounts accounts, Account account, String authTokenType) { + // Fast path - check if account is already cached + synchronized (accounts.cacheLock) { + Map authTokensForAccount = accounts.authTokenCache.get(account); + if (authTokensForAccount != null) { + return authTokensForAccount.get(authTokenType); + } + } + // If not cached yet - do slow path and sync with db if necessary synchronized (accounts.dbLock) { synchronized (accounts.cacheLock) { Map authTokensForAccount = accounts.authTokenCache.get(account);