From 71fa5261d335a877453383eea911e9f51ad15589 Mon Sep 17 00:00:00 2001 From: Dmitry Dementyev Date: Thu, 23 Mar 2017 12:29:17 -0700 Subject: [PATCH] Add cache for Account Visibility values. Test: cts Bug: 36485175 Change-Id: I075b81f8b287199d51ea09dc1108748310ab846c --- .../accounts/AccountManagerService.java | 123 +++++++++++------- .../android/server/accounts/AccountsDb.java | 39 +++++- .../accounts/AccountManagerServiceTest.java | 2 +- .../server/accounts/AccountsDbTest.java | 36 +++++ 4 files changed, 152 insertions(+), 48 deletions(-) diff --git a/services/core/java/com/android/server/accounts/AccountManagerService.java b/services/core/java/com/android/server/accounts/AccountManagerService.java index 8e3e3eaa1a60e..77a02b4d2e9dd 100644 --- a/services/core/java/com/android/server/accounts/AccountManagerService.java +++ b/services/core/java/com/android/server/accounts/AccountManagerService.java @@ -202,16 +202,17 @@ public class AccountManagerService new HashMap(); final Object cacheLock = new Object(); /** protected by the {@link #cacheLock} */ - final HashMap accountCache = - new LinkedHashMap<>(); + final HashMap accountCache = new LinkedHashMap<>(); /** protected by the {@link #cacheLock} */ private final Map> userDataCache = new HashMap<>(); /** protected by the {@link #cacheLock} */ private final Map> authTokenCache = new HashMap<>(); /** protected by the {@link #cacheLock} */ private final TokenCache accountTokenCaches = new TokenCache(); + /** protected by the {@link #cacheLock} */ + private final Map> visibilityCache = new HashMap<>(); - /** protected by the {@link #mReceiversForType} + /** protected by the {@link #mReceiversForType}, * type -> (packageName -> number of active receivers) * type == null is used to get notifications about all account types */ @@ -524,25 +525,29 @@ public class AccountManagerService String.format("uid %s cannot get secrets for account %s", callingUid, account); throw new SecurityException(msg); } - return getPackagesAndVisibilityForAccount(account, accounts); + synchronized (accounts.cacheLock) { + return getPackagesAndVisibilityForAccountLocked(account, accounts); + } } /** - * Returns all package names and visibility values, which were set for given account. + * Returns Map with all package names and visibility values for given account. + * The method and returned map must be guarded by accounts.cacheLock * * @param account Account to get visibility values. * @param accounts UserAccount that currently hosts the account and application * - * @return Map from package names to visibility. + * @return Map with cache for package names to visibility. */ - private Map getPackagesAndVisibilityForAccount(Account account, + private @NonNull Map getPackagesAndVisibilityForAccountLocked(Account account, UserAccounts accounts) { - final StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskReads(); - try { - return accounts.accountsDb.findAllVisibilityValuesForAccount(account); - } finally { - StrictMode.setThreadPolicy(oldPolicy); + Map accountVisibility = accounts.visibilityCache.get(account); + if (accountVisibility == null) { + Log.d(TAG, "Visibility was not initialized"); + accountVisibility = new HashMap<>(); + accounts.visibilityCache.put(account, accountVisibility); } + return accountVisibility; } @Override @@ -572,14 +577,13 @@ public class AccountManagerService * @return Visibility value, AccountManager.VISIBILITY_UNDEFINED if no value was stored. * */ - private int getAccountVisibility(Account account, String packageName, UserAccounts accounts) { - final StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskReads(); - try { - Integer visibility = - accounts.accountsDb.findAccountVisibility(account, packageName); + private int getAccountVisibilityFromCache(Account account, String packageName, + UserAccounts accounts) { + synchronized (accounts.cacheLock) { + Map accountVisibility = + getPackagesAndVisibilityForAccountLocked(account, accounts); + Integer visibility = accountVisibility.get(packageName); return visibility != null ? visibility : AccountManager.VISIBILITY_UNDEFINED; - } finally { - StrictMode.setThreadPolicy(oldPolicy); } } @@ -595,9 +599,7 @@ public class AccountManagerService */ private Integer resolveAccountVisibility(Account account, @NonNull String packageName, UserAccounts accounts) { - Preconditions.checkNotNull(packageName, "packageName cannot be null"); - int uid = -1; try { long identityToken = clearCallingIdentity(); @@ -630,7 +632,7 @@ public class AccountManagerService } // Return stored value if it was set. - int visibility = getAccountVisibility(account, packageName, accounts); + int visibility = getAccountVisibilityFromCache(account, packageName, accounts); if (AccountManager.VISIBILITY_UNDEFINED != visibility) { return visibility; @@ -652,13 +654,13 @@ public class AccountManagerService || canReadContacts || isPrivileged) { // Use legacy for preO apps with GET_ACCOUNTS permission or pre/postO with signature // match. - visibility = getAccountVisibility(account, + visibility = getAccountVisibilityFromCache(account, AccountManager.PACKAGE_NAME_KEY_LEGACY_VISIBLE, accounts); if (AccountManager.VISIBILITY_UNDEFINED == visibility) { visibility = AccountManager.VISIBILITY_USER_MANAGED_VISIBLE; } } else { - visibility = getAccountVisibility(account, + visibility = getAccountVisibilityFromCache(account, AccountManager.PACKAGE_NAME_KEY_LEGACY_NOT_VISIBLE, accounts); if (AccountManager.VISIBILITY_UNDEFINED == visibility) { visibility = AccountManager.VISIBILITY_USER_MANAGED_NOT_VISIBLE; @@ -751,21 +753,10 @@ public class AccountManagerService packagesToVisibility = new HashMap<>(); } - final long accountId = accounts.accountsDb.findDeAccountId(account); - if (accountId < 0) { + if (!updateAccountVisibilityLocked(account, packageName, newVisibility, accounts)) { return false; } - final StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskWrites(); - try { - if (!accounts.accountsDb.setAccountVisibility(accountId, packageName, - newVisibility)) { - return false; - } - } finally { - StrictMode.setThreadPolicy(oldPolicy); - } - if (notify) { for (Entry packageToVisibility : packagesToVisibility.entrySet()) { if (packageToVisibility.getValue() != AccountManager.VISIBILITY_NOT_VISIBLE) { @@ -778,6 +769,29 @@ public class AccountManagerService } } + // Update account visibility in cache and database. + private boolean updateAccountVisibilityLocked(Account account, String packageName, + int newVisibility, UserAccounts accounts) { + final long accountId = accounts.accountsDb.findDeAccountId(account); + if (accountId < 0) { + return false; + } + + final StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskWrites(); + try { + if (!accounts.accountsDb.setAccountVisibility(accountId, packageName, + newVisibility)) { + return false; + } + } finally { + StrictMode.setThreadPolicy(oldPolicy); + } + Map accountVisibility = + getPackagesAndVisibilityForAccountLocked(account, accounts); + accountVisibility.put(packageName, newVisibility); + return true; + } + @Override public void registerAccountListener(String[] accountTypes, String opPackageName) { int callingUid = Binder.getCallingUid(); @@ -1042,9 +1056,12 @@ public class AccountManagerService accounts.userDataCache.remove(account); accounts.authTokenCache.remove(account); accounts.accountTokenCaches.remove(account); + accounts.visibilityCache.remove(account); - for (Entry packageToVisibility : packagesToVisibility.entrySet()) { - if (packageToVisibility.getValue() != AccountManager.VISIBILITY_NOT_VISIBLE) { + for (Entry packageToVisibility : + packagesToVisibility.entrySet()) { + if (packageToVisibility.getValue() + != AccountManager.VISIBILITY_NOT_VISIBLE) { notifyPackage(packageToVisibility.getKey(), accounts); } } @@ -1067,6 +1084,7 @@ public class AccountManagerService } accounts.accountCache.put(accountType, accountsForType); } + accounts.visibilityCache.putAll(accountsDb.findAllVisibilityValues()); } finally { if (accountDeleted) { sendAccountsChangedBroadcast(accounts.userId); @@ -1181,19 +1199,31 @@ public class AccountManagerService } private void removeVisibilityValuesForPackage(String packageName) { + if (isSpecialPackageKey(packageName)) { + return; + } synchronized (mUsers) { - for (int i = 0; i < mUsers.size(); i++) { - UserAccounts accounts = mUsers.valueAt(i); - try { - int uid = mPackageManager.getPackageUidAsUser(packageName, accounts.userId); - } catch (NameNotFoundException e) { - // package does not exist - remove visibility values - accounts.accountsDb.deleteAccountVisibilityForPackage(packageName); + int numberOfUsers = mUsers.size(); + for (int i = 0; i < numberOfUsers; i++) { + UserAccounts accounts = mUsers.valueAt(i); + try { + mPackageManager.getPackageUidAsUser(packageName, accounts.userId); + } catch (NameNotFoundException e) { + // package does not exist - remove visibility values + accounts.accountsDb.deleteAccountVisibilityForPackage(packageName); + synchronized(accounts.cacheLock) { + for (Account account : accounts.visibilityCache.keySet()) { + Map accountVisibility = + getPackagesAndVisibilityForAccountLocked(account, accounts); + accountVisibility.remove(packageName); + } + } } } } } + private void onCleanupUser(int userId) { Log.i(TAG, "onCleanupUser " + userId); UserAccounts accounts; @@ -1849,6 +1879,7 @@ public class AccountManagerService */ Map tmpData = accounts.userDataCache.get(accountToRename); Map tmpTokens = accounts.authTokenCache.get(accountToRename); + Map tmpVisibility = accounts.visibilityCache.get(accountToRename); removeAccountFromCacheLocked(accounts, accountToRename); /* * Update the cached data associated with the renamed @@ -1856,6 +1887,7 @@ public class AccountManagerService */ accounts.userDataCache.put(renamedAccount, tmpData); accounts.authTokenCache.put(renamedAccount, tmpTokens); + accounts.visibilityCache.put(renamedAccount, tmpVisibility); accounts.previousNameCache.put( renamedAccount, new AtomicReference<>(accountToRename.name)); @@ -5369,6 +5401,7 @@ public class AccountManagerService accounts.userDataCache.remove(account); accounts.authTokenCache.remove(account); accounts.previousNameCache.remove(account); + accounts.visibilityCache.remove(account); } /** diff --git a/services/core/java/com/android/server/accounts/AccountsDb.java b/services/core/java/com/android/server/accounts/AccountsDb.java index 22543cb9e0223..8ca7ea1768b91 100644 --- a/services/core/java/com/android/server/accounts/AccountsDb.java +++ b/services/core/java/com/android/server/accounts/AccountsDb.java @@ -909,7 +909,7 @@ class AccountsDb implements AutoCloseable { } Integer findAccountVisibility(Account account, String packageName) { - SQLiteDatabase db = mDeDatabase.getWritableDatabase(); + SQLiteDatabase db = mDeDatabase.getReadableDatabase(); final Cursor cursor = db.query(TABLE_VISIBILITY, new String[] {VISIBILITY_VALUE}, SELECTION_ACCOUNTS_ID_BY_ACCOUNT + " AND " + VISIBILITY_PACKAGE + "=? ", new String[] {account.name, account.type, packageName}, null, null, null); @@ -924,7 +924,7 @@ class AccountsDb implements AutoCloseable { } Integer findAccountVisibility(long accountId, String packageName) { - SQLiteDatabase db = mDeDatabase.getWritableDatabase(); + SQLiteDatabase db = mDeDatabase.getReadableDatabase(); final Cursor cursor = db.query(TABLE_VISIBILITY, new String[] {VISIBILITY_VALUE}, VISIBILITY_ACCOUNTS_ID + "=? AND " + VISIBILITY_PACKAGE + "=? ", new String[] {String.valueOf(accountId), packageName}, null, null, null); @@ -972,6 +972,41 @@ class AccountsDb implements AutoCloseable { return result; } + /** + * Returns a map account -> (package -> visibility) + */ + Map > findAllVisibilityValues() { + SQLiteDatabase db = mDeDatabase.getReadableDatabase(); + Map> result = new HashMap<>(); + Cursor cursor = db.rawQuery( + "SELECT " + TABLE_VISIBILITY + "." + VISIBILITY_PACKAGE + + ", " + TABLE_VISIBILITY + "." + VISIBILITY_VALUE + + ", " + TABLE_ACCOUNTS + "." + ACCOUNTS_NAME + + ", " + TABLE_ACCOUNTS + "." + ACCOUNTS_TYPE + + " FROM " + TABLE_VISIBILITY + + " JOIN " + TABLE_ACCOUNTS + + " ON " + TABLE_ACCOUNTS + "." + ACCOUNTS_ID + + " = " + TABLE_VISIBILITY + "." + VISIBILITY_ACCOUNTS_ID, null); + try { + while (cursor.moveToNext()) { + String packageName = cursor.getString(0); + Integer visibility = cursor.getInt(1); + String accountName = cursor.getString(2); + String accountType = cursor.getString(3); + Account account = new Account(accountName, accountType); + Map accountVisibility = result.get(account); + if (accountVisibility == null) { + accountVisibility = new HashMap<>(); + result.put(account, accountVisibility); + } + accountVisibility.put(packageName, visibility); + } + } finally { + cursor.close(); + } + return result; + } + boolean deleteAccountVisibilityForPackage(String packageName) { SQLiteDatabase db = mDeDatabase.getWritableDatabase(); return db.delete(TABLE_VISIBILITY, VISIBILITY_PACKAGE + "=? ", diff --git a/services/tests/servicestests/src/com/android/server/accounts/AccountManagerServiceTest.java b/services/tests/servicestests/src/com/android/server/accounts/AccountManagerServiceTest.java index 374aee10da452..00f6273d14bf8 100644 --- a/services/tests/servicestests/src/com/android/server/accounts/AccountManagerServiceTest.java +++ b/services/tests/servicestests/src/com/android/server/accounts/AccountManagerServiceTest.java @@ -94,7 +94,7 @@ import java.util.concurrent.atomic.AtomicLong; *

Run with:

  * mmma -j40 frameworks/base/services/tests/servicestests
  * adb install -r ${OUT}/data/app/FrameworksServicesTests/FrameworksServicesTests.apk
- * adb shell am instrument -w -e class package com.android.server.accounts \
+ * adb shell am instrument -w -e package com.android.server.accounts \
  * com.android.frameworks.servicestests\
  * /android.support.test.runner.AndroidJUnitRunner
  *
diff --git a/services/tests/servicestests/src/com/android/server/accounts/AccountsDbTest.java b/services/tests/servicestests/src/com/android/server/accounts/AccountsDbTest.java index 2cb8af4e97e7d..aa374073e4a58 100644 --- a/services/tests/servicestests/src/com/android/server/accounts/AccountsDbTest.java +++ b/services/tests/servicestests/src/com/android/server/accounts/AccountsDbTest.java @@ -384,6 +384,42 @@ public class AccountsDbTest { assertFalse(mAccountsDb.deleteAccountVisibilityForPackage(packageName1)); // 2nd attempt. } + @Test + public void testFindAllVisibilityValues() { + long accId = 10; + long accId2 = 11; + String packageName1 = "com.example.one"; + String packageName2 = "com.example.two"; + Account account = new Account("name", "example.com"); + Account account2 = new Account("name2", "example2.com"); + assertNull(mAccountsDb.findAccountVisibility(account, packageName1)); + + mAccountsDb.insertDeAccount(account, accId); + assertNull(mAccountsDb.findAccountVisibility(account, packageName1)); + assertNull(mAccountsDb.findAccountVisibility(accId, packageName1)); + mAccountsDb.insertDeAccount(account2, accId2); + + mAccountsDb.setAccountVisibility(accId, packageName1, 1); + mAccountsDb.setAccountVisibility(accId, packageName2, 2); + mAccountsDb.setAccountVisibility(accId2, packageName1, 1); + + Map> vis = mAccountsDb.findAllVisibilityValues(); + assertEquals(vis.size(), 2); + Map accnt1Visibility = vis.get(account); + assertEquals(accnt1Visibility.size(), 2); + assertEquals(accnt1Visibility.get(packageName1), Integer.valueOf(1)); + assertEquals(accnt1Visibility.get(packageName2), Integer.valueOf(2)); + Map accnt2Visibility = vis.get(account2); + assertEquals(accnt2Visibility.size(), 1); + assertEquals(accnt2Visibility.get(packageName1), Integer.valueOf(1)); + + mAccountsDb.setAccountVisibility(accId2, packageName2, 3); + vis = mAccountsDb.findAllVisibilityValues(); + accnt2Visibility = vis.get(account2); + assertEquals(accnt2Visibility.size(), 2); + assertEquals(accnt2Visibility.get(packageName2), Integer.valueOf(3)); + } + @Test public void testVisibilityCleanupTrigger() { long accId = 10;