[Android] SigninManager: remove hypothetical SignOut loop

onNativeSignOut() triggers a call to signOut() -> nativeSignOut()

Parts of signOut() are needed, but the call to nativeSignOut() should
be avoided for flows that originated at the native side (native signout
is already done).

Change-Id: Ib205cfd61e94b49838e738b87c5cdb43b2d9f563
Bug: 873116
Reviewed-on: https://chromium-review.googlesource.com/1169462
Commit-Queue: Chrome Cunningham <chcunningham@chromium.org>
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588164}

chrome/android/java_sources.gni

@@ -2260,6 +2260,7 @@ chrome_junit_test_java_sources = [
   "junit/src/org/chromium/chrome/browser/preferences/password/ReauthenticationManagerTest.java",
   "junit/src/org/chromium/chrome/browser/preferences/password/SingleThreadBarrierClosureTest.java",
   "junit/src/org/chromium/chrome/browser/preferences/password/TimedCallbackDelayerTest.java",
+  "junit/src/org/chromium/chrome/browser/signin/SigninManagerTest.java",
   "junit/src/org/chromium/chrome/browser/signin/SigninPromoUtilTest.java",
   "junit/src/org/chromium/chrome/browser/snackbar/SnackbarCollectionUnitTest.java",
   "junit/src/org/chromium/chrome/browser/suggestions/ImageFetcherTest.java",

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

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.signin;
+
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Spy;
+import org.robolectric.annotation.Config;
+
+import org.chromium.base.test.BaseRobolectricTestRunner;
+import org.chromium.components.sync.AndroidSyncSettings;
+import org.chromium.components.sync.test.util.MockSyncContentResolverDelegate;
+
+/** Tests for {@link SigninManager}. */
+@RunWith(BaseRobolectricTestRunner.class)
+@Config(manifest = Config.NONE)
+public class SigninManagerTest {
+    /**
+     * Overrides the native methods called during SigninManager construction.
+     * Other native* methods can be overridden using Spy.
+     */
+    static class TestableSigninManager extends SigninManager {
+        @Override
+        long nativeInit() {
+            return 0;
+        }
+
+        @Override
+        boolean nativeIsSigninAllowedByPolicy(long nativeSigninManagerAndroid) {
+            return true;
+        }
+    }
+
+    @Spy
+    private TestableSigninManager mSigninManager;
+
+    @Before
+    public void setUp() {
+        mSigninManager = spy(new TestableSigninManager());
+
+        // SinginManager interacts with AndroidSyncSettings, but its not the focus
+        // of this test. Using MockSyncContentResolver reduces burden of test setup.
+        AndroidSyncSettings.overrideForTests(new MockSyncContentResolverDelegate(), null);
+    }
+
+    @Test
+    public void signOutFromJavaWithManagedDomain() {
+        // Stub out various native calls. Some of these are verified as never called
+        // and those stubs simply allow that verification to catch any issues.
+        doNothing().when(mSigninManager).nativeSignOut(anyLong());
+        doNothing().when(mSigninManager).nativeWipeProfileData(anyLong());
+        doNothing().when(mSigninManager).nativeWipeGoogleServiceWorkerCaches(anyLong());
+        // See verification of nativeWipeProfileData below.
+        doReturn("TestDomain").when(mSigninManager).nativeGetManagementDomain(anyLong());
+
+        // Trigger the sign out flow!
+        mSigninManager.signOut();
+
+        // nativeSignOut should be called *before* clearing any account data.
+        // http://crbug.com/589028
+        verify(mSigninManager, times(1)).nativeSignOut(anyLong());
+        verify(mSigninManager, never()).nativeWipeGoogleServiceWorkerCaches(anyLong());
+        verify(mSigninManager, never()).nativeWipeProfileData(anyLong());
+
+        // Simulate native callback to trigger clearing of account data.
+        mSigninManager.onNativeSignOut();
+
+        // Sign-out should wipe all profile data when user has managed domain.
+        verify(mSigninManager, times(1)).nativeWipeProfileData(anyLong());
+        verify(mSigninManager, never()).nativeWipeGoogleServiceWorkerCaches(anyLong());
+    }
+
+    @Test
+    public void signOutFromJavaWithNullDomain() {
+        // Stub out various native calls. Some of these are verified as never called
+        // and those stubs simply allow that verification to catch any issues.
+        doNothing().when(mSigninManager).nativeSignOut(anyLong());
+        doNothing().when(mSigninManager).nativeWipeProfileData(anyLong());
+        doNothing().when(mSigninManager).nativeWipeGoogleServiceWorkerCaches(anyLong());
+        // See verification of nativeWipeGoogleServiceWorkerCaches below.
+        doReturn(null).when(mSigninManager).nativeGetManagementDomain(anyLong());
+
+        // Trigger the sign out flow!
+        mSigninManager.signOut();
+
+        // nativeSignOut should be called *before* clearing any account data.
+        // http://crbug.com/589028
+        verify(mSigninManager, times(1)).nativeSignOut(anyLong());
+        verify(mSigninManager, never()).nativeWipeGoogleServiceWorkerCaches(anyLong());
+        verify(mSigninManager, never()).nativeWipeProfileData(anyLong());
+
+        // Simulate native callback to trigger clearing of account data.
+        mSigninManager.onNativeSignOut();
+
+        // Sign-out should only clear the service worker cache when the domain is null.
+        verify(mSigninManager, never()).nativeWipeProfileData(anyLong());
+        verify(mSigninManager, times(1)).nativeWipeGoogleServiceWorkerCaches(anyLong());
+    }
+
+    @Test
+    public void signOutFromNativeWithManagedDomain() {
+        // Stub out various native calls. Some of these are verified as never called
+        // and those stubs simply allow that verification to catch any issues.
+        doNothing().when(mSigninManager).nativeSignOut(anyLong());
+        doNothing().when(mSigninManager).nativeWipeProfileData(anyLong());
+        doNothing().when(mSigninManager).nativeWipeGoogleServiceWorkerCaches(anyLong());
+        // See verification of nativeWipeProfileData below.
+        doReturn("TestDomain").when(mSigninManager).nativeGetManagementDomain(anyLong());
+
+        // Trigger the sign out flow!
+        mSigninManager.onNativeSignOut();
+
+        // nativeSignOut should only be called when signOut() is triggered on
+        // the Java side of the JNI boundary. This test instead initiates sign-out
+        // from the native side.
+        verify(mSigninManager, never()).nativeSignOut(anyLong());
+
+        // Sign-out should wipe profile data when user has managed domain.
+        verify(mSigninManager, times(1)).nativeWipeProfileData(anyLong());
+        verify(mSigninManager, never()).nativeWipeGoogleServiceWorkerCaches(anyLong());
+    }
+
+    @Test
+    public void signOutFromNativeWithNullDomain() {
+        // Stub out various native calls. Some of these are verified as never called
+        // and those stubs simply allow that verification to catch any issues.
+        doNothing().when(mSigninManager).nativeSignOut(anyLong());
+        doNothing().when(mSigninManager).nativeWipeProfileData(anyLong());
+        doNothing().when(mSigninManager).nativeWipeGoogleServiceWorkerCaches(anyLong());
+        // See verification of nativeWipeGoogleServiceWorkerCaches below.
+        doReturn(null).when(mSigninManager).nativeGetManagementDomain(anyLong());
+
+        // Trigger the sign out flow!
+        mSigninManager.onNativeSignOut();
+
+        // nativeSignOut should only be called when signOut() is triggered on
+        // the Java side of the JNI boundary. This test instead initiates sign-out
+        // from the native side.
+        verify(mSigninManager, never()).nativeSignOut(anyLong());
+
+        // Sign-out should only clear the service worker cache when the domain is null.
+        verify(mSigninManager, never()).nativeWipeProfileData(anyLong());
+        verify(mSigninManager, times(1)).nativeWipeGoogleServiceWorkerCaches(anyLong());
+    }
+}

chrome/browser/android/signin/signin_manager_android.cc

@@ -68,10 +68,10 @@ class ProfileDataRemover : public content::BrowsingDataRemover::Observer {
  public:
   ProfileDataRemover(Profile* profile,
                      bool all_data,
-                     const base::Closure& callback)
+                     base::OnceClosure callback)
       : profile_(profile),
         all_data_(all_data),
-        callback_(callback),
+        callback_(std::move(callback)),
         origin_runner_(base::ThreadTaskRunnerHandle::Get()),
         remover_(content::BrowserContext::GetBrowsingDataRemover(profile)) {
     remover_->AddObserver(this);
@@ -113,14 +113,14 @@ class ProfileDataRemover : public content::BrowsingDataRemover::Observer {
       ClearLastSignedInUserForProfile(profile_);
     }
 
-    origin_runner_->PostTask(FROM_HERE, callback_);
+    origin_runner_->PostTask(FROM_HERE, std::move(callback_));
     origin_runner_->DeleteSoon(FROM_HERE, this);
   }
 
  private:
   Profile* profile_;
   bool all_data_;
-  base::Closure callback_;
+  base::OnceClosure callback_;
   scoped_refptr<base::SingleThreadTaskRunner> origin_runner_;
   content::BrowsingDataRemover* remover_;
 
@@ -238,28 +238,19 @@ SigninManagerAndroid::GetManagementDomain(JNIEnv* env,
   return domain;
 }
 
-void SigninManagerAndroid::WipeProfileData(
-    JNIEnv* env,
-    const JavaParamRef<jobject>& obj,
-    const JavaParamRef<jobject>& callback) {
-  base::android::ScopedJavaGlobalRef<jobject> java_callback;
-  java_callback.Reset(env, callback);
-
+void SigninManagerAndroid::WipeProfileData(JNIEnv* env,
+                                           const JavaParamRef<jobject>& obj) {
   WipeData(profile_, true /* all data */,
            base::Bind(&SigninManagerAndroid::OnBrowsingDataRemoverDone,
-                      weak_factory_.GetWeakPtr(), java_callback));
+                      weak_factory_.GetWeakPtr()));
 }
 
 void SigninManagerAndroid::WipeGoogleServiceWorkerCaches(
     JNIEnv* env,
-    const JavaParamRef<jobject>& obj,
-    const JavaParamRef<jobject>& callback) {
-  base::android::ScopedJavaGlobalRef<jobject> java_callback;
-  java_callback.Reset(env, callback);
-
+    const JavaParamRef<jobject>& obj) {
   WipeData(profile_, false /* only Google service worker caches */,
            base::Bind(&SigninManagerAndroid::OnBrowsingDataRemoverDone,
-                      weak_factory_.GetWeakPtr(), java_callback));
+                      weak_factory_.GetWeakPtr()));
 }
 
 void SigninManagerAndroid::OnPolicyRegisterDone(
@@ -288,10 +279,9 @@ void SigninManagerAndroid::OnPolicyFetchDone(bool success) {
       base::android::AttachCurrentThread(), java_signin_manager_);
 }
 
-void SigninManagerAndroid::OnBrowsingDataRemoverDone(
-    const base::android::ScopedJavaGlobalRef<jobject>& callback) {
+void SigninManagerAndroid::OnBrowsingDataRemoverDone() {
   Java_SigninManager_onProfileDataWiped(base::android::AttachCurrentThread(),
-                                        java_signin_manager_, callback);
+                                        java_signin_manager_);
 }
 
 void SigninManagerAndroid::ClearLastSignedInUser(
@@ -363,9 +353,9 @@ void SigninManagerAndroid::OnSigninAllowedPrefChanged() {
 // static
 void SigninManagerAndroid::WipeData(Profile* profile,
                                     bool all_data,
-                                    const base::Closure& callback) {
+                                    base::OnceClosure callback) {
   // The ProfileDataRemover deletes itself once done.
-  new ProfileDataRemover(profile, all_data, callback);
+  new ProfileDataRemover(profile, all_data, std::move(callback));
 }
 
 static jlong JNI_SigninManager_Init(JNIEnv* env,

chrome/browser/android/signin/signin_manager_android.h

@@ -54,14 +54,12 @@ class SigninManagerAndroid : public SigninManagerBase::Observer {
 
   // Delete all data for this profile.
   void WipeProfileData(JNIEnv* env,
-                       const base::android::JavaParamRef<jobject>& obj,
-                       const base::android::JavaParamRef<jobject>& hooks);
+                       const base::android::JavaParamRef<jobject>& obj);
 
   // Delete service worker caches for google.<eTLD>.
   void WipeGoogleServiceWorkerCaches(
       JNIEnv* env,
-      const base::android::JavaParamRef<jobject>& obj,
-      const base::android::JavaParamRef<jobject>& hooks);
+      const base::android::JavaParamRef<jobject>& obj);
 
   void LogInSignedInUser(JNIEnv* env,
                          const base::android::JavaParamRef<jobject>& obj);
@@ -102,14 +100,13 @@ class SigninManagerAndroid : public SigninManagerBase::Observer {
                             const std::string& client_id);
   void OnPolicyFetchDone(bool success);
 
-  void OnBrowsingDataRemoverDone(
-      const base::android::ScopedJavaGlobalRef<jobject>& callback);
+  void OnBrowsingDataRemoverDone();
 
   void OnSigninAllowedPrefChanged();
 
   static void WipeData(Profile* profile,
                        bool all_data,
-                       const base::Closure& callback);
+                       base::OnceClosure callback);
 
   Profile* profile_;