Pass IdentityMutator.java to SigninManager.java directly

Avoid exposing getIdentityMutator in IdentityManager.java by passing the
IdentityMutator reference directly in SigninManagerAndroid's and
SigninManager.java's constructors.

Bug: 934688
Change-Id: I02667bfe89fe2e3925363f2bf0075ef53b0bd8ee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1800405
Commit-Queue: Pâris Meuleman <pmeuleman@chromium.org>
Auto-Submit: Pâris Meuleman <pmeuleman@chromium.org>
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#696743}

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

@@ -237,18 +237,20 @@ public class SigninManager
      */
     @CalledByNative
     private static SigninManager create(long nativeSigninManagerAndroid,
-            AccountTrackerService accountTrackerService, IdentityManager identityManager) {
+            AccountTrackerService accountTrackerService, IdentityManager identityManager,
+            IdentityMutator identityMutator) {
         assert nativeSigninManagerAndroid != 0;
         assert accountTrackerService != null;
         assert identityManager != null;
+        assert identityMutator != null;
         return new SigninManager(ContextUtils.getApplicationContext(), nativeSigninManagerAndroid,
-                accountTrackerService, identityManager, AndroidSyncSettings.get());
+                accountTrackerService, identityManager, identityMutator, AndroidSyncSettings.get());
     }
 
     @VisibleForTesting
     SigninManager(Context context, long nativeSigninManagerAndroid,
             AccountTrackerService accountTrackerService, IdentityManager identityManager,
-            AndroidSyncSettings androidSyncSettings) {
+            IdentityMutator identityMutator, AndroidSyncSettings androidSyncSettings) {
         ThreadUtils.assertOnUiThread();
         assert context != null;
         assert androidSyncSettings != null;
@@ -256,7 +258,7 @@ public class SigninManager
         mNativeSigninManagerAndroid = nativeSigninManagerAndroid;
         mAccountTrackerService = accountTrackerService;
         mIdentityManager = identityManager;
-        mIdentityMutator = identityManager.getIdentityMutator();
+        mIdentityMutator = identityMutator;
         mAndroidSyncSettings = androidSyncSettings;
 
         mSigninAllowedByPolicy =

chrome/android/junit/src/org/chromium/chrome/browser/signin/SigninManagerTest.java

@@ -71,14 +71,13 @@ public class SigninManagerTest {
 
         mIdentityMutator = mock(IdentityMutator.class);
 
-        mIdentityManager =
-                spy(new IdentityManager(0 /* nativeIdentityManager */, mIdentityMutator));
+        mIdentityManager = spy(new IdentityManager(0 /* nativeIdentityManager */));
 
         AndroidSyncSettings androidSyncSettings = mock(AndroidSyncSettings.class);
 
         mSigninManager = new SigninManager(ContextUtils.getApplicationContext(),
                 0 /* nativeSigninManagerAndroid */, mAccountTrackerService, mIdentityManager,
-                androidSyncSettings);
+                mIdentityMutator, androidSyncSettings);
 
         mAccount = new CoreAccountInfo(new CoreAccountId("gaia-id-user"),
                 AccountManagerFacade.createAccountFromName("user@domain.com"), "gaia-id-user");

chrome/browser/android/signin/signin_manager_android.cc

@@ -146,7 +146,8 @@ SigninManagerAndroid::SigninManagerAndroid(
   java_signin_manager_ = Java_SigninManager_create(
       base::android::AttachCurrentThread(), reinterpret_cast<intptr_t>(this),
       identity_manager_->LegacyGetAccountTrackerServiceJavaObject(),
-      identity_manager_->GetJavaObject());
+      identity_manager_->GetJavaObject(),
+      identity_manager_->GetIdentityMutatorJavaObject());
 }
 
 base::android::ScopedJavaLocalRef<jobject>

components/signin/public/identity_manager/android/java/src/org/chromium/components/signin/identitymanager/IdentityManager.java

@@ -37,26 +37,21 @@ public class IdentityManager {
     }
 
     private long mNativeIdentityManager;
-    private IdentityMutator mIdentityMutator;
 
     private final ObserverList<Observer> mObservers = new ObserverList<>();
 
     /**
      * Called by native to create an instance of IdentityManager.
-     * @param identityMutator can be null if native's IdentityManager received a null
-     * IdentityMutator, this happens in tests.
      */
     @CalledByNative
-    static private IdentityManager create(
-            long nativeIdentityManager, @Nullable IdentityMutator identityMutator) {
+    private static IdentityManager create(long nativeIdentityManager) {
         assert nativeIdentityManager != 0;
-        return new IdentityManager(nativeIdentityManager, identityMutator);
+        return new IdentityManager(nativeIdentityManager);
     }
 
     @VisibleForTesting
-    public IdentityManager(long nativeIdentityManager, IdentityMutator identityMutator) {
+    public IdentityManager(long nativeIdentityManager) {
         mNativeIdentityManager = nativeIdentityManager;
-        mIdentityMutator = identityMutator;
     }
 
     /**
@@ -120,14 +115,6 @@ public class IdentityManager {
                         mNativeIdentityManager, email);
     }
 
-    /*
-     * Returns pointer to the object used to change the signed-in state of the
-     * primary account.
-     */
-    public IdentityMutator getIdentityMutator() {
-        return mIdentityMutator;
-    }
-
     @NativeMethods
     interface Natives {
         public boolean hasPrimaryAccount(long nativeIdentityManager);

components/signin/public/identity_manager/android/java/src/org/chromium/components/signin/identitymanager/IdentityMutator.java

@@ -16,8 +16,10 @@ import org.chromium.components.signin.metrics.SignoutReason;
 public class IdentityMutator {
     private static final String TAG = "IdentityMutator";
 
-    private final long mNativePrimaryAccountMutator;
-    private final long mNativeIdentityManager;
+    // Pointer to native PrimaryAccountMutator, not final because of destroy().
+    private long mNativePrimaryAccountMutator;
+    // Pointer to native IdentityManager, not final because of destroy().
+    private long mNativeIdentityManager;
 
     @CalledByNative
     private IdentityMutator(long nativePrimaryAccountMutator, long nativeIdentityManager) {
@@ -27,6 +29,15 @@ public class IdentityMutator {
         mNativeIdentityManager = nativeIdentityManager;
     }
 
+    /**
+     * Called by native IdentityManager upon KeyedService's shutdown
+     */
+    @CalledByNative
+    private void destroy() {
+        mNativeIdentityManager = 0;
+        mNativePrimaryAccountMutator = 0;
+    }
+
     /**
      * Marks the account with |account_id| as the primary account, and returns whether the operation
      * succeeded or not. To succeed, this requires that:

components/signin/public/identity_manager/identity_manager.cc

@@ -100,7 +100,7 @@ IdentityManager::IdentityManager(
     UpdateUnconsentedPrimaryAccount();
 
 #if defined(OS_ANDROID)
-  base::android::ScopedJavaLocalRef<jobject> java_identity_mutator =
+  java_identity_mutator_ =
       primary_account_mutator_
           ? Java_IdentityMutator_Constructor(
                 base::android::AttachCurrentThread(),
@@ -109,8 +109,7 @@ IdentityManager::IdentityManager(
           : nullptr;
 
   java_identity_manager_ = Java_IdentityManager_create(
-      base::android::AttachCurrentThread(), reinterpret_cast<intptr_t>(this),
-      java_identity_mutator);
+      base::android::AttachCurrentThread(), reinterpret_cast<intptr_t>(this));
 #endif
 }
 
@@ -127,6 +126,9 @@ IdentityManager::~IdentityManager() {
   if (java_identity_manager_)
     Java_IdentityManager_destroy(base::android::AttachCurrentThread(),
                                  java_identity_manager_);
+  if (java_identity_mutator_)
+    Java_IdentityMutator_destroy(base::android::AttachCurrentThread(),
+                                 java_identity_mutator_);
 #endif
 }
 
@@ -427,6 +429,12 @@ base::android::ScopedJavaLocalRef<jobject> IdentityManager::GetJavaObject() {
   return base::android::ScopedJavaLocalRef<jobject>(java_identity_manager_);
 }
 
+base::android::ScopedJavaLocalRef<jobject>
+IdentityManager::GetIdentityMutatorJavaObject() {
+  DCHECK(java_identity_manager_);
+  return base::android::ScopedJavaLocalRef<jobject>(java_identity_mutator_);
+}
+
 void IdentityManager::ForceRefreshOfExtendedAccountInfo(
     const CoreAccountId& account_id) {
   DCHECK(HasAccountWithRefreshToken(account_id));

components/signin/public/identity_manager/identity_manager.h

@@ -451,6 +451,9 @@ class IdentityManager : public KeyedService,
   // Get the reference on the java IdentityManager.
   base::android::ScopedJavaLocalRef<jobject> GetJavaObject();
 
+  // Provide the reference on the java IdentityMutator.
+  base::android::ScopedJavaLocalRef<jobject> GetIdentityMutatorJavaObject();
+
   // This method has the contractual assumption that the account is a known
   // account and has as its semantics that it fetches the account info for the
   // account, triggering an OnExtendedAccountInfoUpdated() callback if the info
@@ -686,6 +689,8 @@ class IdentityManager : public KeyedService,
 #if defined(OS_ANDROID)
   // Java-side IdentityManager object.
   base::android::ScopedJavaGlobalRef<jobject> java_identity_manager_;
+  // Java-side IdentityMutator object.
+  base::android::ScopedJavaGlobalRef<jobject> java_identity_mutator_;
 #endif
 
   DISALLOW_COPY_AND_ASSIGN(IdentityManager);