[Android] Fix dialog-induced crash in AccountSigninView

https://crrev.com/2772203004 introduced a crash as dialogs can't be
dismissed when container activity has been destroyed. This CL fixes
this crash by storing an instance of ConfirmSyncDataStateMachine in
AccountSigninView and calling cancel() when the view becomes invisible.
This CL also reimplements ConfirmSyncDataStateMachineDelegate dialogs
as DialogFragments.

Bug: 774421
Change-Id: I2af2a8571921d7b7587b1f25ab771df14af5dbc3
Reviewed-on: https://chromium-review.googlesource.com/730708
Commit-Queue: Boris Sazonov <bsazonov@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512995}

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

@@ -142,6 +142,7 @@ public class AccountSigninView extends FrameLayout {
     private TextView mSigninAccountEmail;
     private TextView mSigninPersonalizeServiceDescription;
     private TextView mSigninSettingsControl;
+    private ConfirmSyncDataStateMachine mConfirmSyncDataStateMachine;
 
     public AccountSigninView(Context context, AttributeSet attrs) {
         super(context, attrs);
@@ -259,6 +260,10 @@ public class AccountSigninView extends FrameLayout {
 
     @Override
     protected void onDetachedFromWindow() {
+        if (mConfirmSyncDataStateMachine != null) {
+            mConfirmSyncDataStateMachine.cancel(false /* dismissDialogs */);
+            mConfirmSyncDataStateMachine = null;
+        }
         if (mProfileDataCache != null) {
             mProfileDataCache.removeObserver(mProfileDataCacheObserver);
         }
@@ -362,10 +367,11 @@ public class AccountSigninView extends FrameLayout {
                 && (mAccountNames.isEmpty()
                            || !mAccountNames.get(accountToSelect)
                                        .equals(oldAccountNames.get(oldSelectedAccount)));
-        if (selectedAccountChanged) {
+        if (selectedAccountChanged && mConfirmSyncDataStateMachine != null) {
             // Any dialogs that may have been showing are now invalid (they were created
             // for the previously selected account).
-            ConfirmSyncDataStateMachine.cancelAllDialogs(mDelegate.getFragmentManager());
+            mConfirmSyncDataStateMachine.cancel(true /* dismissDialogs */);
+            mConfirmSyncDataStateMachine = null;
         }
 
         if (shouldJumpToConfirmationScreen) {
@@ -557,18 +563,20 @@ public class AccountSigninView extends FrameLayout {
     private void showConfirmSigninPagePreviousAccountCheck(long seedingStartTime) {
         RecordHistogram.recordTimesHistogram("Signin.AndroidAccountSigninViewSeedingTime",
                 SystemClock.elapsedRealtime() - seedingStartTime, TimeUnit.MILLISECONDS);
-        ConfirmSyncDataStateMachine.run(PrefServiceBridge.getInstance().getSyncLastAccountName(),
-                mSelectedAccountName, ImportSyncType.PREVIOUS_DATA_FOUND,
-                mDelegate.getFragmentManager(), getContext(),
+        mConfirmSyncDataStateMachine = new ConfirmSyncDataStateMachine(getContext(),
+                mDelegate.getFragmentManager(), ImportSyncType.PREVIOUS_DATA_FOUND,
+                PrefServiceBridge.getInstance().getSyncLastAccountName(), mSelectedAccountName,
                 new ConfirmImportSyncDataDialog.Listener() {
                     @Override
                     public void onConfirm(boolean wipeData) {
+                        mConfirmSyncDataStateMachine = null;
                         SigninManager.wipeSyncUserDataIfRequired(wipeData).then(
                                 (Void v) -> showConfirmSigninPage());
                     }
 
                     @Override
                     public void onCancel() {
+                        mConfirmSyncDataStateMachine = null;
                         setButtonsEnabled(true);
                         onSigninConfirmationCancel();
                     }

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

@@ -5,11 +5,11 @@
 package org.chromium.chrome.browser.signin;
 
 import android.app.DialogFragment;
-import android.app.Fragment;
 import android.app.FragmentManager;
 import android.content.Context;
 import android.os.Handler;
 import android.support.annotation.IntDef;
+import android.support.annotation.Nullable;
 import android.text.TextUtils;
 
 import org.chromium.base.ThreadUtils;
@@ -62,7 +62,7 @@ public class ConfirmSyncDataStateMachine
     private static final int ACCOUNT_CHECK_TIMEOUT_MS = 30000;
 
     private final ConfirmImportSyncDataDialog.Listener mCallback;
-    private final String mOldAccountName;
+    private final @Nullable String mOldAccountName;
     private final String mNewAccountName;
     private final boolean mCurrentlyManaged;
     private final FragmentManager mFragmentManager;
@@ -76,48 +76,22 @@ public class ConfirmSyncDataStateMachine
     private Runnable mCheckTimeoutRunnable;
 
     /**
-     * Run this state machine, displaying the appropriate dialogs.
-     * @param callback One of the two functions of the {@link ConfirmImportSyncDataDialog.Listener}
-     *         are guaranteed to be called.
+     * Create and run state machine, displaying the appropriate dialogs.
+     * @param importSyncType SWITCHING_SYNC_ACCOUNTS if there's already a signed in account,
+     *         PREVIOUS_DATA_FOUND otherwise
+     * @param oldAccountName if importSyncType is SWITCHING_SYNC_ACCOUNTS: the name of the signed in
+     *         account; if importSyncType is PREVIOUS_DATA_FOUND: the name of the last signed in
+     *         account or null
+     * @param newAccountName the name of the account user is signing in with
+     * @param callback the listener to receive the result of this state machine
      */
-    public static void run(String oldAccountName, String newAccountName,
-            ImportSyncType importSyncType, FragmentManager fragmentManager, Context context,
+    public ConfirmSyncDataStateMachine(Context context, FragmentManager fragmentManager,
+            ImportSyncType importSyncType, @Nullable String oldAccountName, String newAccountName,
             ConfirmImportSyncDataDialog.Listener callback) {
+        ThreadUtils.assertOnUiThread();
         // Includes implicit not-null assertion.
         assert !newAccountName.equals("") : "New account name must be provided.";
 
-        ConfirmSyncDataStateMachine stateMachine = new ConfirmSyncDataStateMachine(oldAccountName,
-                newAccountName, importSyncType, fragmentManager, context, callback);
-        stateMachine.progress();
-    }
-
-    /**
-     * If any of the dialogs used by this state machine are shown, cancel them. If this state
-     * machine is running and a dialog is being shown, the given
-     * {@link ConfirmImportSyncDataDialog.Listener#onCancel())} is called.
-     */
-    public static void cancelAllDialogs(FragmentManager fragmentManager) {
-        cancelDialog(fragmentManager,
-                ConfirmImportSyncDataDialog.CONFIRM_IMPORT_SYNC_DATA_DIALOG_TAG);
-        cancelDialog(fragmentManager,
-                ConfirmManagedSyncDataDialog.CONFIRM_IMPORT_SYNC_DATA_DIALOG_TAG);
-    }
-
-    private static void cancelDialog(FragmentManager fragmentManager, String tag) {
-        Fragment fragment = fragmentManager.findFragmentByTag(tag);
-
-        if (fragment == null) return;
-        DialogFragment dialogFragment = (DialogFragment) fragment;
-
-        if (dialogFragment.getDialog() == null) return;
-        dialogFragment.getDialog().cancel();
-    }
-
-    private ConfirmSyncDataStateMachine(String oldAccountName, String newAccountName,
-            ImportSyncType importSyncType, FragmentManager fragmentManager, Context context,
-            ConfirmImportSyncDataDialog.Listener callback) {
-        ThreadUtils.assertOnUiThread();
-
         mOldAccountName = oldAccountName;
         mNewAccountName = newAccountName;
         mImportSyncType = importSyncType;
@@ -127,11 +101,37 @@ public class ConfirmSyncDataStateMachine
 
         mCurrentlyManaged = SigninManager.get(context).getManagementDomain() != null;
 
-        mDelegate = new ConfirmSyncDataStateMachineDelegate(mContext);
+        mDelegate = new ConfirmSyncDataStateMachineDelegate(mFragmentManager);
 
         // New account management status isn't needed right now, but fetching it
         // can take a few seconds, so we kick it off early.
         requestNewAccountManagementStatus();
+
+        progress();
+    }
+
+    /**
+     * Cancels this state machine, dismissing any dialogs being shown.
+     * @param dismissDialogs whether all shown dialogs should dismissed. Should be false if
+     *         enclosing activity is being destroyed (all dialogs will be dismissed anyway).
+     */
+    public void cancel(boolean dismissDialogs) {
+        ThreadUtils.assertOnUiThread();
+
+        cancelTimeout();
+        mState = DONE;
+        mCallback.onCancel();
+
+        if (!dismissDialogs) return;
+        mDelegate.dismissAllDialogs();
+        dismissDialog(ConfirmImportSyncDataDialog.CONFIRM_IMPORT_SYNC_DATA_DIALOG_TAG);
+        dismissDialog(ConfirmManagedSyncDataDialog.CONFIRM_IMPORT_SYNC_DATA_DIALOG_TAG);
+    }
+
+    private void dismissDialog(String tag) {
+        DialogFragment fragment = (DialogFragment) mFragmentManager.findFragmentByTag(tag);
+        if (fragment == null) return;
+        fragment.dismissAllowingStateLoss();
     }
 
     /**
@@ -272,11 +272,7 @@ public class ConfirmSyncDataStateMachine
     // ConfirmImportSyncDataDialog.Listener & ConfirmManagedSyncDataDialog.Listener implementation.
     @Override
     public void onCancel() {
-        cancelTimeout();
-        mDelegate.dismissAllDialogs();
-
-        mState = DONE;
-        mCallback.onCancel();
+        cancel(true);
     }
 }
 

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

@@ -5,7 +5,11 @@
 package org.chromium.chrome.browser.signin;
 
 import android.app.Dialog;
-import android.content.Context;
+import android.app.DialogFragment;
+import android.app.FragmentManager;
+import android.app.FragmentTransaction;
+import android.content.DialogInterface;
+import android.os.Bundle;
 import android.support.v7.app.AlertDialog;
 
 import org.chromium.chrome.R;
@@ -43,13 +47,99 @@ public class ConfirmSyncDataStateMachineDelegate {
         void onRetry();
     }
 
-    private final Context mContext;
+    /**
+     * Progress Dialog that is shown while account management policy is being fetched.
+     */
+    public static class ProgressDialogFragment extends DialogFragment {
+        private ProgressDialogListener mListener;
+
+        public ProgressDialogFragment() {
+            // Fragment must have an empty public constructor
+        }
+
+        @Override
+        public Dialog onCreateDialog(Bundle savedInstanceState) {
+            // If the dialog is being recreated it won't have the listener set and so won't be
+            // functional. Therefore we dismiss, and the user will need to open the dialog again.
+            if (savedInstanceState != null) {
+                dismiss();
+            }
+
+            return new AlertDialog.Builder(getActivity(), R.style.SigninAlertDialogTheme)
+                    .setView(R.layout.signin_progress_bar_dialog)
+                    .setNegativeButton(R.string.cancel, (dialog, i) -> dialog.cancel())
+                    .create();
+        }
+
+        private void setListener(ProgressDialogListener listener) {
+            assert mListener == null;
+            mListener = listener;
+        }
 
-    private Dialog mProgressDialog;
-    private AlertDialog mTimeoutAlertDialog;
+        private static ProgressDialogFragment create(ProgressDialogListener listener) {
+            ProgressDialogFragment result = new ProgressDialogFragment();
+            result.setListener(listener);
+            return result;
+        }
 
-    public ConfirmSyncDataStateMachineDelegate(Context context) {
-        mContext = context;
+        @Override
+        public void onCancel(DialogInterface dialog) {
+            super.onCancel(dialog);
+            mListener.onCancel();
+        }
+    }
+
+    /**
+     * Timeout Dialog that is shown if account management policy fetch times out.
+     */
+    public static class TimeoutDialogFragment extends DialogFragment {
+        private TimeoutDialogListener mListener;
+
+        public TimeoutDialogFragment() {
+            // Fragment must have an empty public constructor
+        }
+
+        @Override
+        public Dialog onCreateDialog(Bundle savedInstanceState) {
+            // If the dialog is being recreated it won't have the listener set and so won't be
+            // functional. Therefore we dismiss, and the user will need to open the dialog again.
+            if (savedInstanceState != null) {
+                dismiss();
+            }
+
+            return new AlertDialog.Builder(getActivity(), R.style.SigninAlertDialogTheme)
+                    .setTitle(R.string.sign_in_timeout_title)
+                    .setMessage(R.string.sign_in_timeout_message)
+                    .setNegativeButton(R.string.cancel, (dialog, which) -> dialog.cancel())
+                    .setPositiveButton(R.string.retry, (dialog, which) -> mListener.onRetry())
+                    .create();
+        }
+
+        private void setListener(TimeoutDialogListener listener) {
+            assert mListener == null;
+            mListener = listener;
+        }
+
+        private static TimeoutDialogFragment create(TimeoutDialogListener listener) {
+            TimeoutDialogFragment result = new TimeoutDialogFragment();
+            result.setListener(listener);
+            return result;
+        }
+
+        @Override
+        public void onCancel(DialogInterface dialog) {
+            super.onCancel(dialog);
+            mListener.onCancel();
+        }
+    }
+
+    private static final String PROGRESS_DIALOG_TAG = "ConfirmSyncTimeoutDialog";
+    private static final String TIMEOUT_DIALOG_TAG = "ConfirmSyncProgressDialog";
+
+    private final FragmentManager mFragmentManager;
+
+    public ConfirmSyncDataStateMachineDelegate(FragmentManager fragmentManager) {
+        mFragmentManager = fragmentManager;
     }
 
     /**
@@ -59,13 +149,7 @@ public class ConfirmSyncDataStateMachineDelegate {
      */
     public void showFetchManagementPolicyProgressDialog(final ProgressDialogListener listener) {
         dismissAllDialogs();
-        mProgressDialog =
-                new AlertDialog.Builder(mContext, R.style.SigninAlertDialogTheme)
-                        .setView(R.layout.signin_progress_bar_dialog)
-                        .setNegativeButton(R.string.cancel, (dialog, i) -> dialog.cancel())
-                        .setOnCancelListener(dialog -> listener.onCancel())
-                        .create();
-        mProgressDialog.show();
+        showAllowingStateLoss(ProgressDialogFragment.create(listener), PROGRESS_DIALOG_TAG);
     }
 
     /**
@@ -75,25 +159,26 @@ public class ConfirmSyncDataStateMachineDelegate {
      */
     public void showFetchManagementPolicyTimeoutDialog(final TimeoutDialogListener listener) {
         dismissAllDialogs();
-        mTimeoutAlertDialog =
-                new AlertDialog.Builder(mContext, R.style.SigninAlertDialogTheme)
-                        .setTitle(R.string.sign_in_timeout_title)
-                        .setMessage(R.string.sign_in_timeout_message)
-                        .setNegativeButton(R.string.cancel, (dialog, which) -> dialog.cancel())
-                        .setPositiveButton(R.string.retry, (dialog, which) -> listener.onRetry())
-                        .setOnCancelListener(dialog -> listener.onCancel())
-                        .create();
-        mTimeoutAlertDialog.show();
+        showAllowingStateLoss(TimeoutDialogFragment.create(listener), TIMEOUT_DIALOG_TAG);
+    }
+
+    private void showAllowingStateLoss(DialogFragment dialogFragment, String tag) {
+        FragmentTransaction transaction = mFragmentManager.beginTransaction();
+        transaction.add(dialogFragment, tag);
+        transaction.commitAllowingStateLoss();
     }
 
     /**
      * Dismisses all dialogs.
      */
     public void dismissAllDialogs() {
-        if (mProgressDialog != null) mProgressDialog.dismiss();
-        mProgressDialog = null;
+        dismissDialog(PROGRESS_DIALOG_TAG);
+        dismissDialog(TIMEOUT_DIALOG_TAG);
+    }
 
-        if (mTimeoutAlertDialog != null) mTimeoutAlertDialog.dismiss();
-        mTimeoutAlertDialog = null;
+    private void dismissDialog(String tag) {
+        DialogFragment fragment = (DialogFragment) mFragmentManager.findFragmentByTag(tag);
+        if (fragment == null) return;
+        fragment.dismissAllowingStateLoss();
     }
 }

chrome/android/java/src/org/chromium/chrome/browser/sync/SyncAccountSwitcher.java

@@ -57,10 +57,8 @@ public class SyncAccountSwitcher
 
         if (TextUtils.equals(mNewAccountName, currentAccount)) return false;
 
-        ConfirmSyncDataStateMachine.run(currentAccount, mNewAccountName,
-                ImportSyncType.SWITCHING_SYNC_ACCOUNTS, mActivity.getFragmentManager(),
-                mActivity, this);
-
+        new ConfirmSyncDataStateMachine(mActivity, mActivity.getFragmentManager(),
+                ImportSyncType.SWITCHING_SYNC_ACCOUNTS, currentAccount, mNewAccountName, this);
 
         // Don't update the selected account in the preference. It will be updated by
         // the call to mSyncAccountListPreference.update() if everything succeeds.