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

Commit 9ac40f1c authored by Fyodor Kupolov's avatar Fyodor Kupolov
Browse files

Only use cacheLock when it's needed 

When reading from cache, we can avoid synchronization on dbLock if we 
only read from cache (no db access).

When doing updates to db and cache, we should hold cacheLock only when 
updating the cache.

This change improves locking in the following methods:
 - getAccountVisibilityFromCache
 - saveAuthTokenToDatabase
 - getAccountsFromCacheLocked no longer allows outside locking. The 
   method was renamed to getAccountsFromCache and now self-manages locks
 - writeAuthTokenIntoCacheLocked
 - readAuthTokenInternal

Test: AccountManagerServiceTest
Bug: 36485175
Bug: 35262596
Change-Id: I9aca45c31716c4f0e0fd9f07859e88a7f5ba6922
parent 76d77937

services/core/java/com/android/server/accounts/AccountManagerService.java

+139 −143
Original line number Diff line number Diff line
@@ -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<Account, Integer> signinRequiredNotificationIds =

                new HashMap<Account, Integer>();

        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<String, Account[]> accountCache = new LinkedHashMap<>();

        /** protected by the {@link #cacheLock} */
@@ -586,7 +585,6 @@ public class AccountManagerService 
     */

    private int getAccountVisibilityFromCache(Account account, String packageName,

            UserAccounts accounts) {

        synchronized (accounts.dbLock) {

        synchronized (accounts.cacheLock) {

            Map<String, Integer> accountVisibility =

                    getPackagesAndVisibilityForAccountLocked(account, accounts);
@@ -594,7 +592,6 @@ public class AccountManagerService 
            return visibility != null ? visibility : AccountManager.VISIBILITY_UNDEFINED;

        }

    }

    }



    /**

     * Method which handles default values for Account visibility.
@@ -2240,16 +2237,23 @@ public class AccountManagerService 
        long identityToken = clearCallingIdentity();

        try {

            UserAccounts accounts = getUserAccounts(userId);

            List<Pair<Account, String>> deletedTokens;

            synchronized (accounts.dbLock) {

                synchronized (accounts.cacheLock) {

                accounts.accountsDb.beginTransaction();

                try {

                        invalidateAuthTokenLocked(accounts, accountType, authToken);

                        invalidateCustomTokenLocked(accounts, accountType, authToken);

                    deletedTokens = invalidateAuthTokenLocked(accounts, accountType, authToken);

                    accounts.accountsDb.setTransactionSuccessful();

                } finally {

                    accounts.accountsDb.endTransaction();

                }

                synchronized (accounts.cacheLock) {

                    for (Pair<Account, String> 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,

            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,

    private List<Pair<Account, String>> invalidateAuthTokenLocked(UserAccounts accounts, String accountType,

            String authToken) {

        if (authToken == null || accountType == null) {

            return;

        }

        // TODO Move to AccountsDB

        List<Pair<Account, String>> 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,13 +2295,11 @@ 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);

        }

    }

    }



    private boolean saveAuthTokenToDatabase(UserAccounts accounts, Account account, String type,

            String authToken) {
@@ -2321,8 +2309,8 @@ public class AccountManagerService 
        cancelNotification(getSigninRequiredNotificationId(accounts, account),

                UserHandle.of(accounts.userId));

        synchronized (accounts.dbLock) {

            synchronized (accounts.cacheLock) {

            accounts.accountsDb.beginTransaction();

            boolean updateCache = false;

            try {

                long accountId = accounts.accountsDb.findDeAccountId(account);

                if (accountId < 0) {
@@ -2331,12 +2319,16 @@ public class AccountManagerService 
                accounts.accountsDb.deleteAuthtokensByAccountIdAndType(accountId, type);

                if (accounts.accountsDb.insertAuthToken(accountId, type, authToken) >= 0) {

                    accounts.accountsDb.setTransactionSuccessful();

                        writeAuthTokenIntoCacheLocked(accounts, account, type, authToken);

                    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(

            Account[] accounts = getAccountsFromCache(

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

                    }

                }

            for (Account account : accounts) {

                runningAccounts.add(new AccountAndUser(account, userId));

            }

        }
@@ -4194,11 +4177,9 @@ public class AccountManagerService 
            String callingPackage,

            List<String> visibleAccountTypes,

            boolean includeUserManagedNotVisible) {

        synchronized (userAccounts.dbLock) {

            synchronized (userAccounts.cacheLock) {

        ArrayList<Account> visibleAccounts = new ArrayList<>();

        for (String visibleType : visibleAccountTypes) {

                    Account[] accountsForType = getAccountsFromCacheLocked(

            Account[] accountsForType = getAccountsFromCache(

                    userAccounts, visibleType, callingUid, callingPackage,

                    includeUserManagedNotVisible);

            if (accountsForType != null) {
@@ -4211,8 +4192,6 @@ public class AccountManagerService 
        }

        return result;

    }

        }

    }



    @Override

    public void addSharedAccountsFromParentUser(int parentUserId, int userId,
@@ -4277,11 +4256,13 @@ public class AccountManagerService 
    public Account[] getSharedAccountsAsUser(int userId) {

        userId = handleIncomingUser(userId);

        UserAccounts accounts = getUserAccounts(userId);

        synchronized (accounts.dbLock) {

            List<Account> accountList = accounts.accountsDb.getSharedAccounts();

            Account[] accountArray = new Account[accountList.size()];

            accountList.toArray(accountArray);

            return accountArray;

        }

    }



    @Override

    @NonNull
@@ -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,14 +4898,14 @@ 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.

            synchronized (userAccounts.dbLock) {

                userAccounts.accountsDb.dumpDeAccountsTable(fout);

            }

        } else {

                    Account[] accounts = getAccountsFromCacheLocked(userAccounts, null /* type */,

            Account[] accounts = getAccountsFromCache(userAccounts, null /* type */,

                    Process.SYSTEM_UID, null /* packageName */, false);

            fout.println("Accounts: " + accounts.length);

            for (Account account : accounts) {
@@ -4938,7 +4914,9 @@ public class AccountManagerService 


            // Add debug information.

            fout.println();

            synchronized (userAccounts.dbLock) {

                userAccounts.accountsDb.dumpDebugTable(fout);

            }

            fout.println();

            synchronized (mSessions) {

                final long now = SystemClock.elapsedRealtime();
@@ -4952,8 +4930,6 @@ public class AccountManagerService 
            mAuthenticatorCache.dump(fd, fout, args, userAccounts.userId);

        }

    }

        }

    }



    private void doNotification(UserAccounts accounts, Account account, CharSequence message,

            Intent intent, String packageName, final int 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.

     *

     * <p>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;

            Account[] accountsArray;

            synchronized (userAccounts.cacheLock) {

                for (Account[] accounts : userAccounts.accountCache.values()) {

                    totalLength += accounts.length;

                }

                if (totalLength == 0) {

                    return EMPTY_ACCOUNT_ARRAY;

                }

            Account[] accounts = new Account[totalLength];

                accountsArray = new Account[totalLength];

                totalLength = 0;

                for (Account[] accountsOfType : userAccounts.accountCache.values()) {

                System.arraycopy(accountsOfType, 0, accounts, totalLength,

                    System.arraycopy(accountsOfType, 0, accountsArray, 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<String, String> 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<String, String> 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<String, String> authTokensForAccount = accounts.authTokenCache.get(account);