[Android] Revise ProfileDataCache and its usage

1. Change ProfileDownloader observer registration - it is now registered
   on-demand iff there are observers of this ProfileDataCache instance.
   It allows removing ProfileDataCache.destroy().
2. Remove FirstRunPageDelegate.getProfileDataCache() - now there's no need for
   Activity to keep track of ProfileDataCache.
3. Fix AccountSigninView - remove ProfileDataCache observer when view is
   detached from window.

Bug: 746519
Change-Id: Id2245038a507aaf09a2afec4225e5fd3a6f49c07
Reviewed-on: https://chromium-review.googlesource.com/577872
Commit-Queue: Boris Sazonov <bsazonov@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488230}

chrome/android/java/src/org/chromium/chrome/browser/firstrun/AccountFirstRunFragment.java

@@ -12,6 +12,7 @@ import android.view.ViewGroup;
 
 import org.chromium.base.metrics.RecordUserAction;
 import org.chromium.chrome.R;
+import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.chrome.browser.signin.AccountSigninView;
 import org.chromium.chrome.browser.signin.SigninAccessPoint;
 import org.chromium.chrome.browser.signin.SigninManager;
@@ -39,8 +40,9 @@ public class AccountFirstRunFragment extends FirstRunPage implements AccountSign
     public void onViewCreated(View view, Bundle savedInstanceState) {
         super.onViewCreated(view, savedInstanceState);
 
-        mView.init(getPageDelegate().getProfileDataCache(),
-                getProperties().getBoolean(IS_CHILD_ACCOUNT),
+        ProfileDataCache profileDataCache =
+                new ProfileDataCache(getActivity(), Profile.getLastUsedProfile());
+        mView.init(profileDataCache, getProperties().getBoolean(IS_CHILD_ACCOUNT),
                 getProperties().getString(FORCE_SIGNIN_ACCOUNT_TO), this,
                 new AccountSigninView.Listener() {
                     @Override

chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java

@@ -28,7 +28,6 @@ import org.chromium.chrome.browser.metrics.UmaUtils;
 import org.chromium.chrome.browser.net.spdyproxy.DataReductionProxySettings;
 import org.chromium.chrome.browser.preferences.datareduction.DataReductionPromoUtils;
 import org.chromium.chrome.browser.preferences.datareduction.DataReductionProxyUma;
-import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.chrome.browser.search_engines.TemplateUrlService;
 import org.chromium.chrome.browser.searchwidget.SearchWidgetProvider;
 import org.chromium.chrome.browser.util.IntentUtils;
@@ -134,7 +133,6 @@ public class FirstRunActivity extends AsyncInitializationActivity implements Fir
     private Set<FirstRunPage> mPagesToNotifyOfNativeInit;
     private boolean mDeferredCompleteFRE;
 
-    private ProfileDataCache mProfileDataCache;
     private FirstRunViewPager mPager;
 
     private FirstRunFlowSequencer mFirstRunFlowSequencer;
@@ -365,12 +363,6 @@ public class FirstRunActivity extends AsyncInitializationActivity implements Fir
         flushPersistentData();
     }
 
-    @Override
-    protected void onDestroy() {
-        super.onDestroy();
-        if (mProfileDataCache != null) mProfileDataCache.destroy();
-    }
-
     @Override
     public void onStart() {
         super.onStart();
@@ -412,16 +404,6 @@ public class FirstRunActivity extends AsyncInitializationActivity implements Fir
     }
 
     // FirstRunPageDelegate:
-
-    @Override
-    public ProfileDataCache getProfileDataCache() {
-        if (mProfileDataCache == null) {
-            mProfileDataCache =
-                    new ProfileDataCache(FirstRunActivity.this, Profile.getLastUsedProfile());
-        }
-        return mProfileDataCache;
-    }
-
     @Override
     public void advanceToNextPage() {
         jumpToPage(mPager.getCurrentItem() + 1);

chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunPageDelegate.java

@@ -10,12 +10,6 @@ import android.app.Fragment;
  * Defines the host interface for First Run Experience pages.
  */
 public interface FirstRunPageDelegate {
-    /**
-     * Must be called only after native has been initialized.
-     * @return A {@link ProfileDataCache} for Android user accounts.
-     */
-    ProfileDataCache getProfileDataCache();
-
     /**
      * Advances the First Run Experience to the next page.
      * Successfully finishes FRE if the current page is the last page.

chrome/android/java/src/org/chromium/chrome/browser/firstrun/ProfileDataCache.java

@@ -15,9 +15,11 @@ import android.graphics.PorterDuffXfermode;
 import android.graphics.Rect;
 import android.graphics.drawable.BitmapDrawable;
 import android.graphics.drawable.Drawable;
+import android.support.annotation.MainThread;
 import android.support.v7.content.res.AppCompatResources;
 
 import org.chromium.base.ObserverList;
+import org.chromium.base.ThreadUtils;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.chrome.browser.profiles.ProfileDownloader;
@@ -30,6 +32,7 @@ import java.util.List;
  * ProfileDataCache doesn't observe account list changes by itself, so account list
  * should be provided by calling {@link #update(List)}
  */
+@MainThread
 public class ProfileDataCache implements ProfileDownloader.Observer {
     /**
      * Observer to get notifications about changes in profile data.
@@ -68,15 +71,17 @@ public class ProfileDataCache implements ProfileDownloader.Observer {
 
         mPlaceholderImage =
                 AppCompatResources.getDrawable(context, R.drawable.logo_avatar_anonymous);
-
-        ProfileDownloader.addObserver(this);
     }
 
     /**
-     * Initiate fetching the user accounts data (images and the full name).
-     * Fetched data will be sent to observers of ProfileDownloader.
+     * Initiate fetching the user accounts data (images and the full name). Fetched data will be
+     * sent to observers of ProfileDownloader. The instance must have at least one observer (see
+     * {@link #addObserver}) when this method is called.
      */
     public void update(List<String> accounts) {
+        ThreadUtils.assertOnUiThread();
+        assert !mObservers.isEmpty();
+
         int imageSizePx =
                 mContext.getResources().getDimensionPixelSize(R.dimen.signin_account_image_size);
         for (int i = 0; i < accounts.size(); i++) {
@@ -120,15 +125,14 @@ public class ProfileDataCache implements ProfileDownloader.Observer {
         return cacheEntry.givenName;
     }
 
-    public void destroy() {
-        ProfileDownloader.removeObserver(this);
-        mObservers.clear();
-    }
-
     /**
      * @param observer Observer that should be notified when new profile images are available.
      */
     public void addObserver(Observer observer) {
+        ThreadUtils.assertOnUiThread();
+        if (mObservers.isEmpty()) {
+            ProfileDownloader.addObserver(this);
+        }
         mObservers.addObserver(observer);
     }
 
@@ -136,12 +140,17 @@ public class ProfileDataCache implements ProfileDownloader.Observer {
      * @param observer Observer that was added by {@link #addObserver} and should be removed.
      */
     public void removeObserver(Observer observer) {
+        ThreadUtils.assertOnUiThread();
         mObservers.removeObserver(observer);
+        if (mObservers.isEmpty()) {
+            ProfileDownloader.removeObserver(this);
+        }
     }
 
     @Override
     public void onProfileDownloaded(String accountId, String fullName, String givenName,
             Bitmap bitmap) {
+        ThreadUtils.assertOnUiThread();
         Drawable drawable = bitmap != null ? getCroppedAvatar(bitmap) : mPlaceholderImage;
         mCacheEntries.put(accountId, new CacheEntry(drawable, fullName, givenName));
         for (Observer observer : mObservers) {

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

@@ -38,7 +38,6 @@ public class AccountSigninActivity extends AppCompatActivity
             "AccountSigninActivity.SigninAccessPoint";
 
     private AccountSigninView mView;
-    private ProfileDataCache mProfileDataCache;
 
     @IntDef({SigninAccessPoint.SETTINGS, SigninAccessPoint.BOOKMARK_MANAGER,
             SigninAccessPoint.RECENT_TABS, SigninAccessPoint.SIGNIN_PROMO,
@@ -104,7 +103,9 @@ public class AccountSigninActivity extends AppCompatActivity
 
         mView = (AccountSigninView) LayoutInflater.from(this).inflate(
                 R.layout.account_signin_view, null);
-        mView.init(getProfileDataCache(), false, null, this, this);
+        ProfileDataCache profileDataCache =
+                new ProfileDataCache(this, Profile.getLastUsedProfile());
+        mView.init(profileDataCache, false, null, this, this);
 
         if (getAccessPoint() == SigninAccessPoint.BOOKMARK_MANAGER
                 || getAccessPoint() == SigninAccessPoint.RECENT_TABS) {
@@ -117,23 +118,6 @@ public class AccountSigninActivity extends AppCompatActivity
         recordSigninStartedUserAction();
     }
 
-    @Override
-    public void onDestroy() {
-        super.onDestroy();
-
-        if (mProfileDataCache != null) {
-            mProfileDataCache.destroy();
-            mProfileDataCache = null;
-        }
-    }
-
-    private ProfileDataCache getProfileDataCache() {
-        if (mProfileDataCache == null) {
-            mProfileDataCache = new ProfileDataCache(this, Profile.getLastUsedProfile());
-        }
-        return mProfileDataCache;
-    }
-
     @AccessPoint private int getAccessPoint() {
         return mAccessPoint;
     }

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

@@ -8,6 +8,7 @@ import android.app.Activity;
 import android.app.FragmentManager;
 import android.content.Context;
 import android.os.SystemClock;
+import android.support.v4.view.ViewCompat;
 import android.support.v7.app.AlertDialog;
 import android.text.TextUtils;
 import android.text.method.LinkMovementMethod;
@@ -105,7 +106,6 @@ public class AccountSigninView extends FrameLayout {
     private static final String SETTINGS_LINK_OPEN = "<LINK1>";
     private static final String SETTINGS_LINK_CLOSE = "</LINK1>";
 
-    private AccountManagerFacade mAccountManagerFacade;
     private List<String> mAccountNames;
     private AccountSigninChooseView mSigninChooseView;
     private ButtonCompat mPositiveButton;
@@ -115,6 +115,7 @@ public class AccountSigninView extends FrameLayout {
     private Delegate mDelegate;
     private String mForcedAccountName;
     private ProfileDataCache mProfileData;
+    private final ProfileDataCache.Observer mProfileDataCacheObserver;
     private boolean mSignedIn;
     private int mCancelButtonTextId;
     private boolean mIsChildAccount;
@@ -130,7 +131,12 @@ public class AccountSigninView extends FrameLayout {
 
     public AccountSigninView(Context context, AttributeSet attrs) {
         super(context, attrs);
-        mAccountManagerFacade = AccountManagerFacade.get();
+        mProfileDataCacheObserver = new ProfileDataCache.Observer() {
+            @Override
+            public void onProfileDataUpdated(String accountId) {
+                updateProfileData();
+            }
+        };
     }
 
     /**
@@ -144,16 +150,14 @@ public class AccountSigninView extends FrameLayout {
     public void init(ProfileDataCache profileData, boolean isChildAccount, String forcedAccountName,
             Delegate delegate, Listener listener) {
         mProfileData = profileData;
-        mProfileData.addObserver(new ProfileDataCache.Observer() {
-            @Override
-            public void onProfileDataUpdated(String accountId) {
-                updateProfileData();
-            }
-        });
         mIsChildAccount = isChildAccount;
         mForcedAccountName = TextUtils.isEmpty(forcedAccountName) ? null : forcedAccountName;
         mDelegate = delegate;
         mListener = listener;
+
+        if (ViewCompat.isAttachedToWindow(this)) {
+            mProfileData.addObserver(mProfileDataCacheObserver);
+        }
         showSigninPage();
     }
 
@@ -193,6 +197,17 @@ public class AccountSigninView extends FrameLayout {
     protected void onAttachedToWindow() {
         super.onAttachedToWindow();
         updateAccounts();
+        if (mProfileData != null) {
+            mProfileData.addObserver(mProfileDataCacheObserver);
+        }
+    }
+
+    @Override
+    protected void onDetachedFromWindow() {
+        if (mProfileData != null) {
+            mProfileData.removeObserver(mProfileDataCacheObserver);
+        }
+        super.onDetachedFromWindow();
     }
 
     @Override
@@ -254,7 +269,7 @@ public class AccountSigninView extends FrameLayout {
             updatingGmsDialog = null;
         }
 
-        mAccountManagerFacade.tryGetGoogleAccountNames(new Callback<List<String>>() {
+        AccountManagerFacade.get().tryGetGoogleAccountNames(new Callback<List<String>>() {
             @Override
             public void onResult(List<String> result) {
                 if (updatingGmsDialog != null) {
@@ -264,6 +279,13 @@ public class AccountSigninView extends FrameLayout {
                 }
                 mIsGooglePlayServicesOutOfDate = false;
 
+                if (!ViewCompat.isAttachedToWindow(AccountSigninView.this)) {
+                    // This callback is invoked after AccountSigninView is detached from window
+                    // (e.g., Chrome is minimized). Updating view now is redundant and dangerous
+                    // (getFragmentManager() can return null, etc.). See https://crbug.com/733117.
+                    return;
+                }
+
                 if (mSignedIn) {
                     // If sign-in completed in the mean time, return in order to avoid showing the
                     // wrong state in the UI.
@@ -298,12 +320,7 @@ public class AccountSigninView extends FrameLayout {
                         && (mAccountNames.isEmpty()
                                    || mAccountNames.get(accountToSelect)
                                               .equals(oldAccountNames.get(oldSelectedAccount)));
-                // There is a race condition where the FragmentManager can be null. This
-                // presumably happens once the AccountSigninView has been detached (the bug is
-                // triggered when Chrome is hidden). Since we are detached, the dialogs will
-                // have already been deleted so we don't need to cancel them.
-                // https://crbug.com/733117
-                if (selectedAccountChanged && mDelegate.getFragmentManager() != null) {
+                if (selectedAccountChanged) {
                     // Any dialogs that may have been showing are now invalid (they were created
                     // for the previously selected account).
                     ConfirmSyncDataStateMachine.cancelAllDialogs(mDelegate.getFragmentManager());