[Android][Signin] Fix signout recorded twice

This CL adds espresso tests for signin and signout workflow and fixes
a bug that signout was recorded twice when confirmed or dismissed.

Bug: 1038326,1038924
Change-Id: I1c7d9d75f0e32c2d9fde6b9b62f928a562b59ea5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1986083
Commit-Queue: Alice Wang <aliceywang@chromium.org>
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Reviewed-by: Tanmoy Mollik <triploblastic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734451}

chrome/android/chrome_test_java_sources.gni

@@ -438,6 +438,7 @@ chrome_test_java_sources = [
   "javatests/src/org/chromium/chrome/browser/signin/ProfileDataCacheRenderTest.java",
   "javatests/src/org/chromium/chrome/browser/signin/SigninFragmentTest.java",
   "javatests/src/org/chromium/chrome/browser/signin/SigninHelperTest.java",
+  "javatests/src/org/chromium/chrome/browser/signin/SigninSignoutIntegrationTest.java",
   "javatests/src/org/chromium/chrome/browser/signin/SigninTest.java",
   "javatests/src/org/chromium/chrome/browser/sms/SmsReceiverInfoBarTest.java",
   "javatests/src/org/chromium/chrome/browser/ssl/CaptivePortalTest.java",

chrome/android/java/src/org/chromium/chrome/browser/settings/sync/AccountManagementFragment.java

@@ -391,15 +391,6 @@ public class AccountManagementFragment extends PreferenceFragmentCompat
                         }
                     }
                 }, forceWipeUserData);
-        SigninUtils.logEvent(ProfileAccountManagementMetrics.SIGNOUT_SIGNOUT, mGaiaServiceType);
-    }
-
-    @Override
-    public void onSignOutDialogDismissed(boolean signOutClicked) {
-        // TODO(https://crbug.com/1038924): Signout is recorded twice when cancelled
-        if (!signOutClicked) {
-            SigninUtils.logEvent(ProfileAccountManagementMetrics.SIGNOUT_CANCEL, mGaiaServiceType);
-        }
     }
 
     // SignInStateObserver implementation:

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

@@ -31,7 +31,7 @@ public class SignOutDialogFragment extends DialogFragment implements
     private static final String SHOW_GAIA_SERVICE_TYPE_EXTRA = "ShowGAIAServiceType";
 
     /**
-     * Receives updates when the user clicks "Sign out" or dismisses the dialog.
+     * Receives updates when the user clicks "Sign out".
      */
     public interface SignOutDialogListener {
         /**
@@ -40,17 +40,8 @@ public class SignOutDialogFragment extends DialogFragment implements
          * @param forceWipeUserData Whether the user selected to wipe local device data.
          */
         void onSignOutClicked(boolean forceWipeUserData);
-
-        /**
-         * Called when the dialog is dismissed.
-         *
-         * @param signOutClicked Whether the user clicked the "sign out" button before the dialog
-         *                       was dismissed.
-         */
-        void onSignOutDialogDismissed(boolean signOutClicked);
     }
 
-    private boolean mSignOutClicked;
     private CheckBox mWipeUserData;
 
     /**
@@ -108,8 +99,6 @@ public class SignOutDialogFragment extends DialogFragment implements
     public void onClick(DialogInterface dialog, int which) {
         if (which == AlertDialog.BUTTON_POSITIVE) {
             SigninUtils.logEvent(ProfileAccountManagementMetrics.SIGNOUT_SIGNOUT, mGaiaServiceType);
-
-            mSignOutClicked = true;
             if (IdentityServicesProvider.get().getSigninManager().getManagementDomain() == null) {
                 RecordHistogram.recordBooleanHistogram(
                         "Signin.UserRequestedWipeDataOnSignout", mWipeUserData.isChecked());
@@ -123,8 +112,5 @@ public class SignOutDialogFragment extends DialogFragment implements
     public void onDismiss(DialogInterface dialog) {
         super.onDismiss(dialog);
         SigninUtils.logEvent(ProfileAccountManagementMetrics.SIGNOUT_CANCEL, mGaiaServiceType);
-
-        SignOutDialogListener targetFragment = (SignOutDialogListener) getTargetFragment();
-        targetFragment.onSignOutDialogDismissed(mSignOutClicked);
     }
 }

chrome/android/javatests/src/org/chromium/chrome/browser/signin/SigninSignoutIntegrationTest.java

+// Copyright 2020 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 android.support.test.espresso.Espresso.onView;
+import static android.support.test.espresso.action.ViewActions.click;
+import static android.support.test.espresso.action.ViewActions.pressBack;
+import static android.support.test.espresso.matcher.RootMatchers.isDialog;
+import static android.support.test.espresso.matcher.ViewMatchers.isRoot;
+import static android.support.test.espresso.matcher.ViewMatchers.withId;
+import static android.support.test.espresso.matcher.ViewMatchers.withText;
+
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.MockitoAnnotations.initMocks;
+
+import android.accounts.Account;
+import android.support.test.InstrumentationRegistry;
+
+import androidx.test.filters.LargeTest;
+
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+
+import org.chromium.base.test.util.CommandLineFlags;
+import org.chromium.base.test.util.JniMocker;
+import org.chromium.chrome.R;
+import org.chromium.chrome.browser.ChromeActivity;
+import org.chromium.chrome.browser.ChromeSwitches;
+import org.chromium.chrome.browser.profiles.ProfileAccountManagementMetrics;
+import org.chromium.chrome.browser.settings.sync.AccountManagementFragment;
+import org.chromium.chrome.test.ChromeActivityTestRule;
+import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
+import org.chromium.chrome.test.util.ActivityUtils;
+import org.chromium.chrome.test.util.browser.signin.SigninTestUtil;
+import org.chromium.components.signin.ChromeSigninController;
+import org.chromium.components.signin.GAIAServiceType;
+import org.chromium.components.signin.metrics.SigninAccessPoint;
+import org.chromium.content_public.browser.test.util.TestThreadUtils;
+import org.chromium.ui.test.util.DisableAnimationsTestRule;
+
+/**
+ * Test the lifecycle of sign-in and sign-out.
+ */
+@RunWith(ChromeJUnit4ClassRunner.class)
+@CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE})
+public class SigninSignoutIntegrationTest {
+    @Rule
+    public final DisableAnimationsTestRule mNoAnimationsRule = new DisableAnimationsTestRule();
+
+    @Rule
+    public final JniMocker mocker = new JniMocker();
+
+    @Mock
+    private SigninUtils.Natives mSigninUtilsNativeMock;
+
+    @Rule
+    public final ChromeActivityTestRule<ChromeActivity> mActivityTestRule =
+            new ChromeActivityTestRule<>(ChromeActivity.class);
+
+    @Mock
+    private SigninManager.SignInStateObserver mSignInStateObserverMock;
+
+    private SigninManager mSigninManager;
+
+    @Before
+    public void setUp() {
+        initMocks(this);
+        mocker.mock(SigninUtilsJni.TEST_HOOKS, mSigninUtilsNativeMock);
+        SigninTestUtil.setUpAuthForTest();
+        mActivityTestRule.startMainActivityOnBlankPage();
+        TestThreadUtils.runOnUiThreadBlocking(
+                () -> { mSigninManager = IdentityServicesProvider.get().getSigninManager(); });
+        mSigninManager.addSignInStateObserver(mSignInStateObserverMock);
+    }
+
+    @After
+    public void tearDown() {
+        mSigninManager.removeSignInStateObserver(mSignInStateObserverMock);
+        SigninTestUtil.tearDownAuthForTest();
+    }
+
+    @Test
+    @LargeTest
+    public void testSignIn() {
+        Account account = SigninTestUtil.addTestAccount();
+        ActivityUtils.waitForActivity(
+                InstrumentationRegistry.getInstrumentation(), SigninActivity.class, () -> {
+                    SigninActivityLauncher.get().launchActivity(
+                            mActivityTestRule.getActivity(), SigninAccessPoint.SETTINGS);
+                });
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            Assert.assertFalse("Account should not be signed in!",
+                    mSigninManager.getIdentityManager().hasPrimaryAccount());
+            Assert.assertNull(mSigninManager.getIdentityManager().getPrimaryAccountInfo());
+        });
+        onView(withId(R.id.positive_button)).perform(click());
+        verify(mSignInStateObserverMock).onSignedIn();
+        verify(mSignInStateObserverMock, never()).onSignedOut();
+        assertSignedIn();
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            Assert.assertEquals(account,
+                    mSigninManager.getIdentityManager().getPrimaryAccountInfo().getAccount());
+        });
+    }
+
+    @Test
+    @LargeTest
+    public void testSignOut() {
+        signIn();
+        mActivityTestRule.startSettingsActivity(AccountManagementFragment.class.getName());
+        onView(withText(R.string.sign_out_and_turn_off_sync)).perform(click());
+        onView(withText(R.string.continue_button)).inRoot(isDialog()).perform(click());
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            Assert.assertFalse("Account should be signed out!",
+                    mSigninManager.getIdentityManager().hasPrimaryAccount());
+            Assert.assertNull(mSigninManager.getIdentityManager().getPrimaryAccountInfo());
+        });
+        verify(mSignInStateObserverMock).onSignedOut();
+        verify(mSigninUtilsNativeMock)
+                .logEvent(ProfileAccountManagementMetrics.TOGGLE_SIGNOUT,
+                        GAIAServiceType.GAIA_SERVICE_TYPE_NONE);
+        verify(mSigninUtilsNativeMock)
+                .logEvent(ProfileAccountManagementMetrics.SIGNOUT_SIGNOUT,
+                        GAIAServiceType.GAIA_SERVICE_TYPE_NONE);
+    }
+
+    @Test
+    @LargeTest
+    public void testSignOutDismissedByPressingBack() {
+        signIn();
+        mActivityTestRule.startSettingsActivity(AccountManagementFragment.class.getName());
+        onView(withText(R.string.sign_out_and_turn_off_sync)).perform(click());
+        onView(isRoot()).perform(pressBack());
+        verify(mSignInStateObserverMock, never()).onSignedOut();
+        assertSignedIn();
+        verify(mSigninUtilsNativeMock)
+                .logEvent(ProfileAccountManagementMetrics.TOGGLE_SIGNOUT,
+                        GAIAServiceType.GAIA_SERVICE_TYPE_NONE);
+        verify(mSigninUtilsNativeMock)
+                .logEvent(ProfileAccountManagementMetrics.SIGNOUT_CANCEL,
+                        GAIAServiceType.GAIA_SERVICE_TYPE_NONE);
+    }
+
+    @Test
+    @LargeTest
+    public void testSignOutCancelled() {
+        signIn();
+        mActivityTestRule.startSettingsActivity(AccountManagementFragment.class.getName());
+        onView(withText(R.string.sign_out_and_turn_off_sync)).perform(click());
+        onView(withText(R.string.cancel)).inRoot(isDialog()).perform(click());
+        verify(mSignInStateObserverMock, never()).onSignedOut();
+        assertSignedIn();
+        verify(mSigninUtilsNativeMock)
+                .logEvent(ProfileAccountManagementMetrics.TOGGLE_SIGNOUT,
+                        GAIAServiceType.GAIA_SERVICE_TYPE_NONE);
+        verify(mSigninUtilsNativeMock)
+                .logEvent(ProfileAccountManagementMetrics.SIGNOUT_CANCEL,
+                        GAIAServiceType.GAIA_SERVICE_TYPE_NONE);
+    }
+
+    private void signIn() {
+        Account account = SigninTestUtil.addTestAccount();
+        TestThreadUtils.runOnUiThreadBlocking(
+                () -> { mSigninManager.signIn(SigninAccessPoint.SETTINGS, account, null); });
+        assertSignedIn();
+        // TODO(https://crbug.com/1041815): Usage of ChromeSigninController should be removed later
+        Assert.assertTrue(ChromeSigninController.get().isSignedIn());
+    }
+
+    private void assertSignedIn() {
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            Assert.assertTrue("Account should be signed in!",
+                    mSigninManager.getIdentityManager().hasPrimaryAccount());
+        });
+    }
+}

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

@@ -43,9 +43,6 @@ public class SignOutDialogFragmentTest {
             extends Fragment implements SignOutDialogFragment.SignOutDialogListener {
         @Override
         public void onSignOutClicked(boolean forceWipeUserData) {}
-
-        @Override
-        public void onSignOutDialogDismissed(boolean signOutClicked) {}
     }
 
     @Rule
@@ -87,7 +84,7 @@ public class SignOutDialogFragmentTest {
     @Test
     public void testMessageWhenAccountIsManaged() {
         when(mSigninManagerMock.getManagementDomain()).thenReturn(TEST_DOMAIN);
-        AlertDialog alertDialog = getSignOutAlertDialogAfterShowingIt();
+        AlertDialog alertDialog = showSignOutAlertDialog();
         TextView messageTextView = alertDialog.findViewById(android.R.id.message);
         assertEquals(
                 mSignOutDialog.getString(R.string.signout_managed_account_message, TEST_DOMAIN),
@@ -97,7 +94,7 @@ public class SignOutDialogFragmentTest {
     @Test
     public void testPositiveButtonWhenAccountIsManaged() {
         when(mSigninManagerMock.getManagementDomain()).thenReturn(TEST_DOMAIN);
-        AlertDialog alertDialog = getSignOutAlertDialogAfterShowingIt();
+        AlertDialog alertDialog = showSignOutAlertDialog();
         alertDialog.getButton(AlertDialog.BUTTON_POSITIVE).performClick();
         verify(mSigninUtilsNativeMock)
                 .logEvent(ProfileAccountManagementMetrics.SIGNOUT_SIGNOUT,
@@ -107,7 +104,7 @@ public class SignOutDialogFragmentTest {
 
     @Test
     public void testPositiveButtonWhenAccountIsNotManagedAndRemoveLocalDataNotChecked() {
-        AlertDialog alertDialog = getSignOutAlertDialogAfterShowingIt();
+        AlertDialog alertDialog = showSignOutAlertDialog();
         alertDialog.getButton(AlertDialog.BUTTON_POSITIVE).performClick();
         verify(mSigninUtilsNativeMock)
                 .logEvent(ProfileAccountManagementMetrics.SIGNOUT_SIGNOUT,
@@ -117,7 +114,7 @@ public class SignOutDialogFragmentTest {
 
     @Test
     public void testPositiveButtonWhenAccountIsNotManagedAndRemoveLocalDataChecked() {
-        AlertDialog alertDialog = getSignOutAlertDialogAfterShowingIt();
+        AlertDialog alertDialog = showSignOutAlertDialog();
         alertDialog.findViewById(R.id.remove_local_data).performClick();
         alertDialog.getButton(AlertDialog.BUTTON_POSITIVE).performClick();
         verify(mSigninUtilsNativeMock)
@@ -127,42 +124,37 @@ public class SignOutDialogFragmentTest {
     }
 
     @Test
-    public void testNegativeButtonHasNoEffectWhenAccountIsManaged() {
+    public void testNegativeButtonWhenAccountIsManaged() {
         when(mSigninManagerMock.getManagementDomain()).thenReturn(TEST_DOMAIN);
-        AlertDialog alertDialog = getSignOutAlertDialogAfterShowingIt();
+        AlertDialog alertDialog = showSignOutAlertDialog();
         alertDialog.getButton(AlertDialog.BUTTON_NEGATIVE).performClick();
         verify(mTargetFragment, never()).onSignOutClicked(anyBoolean());
+        verify(mSigninUtilsNativeMock)
+                .logEvent(ProfileAccountManagementMetrics.SIGNOUT_CANCEL,
+                        GAIAServiceType.GAIA_SERVICE_TYPE_NONE);
     }
 
     @Test
-    public void testNegativeButtonHasNoEffectWhenAccountIsNotManaged() {
-        AlertDialog alertDialog = getSignOutAlertDialogAfterShowingIt();
+    public void testNegativeButtonWhenAccountIsNotManaged() {
+        AlertDialog alertDialog = showSignOutAlertDialog();
         alertDialog.getButton(AlertDialog.BUTTON_NEGATIVE).performClick();
         verify(mTargetFragment, never()).onSignOutClicked(anyBoolean());
-    }
-
-    @Test
-    public void testDismissWhenPositiveButtonIsNotClicked() {
-        AlertDialog alertDialog = getSignOutAlertDialogAfterShowingIt();
-        alertDialog.dismiss();
         verify(mSigninUtilsNativeMock)
                 .logEvent(ProfileAccountManagementMetrics.SIGNOUT_CANCEL,
                         GAIAServiceType.GAIA_SERVICE_TYPE_NONE);
-        verify(mTargetFragment).onSignOutDialogDismissed(false);
     }
 
     @Test
-    public void testDismissWhenPositiveButtonIsClicked() {
-        AlertDialog alertDialog = getSignOutAlertDialogAfterShowingIt();
-        alertDialog.getButton(AlertDialog.BUTTON_POSITIVE).performClick();
+    public void testEventLoggedWhenDialogDismissed() {
+        AlertDialog alertDialog = showSignOutAlertDialog();
         alertDialog.dismiss();
+        verify(mTargetFragment, never()).onSignOutClicked(anyBoolean());
         verify(mSigninUtilsNativeMock)
                 .logEvent(ProfileAccountManagementMetrics.SIGNOUT_CANCEL,
                         GAIAServiceType.GAIA_SERVICE_TYPE_NONE);
-        verify(mTargetFragment).onSignOutDialogDismissed(true);
     }
 
-    private AlertDialog getSignOutAlertDialogAfterShowingIt() {
+    private AlertDialog showSignOutAlertDialog() {
         mSignOutDialog.show(mFragmentManager, null);
         return (AlertDialog) ShadowAlertDialog.getLatestDialog();
     }