[signin] Re-add missing call to ProfileMetrics::LogNumberOfProfiles

The logic to execute a task on a timer, persisted across Chrome
restarts, already existed in account_fetcher_service.
This CL extracts it in a new class, and reuses it in ProfileInfoCache.

The metric is not loaded exactly at the same time it was before:
it used to be logged for each profile, but now it's logged per Chrome
run. This makes more sense given the nature of the metric.

Fixed: 1060096
Change-Id: I5b8c709f088709934b8ad54fa9a46fac091923cf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2096743
Commit-Queue: David Roger <droger@chromium.org>
Reviewed-by: Monica Basta <msalama@chromium.org>
Cr-Commit-Position: refs/heads/master@{#749621}

chrome/browser/profiles/profile_attributes_storage.h

@@ -127,6 +127,10 @@ class ProfileAttributesStorage
   // ProfileAttributesEntry.
   void NotifyOnProfileAvatarChanged(const base::FilePath& profile_path) const;
 
+  // Disables the periodic reporting of profile metrics, as this is causing
+  // tests to time out.
+  virtual void DisableProfileMetricsForTesting() {}
+
  protected:
   FRIEND_TEST_ALL_PREFIXES(ProfileInfoCacheTest, EntriesInAttributesStorage);
   FRIEND_TEST_ALL_PREFIXES(ProfileAttributesStorageTest,

chrome/browser/profiles/profile_info_cache.cc

@@ -46,6 +46,7 @@ const char kGAIAPictureFileNameKey[] = "gaia_picture_file_name";
 const char kLastDownloadedGAIAPictureUrlWithSizeKey[] =
     "last_downloaded_gaia_picture_url_with_size";
 const char kAccountIdKey[] = "account_id_key";
+const char kProfileCountLastUpdatePref[] = "profile.profile_counts_reported";
 #if !defined(OS_ANDROID) && !defined(OS_CHROMEOS)
 const char kLegacyProfileNameMigrated[] = "legacy.profile.name.migrated";
 bool migration_enabled_for_testing = false;
@@ -121,12 +122,16 @@ ProfileInfoCache::ProfileInfoCache(PrefService* prefs,
     MigrateLegacyProfileNamesAndRecomputeIfNeeded();
     prefs_->SetBoolean(kLegacyProfileNameMigrated, true);
   }
-#endif  //! defined(OS_ANDROID) && !defined(OS_CHROMEOS)
-}
 
-ProfileInfoCache::~ProfileInfoCache() {
+  repeating_timer_ = std::make_unique<signin::PersistentRepeatingTimer>(
+      prefs_, kProfileCountLastUpdatePref, base::TimeDelta::FromHours(24),
+      base::Bind(&ProfileMetrics::LogNumberOfProfiles, this));
+  repeating_timer_->Start();
+#endif  // !defined(OS_ANDROID) && !defined(OS_CHROMEOS)
 }
 
+ProfileInfoCache::~ProfileInfoCache() = default;
+
 void ProfileInfoCache::AddProfileToCache(const base::FilePath& profile_path,
                                          const base::string16& name,
                                          const std::string& gaia_id,
@@ -187,6 +192,12 @@ void ProfileInfoCache::AddProfileToCache(const base::FilePath& profile_path,
     observer.OnProfileAdded(profile_path);
 }
 
+void ProfileInfoCache::DisableProfileMetricsForTesting() {
+#if !defined(OS_ANDROID) && !defined(OS_CHROMEOS)
+  repeating_timer_.reset();
+#endif
+}
+
 void ProfileInfoCache::NotifyIfProfileNamesHaveChanged() {
   std::vector<ProfileAttributesEntry*> entries = GetAllProfilesAttributes();
   for (ProfileAttributesEntry* entry : entries) {
@@ -473,6 +484,7 @@ const base::FilePath& ProfileInfoCache::GetUserDataDir() const {
 // static
 void ProfileInfoCache::RegisterPrefs(PrefRegistrySimple* registry) {
   registry->RegisterDictionaryPref(prefs::kProfileInfoCache);
+  registry->RegisterTimePref(kProfileCountLastUpdatePref, base::Time());
 #if !defined(OS_ANDROID) && !defined(OS_CHROMEOS)
   registry->RegisterBooleanPref(kLegacyProfileNameMigrated, false);
 #endif  // !defined(OS_ANDROID) && !defined(OS_CHROMEOS)

chrome/browser/profiles/profile_info_cache.h

@@ -25,6 +25,7 @@
 #include "chrome/browser/profiles/profile_attributes_storage.h"
 #include "chrome/browser/profiles/profile_info_cache_observer.h"
 #include "chrome/browser/profiles/profile_info_interface.h"
+#include "components/signin/public/base/persistent_repeating_timer.h"
 
 namespace gfx {
 class Image;
@@ -120,6 +121,7 @@ class ProfileInfoCache : public ProfileInfoInterface,
 
   bool GetProfileAttributesWithPath(const base::FilePath& path,
                                     ProfileAttributesEntry** entry) override;
+  void DisableProfileMetricsForTesting() override;
 
   void NotifyProfileAuthInfoChanged(const base::FilePath& profile_path);
   void NotifyIfProfileNamesHaveChanged();
@@ -177,6 +179,8 @@ class ProfileInfoCache : public ProfileInfoInterface,
   // recomputed to "Person 1" and "Person 2".
   void MigrateLegacyProfileNamesAndRecomputeIfNeeded();
   static void SetLegacyProfileMigrationForTesting(bool value);
+
+  std::unique_ptr<signin::PersistentRepeatingTimer> repeating_timer_;
 #endif  // !defined(OS_ANDROID) && !defined(OS_CHROMEOS)
 
   std::vector<std::string> keys_;

chrome/browser/profiles/profile_metrics.cc

@@ -156,22 +156,21 @@ enum ProfileAvatar {
   NUM_PROFILE_AVATAR_METRICS
 };
 
-bool ProfileMetrics::CountProfileInformation(ProfileManager* manager,
+void ProfileMetrics::CountProfileInformation(ProfileAttributesStorage* storage,
                                              profile_metrics::Counts* counts) {
-  ProfileAttributesStorage& storage = manager->GetProfileAttributesStorage();
-  size_t number_of_profiles = storage.GetNumberOfProfiles();
+  size_t number_of_profiles = storage->GetNumberOfProfiles();
   counts->total = number_of_profiles;
 
   // Ignore other metrics if we have no profiles.
   if (!number_of_profiles)
-    return false;
+    return;
 
   // Maximum age for "active" profile is 4 weeks.
   base::Time oldest = base::Time::Now() -
       base::TimeDelta::FromDays(kMaximumDaysOfDisuse);
 
   std::vector<ProfileAttributesEntry*> entries =
-      storage.GetAllProfilesAttributes();
+      storage->GetAllProfilesAttributes();
   for (ProfileAttributesEntry* entry : entries) {
     if (!HasProfileBeenActiveSince(entry, oldest)) {
       counts->unused++;
@@ -190,7 +189,6 @@ bool ProfileMetrics::CountProfileInformation(ProfileManager* manager,
       }
     }
   }
-  return true;
 }
 
 profile_metrics::BrowserProfileType ProfileMetrics::GetBrowserProfileType(
@@ -221,9 +219,9 @@ void ProfileMetrics::LogNumberOfProfileSwitches() {
 }
 #endif
 
-void ProfileMetrics::LogNumberOfProfiles(ProfileManager* manager) {
+void ProfileMetrics::LogNumberOfProfiles(ProfileAttributesStorage* storage) {
   profile_metrics::Counts counts;
-  CountProfileInformation(manager, &counts);
+  CountProfileInformation(storage, &counts);
   profile_metrics::LogProfileMetricsCounts(counts);
 }
 

chrome/browser/profiles/profile_metrics.h

@@ -12,6 +12,7 @@
 #include "build/build_config.h"
 
 class Profile;
+class ProfileAttributesStorage;
 class ProfileManager;
 
 namespace base {
@@ -151,8 +152,8 @@ class ProfileMetrics {
 #endif  // defined(OS_ANDROID)
 
   // Count and return summary information about the profiles currently in the
-  // |manager|. This information is returned in the output variable |counts|.
-  static bool CountProfileInformation(ProfileManager* manager,
+  // |storage|. This information is returned in the output variable |counts|.
+  static void CountProfileInformation(ProfileAttributesStorage* storage,
                                       profile_metrics::Counts* counts);
 
 #if !defined(OS_ANDROID)
@@ -163,7 +164,7 @@ class ProfileMetrics {
   static profile_metrics::BrowserProfileType GetBrowserProfileType(
       Profile* profile);
 
-  static void LogNumberOfProfiles(ProfileManager* manager);
+  static void LogNumberOfProfiles(ProfileAttributesStorage* storage);
   static void LogProfileAddNewUser(ProfileAdd metric);
   static void LogProfileAvatarSelection(size_t icon_index);
   static void LogProfileDeleteUser(ProfileDelete metric);

chrome/browser/safe_browsing/incident_reporting/incident_reporting_service_unittest.cc

@@ -24,6 +24,7 @@
 #include "base/values.h"
 #include "build/build_config.h"
 #include "chrome/browser/prefs/browser_prefs.h"
+#include "chrome/browser/profiles/profile_attributes_storage.h"
 #include "chrome/browser/safe_browsing/incident_reporting/incident.h"
 #include "chrome/browser/safe_browsing/incident_reporting/incident_receiver.h"
 #include "chrome/browser/safe_browsing/incident_reporting/incident_report_uploader.h"
@@ -212,6 +213,10 @@ class IncidentReportingServiceTest : public testing::Test {
         registry_override_manager_.OverrideRegistry(HKEY_CURRENT_USER));
 #endif
     ASSERT_TRUE(profile_manager_.SetUp());
+    // Disable profile metrics reporting, otherwise the calls to
+    // FastForwardUntilNoTasksRemain() never return.
+    profile_manager_.profile_attributes_storage()
+        ->DisableProfileMetricsForTesting();
   }
 
   void CreateIncidentReportingService() {

chrome/browser/signin/signin_global_error_unittest.cc

@@ -150,8 +150,9 @@ TEST_F(SigninGlobalErrorTest, AuthStatusEnumerateAllErrors) {
     EXPECT_FALSE(global_error()->GetBubbleViewAcceptButtonLabel().empty());
     EXPECT_TRUE(global_error()->GetBubbleViewCancelButtonLabel().empty());
 
-    ProfileMetrics::LogNumberOfProfiles(
-        testing_profile_manager()->profile_manager());
+    ProfileMetrics::LogNumberOfProfiles(&testing_profile_manager()
+                                             ->profile_manager()
+                                             ->GetProfileAttributesStorage());
 
     if (entry.is_error) {
       histogram_tester.ExpectBucketCount("Signin.AuthError", entry.error_state,

components/signin/internal/identity_manager/account_fetcher_service.cc

@@ -75,8 +75,11 @@ void AccountFetcherService::Initialize(
   DCHECK(image_decoder);
   DCHECK(!image_decoder_);
   image_decoder_ = std::move(image_decoder);
-  last_updated_ = signin_client_->GetPrefs()->GetTime(
-      AccountFetcherService::kLastUpdatePref);
+  repeating_timer_ = std::make_unique<signin::PersistentRepeatingTimer>(
+      signin_client_->GetPrefs(), AccountFetcherService::kLastUpdatePref,
+      kRefreshFromTokenServiceDelay,
+      base::Bind(&AccountFetcherService::RefreshAllAccountInfo,
+                 base::Unretained(this), false));
 
   // Tokens may have already been loaded and we will not receive a
   // notification-on-registration for |token_service_->AddObserver(this)| few
@@ -162,7 +165,7 @@ void AccountFetcherService::MaybeEnableNetworkFetches() {
     return;
   if (!network_fetches_enabled_) {
     network_fetches_enabled_ = true;
-    ScheduleNextRefresh();
+    repeating_timer_->Start();
   }
   RefreshAllAccountInfo(true);
 #if defined(OS_ANDROID)
@@ -170,29 +173,6 @@ void AccountFetcherService::MaybeEnableNetworkFetches() {
 #endif
 }
 
-void AccountFetcherService::RefreshAllAccountsAndScheduleNext() {
-  DCHECK(network_fetches_enabled_);
-  RefreshAllAccountInfo(false);
-  last_updated_ = base::Time::Now();
-  signin_client_->GetPrefs()->SetTime(AccountFetcherService::kLastUpdatePref,
-                                      last_updated_);
-  ScheduleNextRefresh();
-}
-
-void AccountFetcherService::ScheduleNextRefresh() {
-  DCHECK(!timer_.IsRunning());
-  DCHECK(network_fetches_enabled_);
-
-  const base::TimeDelta time_since_update = base::Time::Now() - last_updated_;
-  if (time_since_update > kRefreshFromTokenServiceDelay) {
-    RefreshAllAccountsAndScheduleNext();
-  } else {
-    timer_.Start(FROM_HERE, kRefreshFromTokenServiceDelay - time_since_update,
-                 this,
-                 &AccountFetcherService::RefreshAllAccountsAndScheduleNext);
-  }
-}
-
 // Starts fetching user information. This is called periodically to refresh.
 void AccountFetcherService::StartFetchingUserInfo(
     const CoreAccountId& account_id) {

components/signin/internal/identity_manager/account_fetcher_service.h

@@ -16,6 +16,7 @@
 #include "base/timer/timer.h"
 #include "build/build_config.h"
 #include "components/signin/internal/identity_manager/profile_oauth2_token_service_observer.h"
+#include "components/signin/public/base/persistent_repeating_timer.h"
 
 class AccountInfoFetcher;
 class AccountTrackerService;
@@ -100,8 +101,6 @@ class AccountFetcherService : public ProfileOAuth2TokenServiceObserver {
   friend class AccountInfoFetcher;
 
   void RefreshAllAccountInfo(bool only_fetch_if_invalid);
-  void RefreshAllAccountsAndScheduleNext();
-  void ScheduleNextRefresh();
 
 #if defined(OS_ANDROID)
   // Called on all account state changes. Decides whether to fetch new child
@@ -150,8 +149,7 @@ class AccountFetcherService : public ProfileOAuth2TokenServiceObserver {
   bool refresh_tokens_loaded_ = false;
   bool shutdown_called_ = false;
   bool enable_account_removal_for_test_ = false;
-  base::Time last_updated_;
-  base::OneShotTimer timer_;
+  std::unique_ptr<signin::PersistentRepeatingTimer> repeating_timer_;
 
 #if defined(OS_ANDROID)
   CoreAccountId child_request_account_id_;

components/signin/public/base/BUILD.gn

@@ -27,6 +27,8 @@ static_library("base") {
     "device_id_helper.h",
     "multilogin_parameters.cc",
     "multilogin_parameters.h",
+    "persistent_repeating_timer.cc",
+    "persistent_repeating_timer.h",
     "signin_client.cc",
     "signin_client.h",
     "signin_metrics.cc",
@@ -87,6 +89,7 @@ source_set("unit_tests") {
   sources = [
     "avatar_icon_util_unittest.cc",
     "device_id_helper_unittest.cc",
+    "persistent_repeating_timer_unittest.cc",
     "signin_metrics_unittest.cc",
   ]
 
@@ -94,6 +97,7 @@ source_set("unit_tests") {
     ":base",
     "//base",
     "//base/test:test_support",
+    "//components/prefs:test_support",
     "//components/sync_preferences:test_support",
     "//testing/gtest",
     "//url",

components/signin/public/base/persistent_repeating_timer.cc

+// 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.
+
+#include "components/signin/public/base/persistent_repeating_timer.h"
+
+#include "base/callback.h"
+#include "components/prefs/pref_service.h"
+
+namespace signin {
+
+PersistentRepeatingTimer::PersistentRepeatingTimer(
+    PrefService* prefs,
+    const char* timer_last_update_pref_name,
+    base::TimeDelta delay,
+    base::RepeatingClosure task)
+    : prefs_(prefs),
+      last_fired_pref_name_(timer_last_update_pref_name),
+      delay_(delay),
+      user_task_(task) {}
+
+PersistentRepeatingTimer::~PersistentRepeatingTimer() = default;
+
+void PersistentRepeatingTimer::Start() {
+  if (timer_.IsRunning())
+    return;  // Already started.
+
+  const base::TimeDelta time_since_update = base::Time::Now() - GetLastFired();
+  if (time_since_update >= delay_) {
+    OnTimerFired();
+  } else {
+    timer_.Start(FROM_HERE, delay_ - time_since_update,
+                 base::Bind(&PersistentRepeatingTimer::OnTimerFired,
+                            base::Unretained(this)));
+  }
+  DCHECK(timer_.IsRunning());
+}
+
+base::Time PersistentRepeatingTimer::GetLastFired() {
+  return prefs_->GetTime(last_fired_pref_name_);
+}
+
+void PersistentRepeatingTimer::SetLastFiredNow() {
+  prefs_->SetTime(last_fired_pref_name_, base::Time::Now());
+}
+
+void PersistentRepeatingTimer::OnTimerFired() {
+  DCHECK(!timer_.IsRunning());
+  SetLastFiredNow();
+  user_task_.Run();
+  Start();
+}
+
+}  // namespace signin

components/signin/public/base/persistent_repeating_timer.h

+// 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.
+
+#ifndef COMPONENTS_SIGNIN_PUBLIC_BASE_PERSISTENT_REPEATING_TIMER_H_
+#define COMPONENTS_SIGNIN_PUBLIC_BASE_PERSISTENT_REPEATING_TIMER_H_
+
+#include <string>
+
+#include "base/callback_forward.h"
+#include "base/time/time.h"
+#include "base/timer/timer.h"
+
+class PrefService;
+
+namespace signin {
+
+// This class fires a task repeatedly, across application restarts. The timer
+// stores the date of the last invocation in a preference, which is persisted
+// to disk.
+class PersistentRepeatingTimer {
+ public:
+  // The timer is not started at creation.
+  PersistentRepeatingTimer(PrefService* prefs,
+                           const char* timer_last_update_pref_name,
+                           base::TimeDelta delay,
+                           base::RepeatingClosure task);
+
+  ~PersistentRepeatingTimer();
+
+  // Starts the timer. Calling Start() when the timer is running has no effect.
+  void Start();
+
+ private:
+  // Reads the date of the last event from the pref.
+  base::Time GetLastFired();
+
+  // Updates the pref with the current time.
+  void SetLastFiredNow();
+
+  // Called when |timer_| fires.
+  void OnTimerFired();
+
+  PrefService* prefs_;
+  std::string last_fired_pref_name_;
+  base::TimeDelta delay_;
+  base::RepeatingClosure user_task_;
+
+  base::RetainingOneShotTimer timer_;
+};
+
+}  // namespace signin
+
+#endif  // COMPONENTS_SIGNIN_PUBLIC_BASE_PERSISTENT_REPEATING_TIMER_H_

components/signin/public/base/persistent_repeating_timer_unittest.cc

+// 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.
+
+#include "components/signin/public/base/persistent_repeating_timer.h"
+
+#include "base/test/task_environment.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/testing_pref_service.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace signin {
+
+namespace {
+
+const char kLastUpdatedTimePref[] = "test.last_updated_time";
+constexpr base::TimeDelta kTestDelay = base::TimeDelta::FromHours(2);
+
+}  // namespace
+
+class PersistentRepeatingTimerTest : public ::testing::Test {
+ public:
+  PersistentRepeatingTimerTest() {
+    pref_service_.registry()->RegisterTimePref(kLastUpdatedTimePref,
+                                               base::Time());
+  }
+
+  void RunTask() { ++call_count_; }
+
+  void CheckCallCount(int call_count) {
+    base::RunLoop().RunUntilIdle();
+    EXPECT_EQ(call_count, call_count_);
+  }
+
+  base::test::TaskEnvironment task_environment_{
+      base::test::TaskEnvironment::TimeSource::MOCK_TIME};
+  TestingPrefServiceSimple pref_service_;
+  int call_count_ = 0;
+};
+
+// Checks that the missing pref is treated like an old one.
+TEST_F(PersistentRepeatingTimerTest, MissingPref) {
+  PersistentRepeatingTimer timer(
+      &pref_service_, kLastUpdatedTimePref, kTestDelay,
+      base::Bind(&PersistentRepeatingTimerTest::RunTask,
+                 base::Unretained(this)));
+  CheckCallCount(0);
+
+  // The task is run immediately on start.
+  timer.Start();
+  CheckCallCount(1);
+
+  task_environment_.FastForwardBy(base::TimeDelta::FromMinutes(1));
+  CheckCallCount(1);
+
+  // And after the delay.
+  task_environment_.FastForwardBy(kTestDelay);
+  CheckCallCount(2);
+}
+
+// Checks that spurious calls to Start() have no effect.
+TEST_F(PersistentRepeatingTimerTest, MultipleStarts) {
+  PersistentRepeatingTimer timer(
+      &pref_service_, kLastUpdatedTimePref, kTestDelay,
+      base::Bind(&PersistentRepeatingTimerTest::RunTask,
+                 base::Unretained(this)));
+  CheckCallCount(0);
+
+  // The task is run immediately on start.
+  timer.Start();
+  CheckCallCount(1);
+  timer.Start();
+  CheckCallCount(1);
+
+  task_environment_.FastForwardBy(base::TimeDelta::FromMinutes(1));
+  CheckCallCount(1);
+  task_environment_.FastForwardBy(base::TimeDelta::FromMinutes(1));
+  timer.Start();
+  CheckCallCount(1);
+
+  // And after the delay.
+  task_environment_.FastForwardBy(kTestDelay);
+  CheckCallCount(2);
+  timer.Start();
+  CheckCallCount(2);
+}
+
+TEST_F(PersistentRepeatingTimerTest, RecentPref) {
+  pref_service_.SetTime(kLastUpdatedTimePref,
+                        base::Time::Now() - base::TimeDelta::FromHours(1));
+
+  PersistentRepeatingTimer timer(
+      &pref_service_, kLastUpdatedTimePref, kTestDelay,
+      base::Bind(&PersistentRepeatingTimerTest::RunTask,
+                 base::Unretained(this)));
+  CheckCallCount(0);
+
+  // The task is NOT run immediately on start.
+  timer.Start();
+  CheckCallCount(0);
+
+  task_environment_.FastForwardBy(base::TimeDelta::FromMinutes(1));
+  CheckCallCount(0);
+
+  // It is run after te delay.
+  task_environment_.FastForwardBy(base::TimeDelta::FromHours(1));
+  CheckCallCount(1);
+  task_environment_.FastForwardBy(base::TimeDelta::FromHours(1));
+  CheckCallCount(1);
+
+  task_environment_.FastForwardBy(base::TimeDelta::FromHours(1));
+  CheckCallCount(2);
+}
+
+TEST_F(PersistentRepeatingTimerTest, OldPref) {
+  pref_service_.SetTime(kLastUpdatedTimePref,
+                        base::Time::Now() - base::TimeDelta::FromHours(10));
+
+  PersistentRepeatingTimer timer(
+      &pref_service_, kLastUpdatedTimePref, kTestDelay,
+      base::Bind(&PersistentRepeatingTimerTest::RunTask,
+                 base::Unretained(this)));
+  CheckCallCount(0);
+
+  // The task is run immediately on start.
+  timer.Start();
+  CheckCallCount(1);
+
+  task_environment_.FastForwardBy(base::TimeDelta::FromMinutes(1));
+  CheckCallCount(1);
+
+  // And after the delay.
+  task_environment_.FastForwardBy(kTestDelay);
+  CheckCallCount(2);
+}
+
+}  // namespace signin