Fetch the user info when the account info in invalid.

This CL removes some Android-only logic that avoided downloading the
account info when the account info was invalid. This ensures that the
account info is correctly updated when the user signs in to Chrome.

This CL also fixes a subtle bug that lead to the avatar image not being
downloaded on start-up if the previous attempt to download the avatar
image failed.

Bug: 1135958
Change-Id: Ia31993f969aac2fc6b7c6f1ed325eb012ad33dd0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2461310
Commit-Queue: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Alice Wang <aliceywang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#815632}

components/signin/internal/identity_manager/account_fetcher_service.cc

@@ -79,7 +79,7 @@ void AccountFetcherService::Initialize(
       signin_client_->GetPrefs(), AccountFetcherService::kLastUpdatePref,
       kRefreshFromTokenServiceDelay,
       base::Bind(&AccountFetcherService::RefreshAllAccountInfo,
-                 base::Unretained(this), false));
+                 base::Unretained(this), /*only_fetch_if_invalid=*/false));
 
   // Tokens may have already been loaded and we will not receive a
   // notification-on-registration for |token_service_->AddObserver(this)| few
@@ -105,7 +105,7 @@ bool AccountFetcherService::IsAllUserInfoFetched() const {
 void AccountFetcherService::ForceRefreshOfAccountInfo(
     const CoreAccountId& account_id) {
   DCHECK(network_fetches_enabled_);
-  RefreshAccountInfo(account_id, false);
+  RefreshAccountInfo(account_id, /*only_fetch_if_invalid=*/false);
 }
 
 void AccountFetcherService::OnNetworkInitialized() {
@@ -167,7 +167,7 @@ void AccountFetcherService::MaybeEnableNetworkFetches() {
     network_fetches_enabled_ = true;
     repeating_timer_->Start();
   }
-  RefreshAllAccountInfo(true);
+  RefreshAllAccountInfo(/*only_fetch_if_invalid=*/true);
 #if defined(OS_ANDROID)
   UpdateChildInfo();
 #endif
@@ -221,15 +221,21 @@ void AccountFetcherService::RefreshAccountInfo(const CoreAccountId& account_id,
   const AccountInfo& info =
       account_tracker_service_->GetAccountInfo(account_id);
 
-// |only_fetch_if_invalid| is false when the service is due for a timed update.
-#if defined(OS_ANDROID)
-  // TODO(mlerman): Change this condition back to info.IsValid() and ensure the
-  // Fetch doesn't occur until after ProfileImpl::OnPrefsLoaded().
-  if (!only_fetch_if_invalid || info.gaia.empty())
-#else
-  if (!only_fetch_if_invalid || !info.IsValid())
-#endif
+  // |only_fetch_if_invalid| is false when the service is due for a timed
+  // update.
+  if (!only_fetch_if_invalid || !info.IsValid()) {
+    // Fetching the user info will also fetch the account image.
     StartFetchingUserInfo(account_id);
+    return;
+  }
+
+  // User info is already valid and does not need to be downloaded again.
+  // Fetch the account image in case it was not fetched previously.
+  //
+  // Note: |FetchAccountImage()| does not fetch the account image if the
+  // account image was already downloaded. So it is fine to call this method
+  // even when |only_fetch_if_invalid| is true.
+  FetchAccountImage(account_id);
 }
 
 void AccountFetcherService::OnUserInfoFetchSuccess(
@@ -323,7 +329,7 @@ void AccountFetcherService::OnRefreshTokenAvailable(
 
   if (!network_fetches_enabled_)
     return;
-  RefreshAccountInfo(account_id, true);
+  RefreshAccountInfo(account_id, /*only_fetch_if_invalid=*/true);
 #if defined(OS_ANDROID)
   UpdateChildInfo();
 #endif

components/signin/internal/identity_manager/account_tracker_service_unittest.cc

@@ -107,11 +107,10 @@ std::string AccountKeyToPictureURL(AccountKey account_key) {
       account_key.value);
 }
 
-std::string AccountKeyToPictureURLWithSize(AccountKey account_key) {
+GURL AccountKeyToPictureURLWithSize(AccountKey account_key) {
   return signin::GetAvatarImageURLWithOptions(
-             GURL(AccountKeyToPictureURL(account_key)),
-             signin::kAccountInfoImageSize, true /* no_silhouette */)
-      .spec();
+      GURL(AccountKeyToPictureURL(account_key)), signin::kAccountInfoImageSize,
+      true /* no_silhouette */);
 }
 
 class TrackingEvent {
@@ -315,7 +314,8 @@ class AccountTrackerServiceTest : public testing::Test {
   }
 
  protected:
-  void ReturnFetchResults(net::HttpStatusCode response_code,
+  void ReturnFetchResults(const GURL& url,
+                          net::HttpStatusCode response_code,
                           const std::string& response_string);
 
   base::test::TaskEnvironment task_environment_;
@@ -371,9 +371,9 @@ class AccountTrackerServiceTest : public testing::Test {
 };
 
 void AccountTrackerServiceTest::ReturnFetchResults(
+    const GURL& url,
     net::HttpStatusCode response_code,
     const std::string& response_string) {
-  GURL url = GaiaUrls::GetInstance()->oauth_user_info_url();
   EXPECT_TRUE(GetTestURLLoaderFactory()->IsPending(url.spec()));
 
   // It's possible for multiple requests to be pending. Respond to all of them.
@@ -388,35 +388,35 @@ void AccountTrackerServiceTest::ReturnFetchResults(
 void AccountTrackerServiceTest::ReturnAccountInfoFetchSuccess(
     AccountKey account_key) {
   IssueAccessToken(account_key);
-  ReturnFetchResults(net::HTTP_OK, GenerateValidTokenInfoResponse(account_key));
+  ReturnFetchResults(GaiaUrls::GetInstance()->oauth_user_info_url(),
+                     net::HTTP_OK, GenerateValidTokenInfoResponse(account_key));
 }
 
 void AccountTrackerServiceTest::ReturnAccountInfoFetchSuccessIncomplete(
     AccountKey account_key) {
   IssueAccessToken(account_key);
-  ReturnFetchResults(net::HTTP_OK,
+  ReturnFetchResults(GaiaUrls::GetInstance()->oauth_user_info_url(),
+                     net::HTTP_OK,
                      GenerateIncompleteTokenInfoResponse(account_key));
 }
 
 void AccountTrackerServiceTest::ReturnAccountInfoFetchFailure(
     AccountKey account_key) {
   IssueAccessToken(account_key);
-  ReturnFetchResults(net::HTTP_BAD_REQUEST, std::string());
+  ReturnFetchResults(GaiaUrls::GetInstance()->oauth_user_info_url(),
+                     net::HTTP_BAD_REQUEST, std::string());
 }
 
 void AccountTrackerServiceTest::ReturnAccountImageFetchSuccess(
     AccountKey account_key) {
-  GetTestURLLoaderFactory()->AddResponse(
-      AccountKeyToPictureURLWithSize(account_key), "image data");
-  task_environment_.RunUntilIdle();
+  ReturnFetchResults(AccountKeyToPictureURLWithSize(account_key), net::HTTP_OK,
+                     "image data");
 }
 
 void AccountTrackerServiceTest::ReturnAccountImageFetchFailure(
     AccountKey account_key) {
-  GetTestURLLoaderFactory()->AddResponse(
-      AccountKeyToPictureURLWithSize(account_key), std::string(),
-      net::HTTP_BAD_REQUEST);
-  task_environment_.RunUntilIdle();
+  ReturnFetchResults(AccountKeyToPictureURLWithSize(account_key),
+                     net::HTTP_BAD_REQUEST, "image data");
 }
 
 TEST_F(AccountTrackerServiceTest, Basic) {}
@@ -563,6 +563,44 @@ TEST_F(AccountTrackerServiceTest, TwoTokenAvailable_OneUserInfo) {
   }));
 }
 
+// Regression test for http://crbug.com/1135958
+TEST_F(AccountTrackerServiceTest,
+       AccountSeeded_TokenAvailable_UserInfoSuccess) {
+  // Setup: Seed the account before simulating that the refresh token is
+  // available.
+  account_tracker()->SeedAccountInfo(AccountKeyToGaiaId(kAccountKeyAlpha),
+                                     AccountKeyToEmail(kAccountKeyAlpha));
+
+  // Account fetcher service should fetch user info for accounts that were
+  // seeded.
+  SimulateTokenAvailable(kAccountKeyAlpha);
+  ReturnAccountInfoFetchSuccess(kAccountKeyAlpha);
+  EXPECT_TRUE(account_fetcher()->IsAllUserInfoFetched());
+}
+
+// Regression test for http://crbug.com/1135958
+TEST_F(AccountTrackerServiceTest, RefreshAccount_FetchImageSuccess) {
+  CoreAccountId account_id = AccountKeyToAccountId(kAccountKeyAlpha);
+
+  // Setup: User info fetched successfully, but missing the account image.
+  SimulateTokenAvailable(kAccountKeyAlpha);
+  ReturnAccountInfoFetchSuccess(kAccountKeyAlpha);
+  ReturnAccountImageFetchFailure(kAccountKeyAlpha);
+  ASSERT_TRUE(account_tracker()->GetAccountInfo(account_id).IsValid());
+  ASSERT_TRUE(
+      account_tracker()->GetAccountInfo(account_id).account_image.IsEmpty());
+
+  // Account fetcher should fetch the account image even when user info if
+  // the account image was not fetched before.
+  SimulateTokenAvailable(kAccountKeyAlpha);
+  ReturnAccountImageFetchSuccess(kAccountKeyAlpha);
+  AccountInfo account_info = account_tracker()->GetAccountInfo(
+      AccountKeyToAccountId(kAccountKeyAlpha));
+  EXPECT_FALSE(account_info.account_image.IsEmpty());
+  EXPECT_EQ(account_info.last_downloaded_image_url_with_size,
+            AccountKeyToPictureURLWithSize(kAccountKeyAlpha));
+}
+
 TEST_F(AccountTrackerServiceTest, GetAccounts) {
   SimulateTokenAvailable(kAccountKeyAlpha);
   SimulateTokenAvailable(kAccountKeyBeta);
@@ -1139,7 +1177,8 @@ TEST_F(AccountTrackerServiceTest, ChildAccountUpdatedAndRevoked) {
   account_tracker()->SetIsChildAccount(AccountKeyToAccountId(kAccountKeyChild),
                                        false);
 #endif
-  ReturnFetchResults(net::HTTP_OK,
+  ReturnFetchResults(GaiaUrls::GetInstance()->oauth_user_info_url(),
+                     net::HTTP_OK,
                      GenerateValidTokenInfoResponse(kAccountKeyChild));
   EXPECT_TRUE(CheckAccountTrackerEvents({
       TrackingEvent(UPDATED, AccountKeyToAccountId(kAccountKeyChild),
@@ -1167,7 +1206,8 @@ TEST_F(AccountTrackerServiceTest, ChildAccountUpdatedAndRevokedWithUpdate) {
   account_tracker()->SetIsChildAccount(AccountKeyToAccountId(kAccountKeyChild),
                                        true);
 #endif
-  ReturnFetchResults(net::HTTP_OK,
+  ReturnFetchResults(GaiaUrls::GetInstance()->oauth_user_info_url(),
+                     net::HTTP_OK,
                      GenerateValidTokenInfoResponse(kAccountKeyChild));
   EXPECT_TRUE(CheckAccountTrackerEvents({
       TrackingEvent(UPDATED, AccountKeyToAccountId(kAccountKeyChild),
@@ -1253,7 +1293,8 @@ TEST_F(AccountTrackerServiceTest, ChildAccountGraduation) {
   AccountInfo info = account_tracker()->GetAccountInfo(
       AccountKeyToAccountId(kAccountKeyChild));
   EXPECT_TRUE(info.is_child_account);
-  ReturnFetchResults(net::HTTP_OK,
+  ReturnFetchResults(GaiaUrls::GetInstance()->oauth_user_info_url(),
+                     net::HTTP_OK,
                      GenerateValidTokenInfoResponse(kAccountKeyChild));
   EXPECT_TRUE(CheckAccountTrackerEvents({
       TrackingEvent(UPDATED, AccountKeyToAccountId(kAccountKeyChild),