Calls FireRefreshTokenRevoked if an account is removed.

chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java

Index: chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java b/chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java
index 2e5daa80fbd2633bdb4f86a550669fd4ac23207a..1673a1eabf3fb67a468325123e6c3e7a0896d12f 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java
@@ -7,8 +7,11 @@ package org.chromium.chrome.browser.signin;
 import android.accounts.Account;
 import android.app.Activity;
 import android.content.Context;
+import android.preference.PreferenceManager;
 import android.util.Log;
 
+import com.google.common.annotations.VisibleForTesting;
+
 import org.chromium.base.CalledByNative;
 import org.chromium.base.ObserverList;
 import org.chromium.base.ThreadUtils;
@@ -16,9 +19,12 @@ import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.sync.signin.AccountManagerHelper;
 import org.chromium.sync.signin.ChromeSigninController;
 
+import java.util.Arrays;
 import java.util.concurrent.Semaphore;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.HashSet;
+import java.util.Set;
 
 import javax.annotation.Nullable;
 
@@ -33,6 +39,9 @@ public final class OAuth2TokenService {
 
     private static final String TAG = "OAuth2TokenService";
 
+    @VisibleForTesting
+    public static final String STORED_ACCOUNTS_KEY = "google.services.stored_accounts";
+
     public interface OAuth2TokenServiceObserver {
         void onRefreshTokenAvailable(Account account);
         void onRefreshTokenRevoked(Account account);
@@ -85,14 +94,18 @@ public final class OAuth2TokenService {
         return account;
     }
 
+    public static String[] getSystemAccounts(Context context) {

[nyquist, 2014/04/10 19:07:40]

Does this need to be public?

[acleung1, 2014/04/10 21:36:01]

No. Removed public visibility.

+        AccountManagerHelper accountManagerHelper = AccountManagerHelper.get(context);
+        java.util.List<String> accountNames = accountManagerHelper.getGoogleAccountNames();
+        return accountNames.toArray(new String[accountNames.size()]);
+    }
+
     /**
      * Called by native to list the accounts with OAuth2 refresh tokens.
      */
     @CalledByNative
     public static String[] getAccounts(Context context) {
-        AccountManagerHelper accountManagerHelper = AccountManagerHelper.get(context);
-        java.util.List<String> accountNames = accountManagerHelper.getGoogleAccountNames();
-        return accountNames.toArray(new String[accountNames.size()]);
+        return getSystemAccounts(context);

[Roger Tawa OOO till Jul 10th, 2014/04/10 15:30:54]

This needs to remain getStoredAccounts() right?  Otherwise, after revoking all
tokens, callers of O2TS::GetAccounts() will still see all accounts on the
system.

I think you need to annotate getSystemAccounts() with @CalledByNative, and then
have getSystemAccounts() called at line 179 in
android_profile_oauth2_token_service.cc.  Yes/no?

Alternatively, pass in system accounts to nativeValidateAccounts() so it does
not need to fetch it explicitly?  Might make it easier to test that function
too?

[acleung1, 2014/04/10 21:36:01]

I have been trying to find a way to get this to work (I think you might have
missed a PM I sent you?)

Anyways, here are the issues.

1) GetAccounts is called in more than just one place as this is an PO2TS method.
This mean it is possible that someone might want to list accounts before
ValidateAccounts gets a chance to update the pref.

2) GetAccounts need to memorize the result of the last system to list account.
This means that we are make GetAccount non-sideeffect-free. Which is gets into
all kinds of weird race condition with other people calling it from #1.


For @CalledByNative: I don't think so. Doesn't look like I am making any direct
calls. I haven't gotten any dynamic linking error when I test it.

[Roger Tawa OOO till Jul 10th, 2014/04/14 16:03:58]

GetAccounts() is a public api of OAuth2TokenService, so its behaviour needs to
be well defined.  Its overridden in AndroidProfileOAuth2TokenService to return
the accounts known to chrome.  The behaviour of GetAccounts() on android should
be:

- after initial creation of APO2TS, it should return an empty list
- after a call to nativeValidateAccounts() where currentlySignedInAccount is
empty, it should continue to return an empty list
- after a call to nativeValidateAccounts() where currentlySignedInAccount is
non-empty, it should return the system accounts
- after a call to RevokeAllCredentials(), it should return an empty list

If there is a customer of APO2TS that wants to get the list of "system" accounts
before validateAccounts() has run, there seems to be a layering violation going
on.  What is the scenario?

All calls to GetAccounts() internally (i.e. from this class or the sibling C++
class) should be changed if they need different behaviour.  So my suggestion
would be:

- stored accounts starts out empty
- GetAccounts() returns the stored accounts.  This is side-effect free
- stored accounts is updated in validateAccounts(), as is done already below
- no code called directly or indirectly from validateAccounts() should call
GetAccounts(), since the list is rather undefined at this point.  Code should
call getStoredAccounts() and getSystemAccounts() to get a full picture of the
current state and validate it.  In fact, it might be best to have
validateAccounts() pass all required info as args to nativeValidateAccounts() so
that there is no need to call from C++ code to java code.

Does that seem reasonable?  What situations are not covered by this?

     }
 
     /**
@@ -202,9 +215,15 @@ public final class OAuth2TokenService {
         ThreadUtils.assertOnUiThread();
         String currentlySignedInAccount =
                 ChromeSigninController.get(context).getSignedInAccountName();
-        String[] accounts = getAccounts(context);
-        nativeValidateAccounts(
-                mNativeProfileOAuth2TokenService, accounts, currentlySignedInAccount);
+        String[] prevAccounts = getStoredAccounts(context);
+        saveStoredAccounts(context, getSystemAccounts(context));
+
+        if (!nativeValidateAccounts(
+                mNativeProfileOAuth2TokenService, prevAccounts, currentlySignedInAccount)) {
+            // We will assume that there is no way to get tokens unless we have a sync account.
+            // again.
+            saveStoredAccounts(context, new String[]{});
+        }
     }
 
     /**
@@ -262,10 +281,23 @@ public final class OAuth2TokenService {
         }
     }
 
+    private static String[] getStoredAccounts(Context context) {
+        Set<String> accounts =
+                PreferenceManager.getDefaultSharedPreferences(context)
+                        .getStringSet(STORED_ACCOUNTS_KEY, null);
+        return accounts == null ? new String[]{} : accounts.toArray(new String[accounts.size()]);
+    }
+
+    private static void saveStoredAccounts(Context context, String[] accounts) {
+        Set<String> set = new HashSet<String>(Arrays.asList(accounts));
+        PreferenceManager.getDefaultSharedPreferences(context).edit().
+                putStringSet(STORED_ACCOUNTS_KEY, set).apply();
+    }
+
     private static native Object nativeGetForProfile(Profile profile);
     private static native void nativeOAuth2TokenFetched(
             String authToken, boolean result, long nativeCallback);
-    private native void nativeValidateAccounts(
+    private native boolean nativeValidateAccounts(
             long nativeAndroidProfileOAuth2TokenService,
             String[] accounts, String currentlySignedInAccount);
     private native void nativeFireRefreshTokenAvailableFromJava(