[Metrics] Fix guest crash and restructure FamilyUserMetricsProvider

FamilyUserMetricsProvider currently causes a crash in guest mode. This
CL fixes the crash and categorizes guest users into the other bucket.

This CL also restructures the FamilyUserMetricsProvider to an
observer-based approach that improves stability and avoids unnecessary
work by caching results.

Bug: 1137352,1103077
Change-Id: Ie421861970f6b26def1cf7390bd89480081bcab5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2476897Reviewed-by: Aga Wronska <agawronska@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Toby Huang <tobyhuang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#819118}

chrome/browser/chromeos/child_accounts/family_features.cc

@@ -7,6 +7,6 @@
 namespace chromeos {
 
 const base::Feature kFamilyUserMetricsProvider{
-    "FamilyUserMetricsProvider", base::FEATURE_DISABLED_BY_DEFAULT};
+    "FamilyUserMetricsProvider", base::FEATURE_ENABLED_BY_DEFAULT};
 
 }  // namespace chromeos

chrome/browser/metrics/chrome_metrics_service_client.cc

@@ -144,6 +144,8 @@
 #endif
 
 #if defined(OS_CHROMEOS)
+#include "base/feature_list.h"
+#include "chrome/browser/chromeos/child_accounts/family_features.h"
 #include "chrome/browser/chromeos/printing/printer_metrics_provider.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/metrics/ambient_mode_metrics_provider.h"
@@ -743,8 +745,10 @@ void ChromeMetricsServiceClient::RegisterMetricsServiceProviders() {
   metrics_service_->RegisterMetricsProvider(
       std::make_unique<AmbientModeMetricsProvider>());
 
-  metrics_service_->RegisterMetricsProvider(
-      std::make_unique<FamilyUserMetricsProvider>());
+  if (base::FeatureList::IsEnabled(chromeos::kFamilyUserMetricsProvider)) {
+    metrics_service_->RegisterMetricsProvider(
+        std::make_unique<FamilyUserMetricsProvider>());
+  }
 #endif  // defined(OS_CHROMEOS)
 
 #if !defined(OS_CHROMEOS)

chrome/browser/metrics/family_user_metrics_provider.cc

@@ -4,12 +4,10 @@
 
 #include "chrome/browser/metrics/family_user_metrics_provider.h"
 
-#include "base/feature_list.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_functions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browser_process_platform_part.h"
-#include "chrome/browser/chromeos/child_accounts/family_features.h"
 #include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
 #include "chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
@@ -17,12 +15,14 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/signin/identity_manager_factory.h"
 #include "components/policy/proto/device_management_backend.pb.h"
-#include "components/signin/public/identity_manager/identity_manager.h"
+#include "components/session_manager/core/session_manager.h"
 #include "components/user_manager/user.h"
 #include "components/user_manager/user_manager.h"
 
 namespace {
 
+constexpr char kHistogramName[] = "ChromeOS.FamilyUser.LogSegment";
+
 // Returns user's segment for metrics logging.
 enterprise_management::PolicyData::MetricsLogSegment GetMetricsLogSegment(
     Profile* profile) {
@@ -37,11 +37,6 @@ enterprise_management::PolicyData::MetricsLogSegment GetMetricsLogSegment(
   return policy->metrics_log_segment();
 }
 
-bool IsLoggedIn() {
-  return user_manager::UserManager::IsInitialized() &&
-         user_manager::UserManager::Get()->IsUserLoggedIn();
-}
-
 bool IsEnterpriseManaged() {
   policy::BrowserPolicyConnectorChromeOS* connector =
       g_browser_process->platform_part()->browser_policy_connector_chromeos();
@@ -50,47 +45,97 @@ bool IsEnterpriseManaged() {
 
 }  // namespace
 
-// static
-const char FamilyUserMetricsProvider::kFamilyUserLogSegmentHistogramName[] =
-    "ChromeOS.FamilyUser.LogSegment";
-
-FamilyUserMetricsProvider::FamilyUserMetricsProvider() = default;
+FamilyUserMetricsProvider::FamilyUserMetricsProvider()
+    : identity_manager_observer_(this) {
+  session_manager::SessionManager* session_manager =
+      session_manager::SessionManager::Get();
+  // The |session_manager| is nullptr only for unit tests.
+  if (session_manager)
+    session_manager->AddObserver(this);
+}
 
-FamilyUserMetricsProvider::~FamilyUserMetricsProvider() = default;
+FamilyUserMetricsProvider::~FamilyUserMetricsProvider() {
+  session_manager::SessionManager* session_manager =
+      session_manager::SessionManager::Get();
+  // The |session_manager| is nullptr only for unit tests.
+  if (session_manager)
+    session_manager->RemoveObserver(this);
+}
 
+// This function is called at unpredictable intervals throughout the entire
+// ChromeOS session, so guarantee it will never crash.
 void FamilyUserMetricsProvider::ProvideCurrentSessionData(
     metrics::ChromeUserMetricsExtension* uma_proto_unused) {
-  if (!base::FeatureList::IsEnabled(chromeos::kFamilyUserMetricsProvider))
+  if (!log_segment_)
     return;
-  if (!IsLoggedIn())
+  base::UmaHistogramEnumeration(kHistogramName, log_segment_.value());
+}
+
+void FamilyUserMetricsProvider::OnUserSessionStarted(bool is_primary_user) {
+  if (!is_primary_user)
     return;
   const user_manager::User* primary_user =
       user_manager::UserManager::Get()->GetPrimaryUser();
-  if (!primary_user || !primary_user->is_profile_created())
-    return;
+  DCHECK(primary_user);
+  DCHECK(primary_user->is_profile_created());
   Profile* profile =
       chromeos::ProfileHelper::Get()->GetProfileByUser(primary_user);
   DCHECK(profile);
   DCHECK(chromeos::ProfileHelper::IsRegularProfile(profile));
+
+  // Check for incognito profiles.
+  if (!profile->IsRegularProfile()) {
+    log_segment_ = LogSegment::kOther;
+    return;
+  }
+
   signin::IdentityManager* identity_manager =
       IdentityManagerFactory::GetForProfile(profile);
+  DCHECK(identity_manager);
+  if (!identity_manager_observer_.IsObserving(identity_manager))
+    identity_manager_observer_.Add(identity_manager);
   auto accounts_size = identity_manager->GetAccountsWithRefreshTokens().size();
-  DCHECK_GT(accounts_size, 0);
 
-  LogSegment log_segment = LogSegment::kOther;
   if (profile->IsChild() && accounts_size == 1) {
-    log_segment = LogSegment::kSupervisedUser;
+    log_segment_ = LogSegment::kSupervisedUser;
   } else if (profile->IsChild() && accounts_size > 1) {
     // If a supervised user has a secondary account, then the secondary
     // account must be EDU.
-    log_segment = LogSegment::kSupervisedStudent;
+    log_segment_ = LogSegment::kSupervisedStudent;
   } else if (!IsEnterpriseManaged() &&
              GetMetricsLogSegment(profile) ==
                  enterprise_management::PolicyData::K12) {
     DCHECK(profile->GetProfilePolicyConnector()->IsManaged());
     // This is a K-12 EDU user on an unmanaged ChromeOS device.
-    log_segment = LogSegment::kStudentAtHome;
+    log_segment_ = LogSegment::kStudentAtHome;
+  } else {
+    log_segment_ = LogSegment::kOther;
   }
-  base::UmaHistogramEnumeration(kFamilyUserLogSegmentHistogramName,
-                                log_segment);
+}
+
+// Called when the user adds a secondary account. We're only interested in
+// detecting when a supervised user adds an EDU secondary account.
+void FamilyUserMetricsProvider::OnRefreshTokenUpdatedForAccount(
+    const CoreAccountInfo& account_info) {
+  const user_manager::User* primary_user =
+      user_manager::UserManager::Get()->GetPrimaryUser();
+  Profile* profile =
+      chromeos::ProfileHelper::Get()->GetProfileByUser(primary_user);
+  // Check for incognito profiles.
+  if (!profile->IsRegularProfile())
+    return;
+
+  signin::IdentityManager* identity_manager =
+      IdentityManagerFactory::GetForProfile(profile);
+  auto accounts_size = identity_manager->GetAccountsWithRefreshTokens().size();
+
+  // If a supervised user has a secondary account, then the secondary account
+  // must be EDU.
+  if (profile->IsChild() && accounts_size > 1)
+    log_segment_ = LogSegment::kSupervisedStudent;
+}
+
+// static
+const char* FamilyUserMetricsProvider::GetHistogramNameForTesting() {
+  return kHistogramName;
 }

chrome/browser/metrics/family_user_metrics_provider.h

@@ -5,7 +5,11 @@
 #ifndef CHROME_BROWSER_METRICS_FAMILY_USER_METRICS_PROVIDER_H_
 #define CHROME_BROWSER_METRICS_FAMILY_USER_METRICS_PROVIDER_H_
 
+#include "base/optional.h"
+#include "base/scoped_observer.h"
 #include "components/metrics/metrics_provider.h"
+#include "components/session_manager/core/session_manager_observer.h"
+#include "components/signin/public/identity_manager/identity_manager.h"
 
 namespace metrics {
 class ChromeUserMetricsExtension;
@@ -13,7 +17,10 @@ class ChromeUserMetricsExtension;
 
 // Categorizes the current user into a family user type for UMA dashboard
 // filtering. This metrics provider is ChromeOS specific.
-class FamilyUserMetricsProvider : public metrics::MetricsProvider {
+class FamilyUserMetricsProvider
+    : public metrics::MetricsProvider,
+      public session_manager::SessionManagerObserver,
+      public signin::IdentityManager::Observer {
  public:
   // These enum values represent the current user's log segment for the Family
   // Experiences team's metrics.
@@ -21,7 +28,8 @@ class FamilyUserMetricsProvider : public metrics::MetricsProvider {
   // numeric values should never be reused. Please keep in sync with
   // "FamilyUserLogSegment" in src/tools/metrics/histograms/enums.xml.
   enum class LogSegment {
-    // User does not fall into any of the below categories.
+    // User does not fall into any of the below categories. For example, this
+    // bucket includes regular users.
     kOther = 0,
     // Supervised primary account with no secondary accounts.
     kSupervisedUser = 1,
@@ -38,18 +46,33 @@ class FamilyUserMetricsProvider : public metrics::MetricsProvider {
     kMaxValue = kStudentAtHome
   };
 
-  // Family user metrics log segment histogram name.
-  static const char kFamilyUserLogSegmentHistogramName[];
-
   FamilyUserMetricsProvider();
-  ~FamilyUserMetricsProvider() override;
   FamilyUserMetricsProvider(const FamilyUserMetricsProvider&) = delete;
   FamilyUserMetricsProvider& operator=(const FamilyUserMetricsProvider&) =
       delete;
+  ~FamilyUserMetricsProvider() override;
 
   // MetricsProvider:
   void ProvideCurrentSessionData(
       metrics::ChromeUserMetricsExtension* uma_proto_unused) override;
+
+  // session_manager::SessionManagerObserver:
+  void OnUserSessionStarted(bool is_primary_user) override;
+
+  // signin::IdentityManager::Observer:
+  void OnRefreshTokenUpdatedForAccount(
+      const CoreAccountInfo& account_info) override;
+
+  static const char* GetHistogramNameForTesting();
+
+ private:
+  // The only way the |log_segment_| can change during a ChromeOS session is if
+  // a child user adds an EDU secondary account. Since this action doesn't
+  // happen often, cache the log segment.
+  base::Optional<LogSegment> log_segment_;
+
+  ScopedObserver<signin::IdentityManager, signin::IdentityManager::Observer>
+      identity_manager_observer_;
 };
 
 #endif  // CHROME_BROWSER_METRICS_FAMILY_USER_METRICS_PROVIDER_H_

chrome/browser/metrics/family_user_metrics_provider_browsertest.cc

@@ -6,7 +6,12 @@
 
 #include "base/optional.h"
 #include "base/test/metrics/histogram_tester.h"
+#include "base/test/scoped_feature_list.h"
+#include "base/time/time.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/chromeos/child_accounts/family_features.h"
 #include "chrome/browser/chromeos/login/test/fake_gaia_mixin.h"
+#include "chrome/browser/chromeos/login/test/guest_session_mixin.h"
 #include "chrome/browser/chromeos/login/test/scoped_policy_update.h"
 #include "chrome/browser/chromeos/login/test/user_policy_mixin.h"
 #include "chrome/browser/profiles/profile.h"
@@ -15,10 +20,13 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/test/base/mixin_based_in_process_browser_test.h"
 #include "components/account_id/account_id.h"
+#include "components/metrics/metrics_service.h"
 #include "components/signin/public/identity_manager/account_info.h"
 #include "components/signin/public/identity_manager/identity_manager.h"
 #include "components/signin/public/identity_manager/identity_test_utils.h"
 #include "content/public/test/browser_test.h"
+#include "third_party/metrics_proto/chrome_user_metrics_extension.pb.h"
+#include "third_party/metrics_proto/system_profile.pb.h"
 
 namespace {
 
@@ -56,6 +64,20 @@ base::Optional<AccountId> GetPrimaryAccountId(
   return base::nullopt;
 }
 
+void ProvideCurrentSessionData() {
+  // The purpose of the below call is to avoid a DCHECK failure in an unrelated
+  // metrics provider, in |FieldTrialsProvider::ProvideCurrentSessionData()|.
+  metrics::SystemProfileProto system_profile_proto;
+  g_browser_process->metrics_service()
+      ->GetDelegatingProviderForTesting()
+      ->ProvideSystemProfileMetricsWithLogCreationTime(base::TimeTicks::Now(),
+                                                       &system_profile_proto);
+  metrics::ChromeUserMetricsExtension uma_proto;
+  g_browser_process->metrics_service()
+      ->GetDelegatingProviderForTesting()
+      ->ProvideCurrentSessionData(&uma_proto);
+}
+
 }  // namespace
 
 class FamilyUserMetricsProviderTest
@@ -63,6 +85,11 @@ class FamilyUserMetricsProviderTest
       public testing::WithParamInterface<
           FamilyUserMetricsProvider::LogSegment> {
  public:
+  FamilyUserMetricsProviderTest() {
+    scoped_feature_list_.InitAndEnableFeature(
+        chromeos::kFamilyUserMetricsProvider);
+  }
+
   void SetUpInProcessBrowserTestFixture() override {
     MixinBasedInProcessBrowserTest::SetUpInProcessBrowserTestFixture();
 
@@ -85,18 +112,20 @@ class FamilyUserMetricsProviderTest
       // PolicyData.
       // TODO(crbug/1112885): Use LocalPolicyTestServer when this is fixed.
       /*use_local_policy_server=*/false};
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
 };
 
-IN_PROC_BROWSER_TEST_P(FamilyUserMetricsProviderTest, DISABLED_UserCategory) {
+IN_PROC_BROWSER_TEST_P(FamilyUserMetricsProviderTest, UserCategory) {
   base::HistogramTester histogram_tester;
-  FamilyUserMetricsProvider provider;
   // Simulate calling ProvideCurrentSessionData() prior to logging in.
   // This call should return prematurely.
-  provider.ProvideCurrentSessionData(/*uma_proto_unused=*/nullptr);
+  ProvideCurrentSessionData();
 
   // No metrics were recorded.
   histogram_tester.ExpectTotalCount(
-      FamilyUserMetricsProvider::kFamilyUserLogSegmentHistogramName, 0);
+      FamilyUserMetricsProvider::GetHistogramNameForTesting(), 0);
 
   logged_in_user_mixin_.LogInUser();
 
@@ -115,11 +144,10 @@ IN_PROC_BROWSER_TEST_P(FamilyUserMetricsProviderTest, DISABLED_UserCategory) {
   }
 
   // Simulate calling ProvideCurrentSessionData() after logging in.
-  provider.ProvideCurrentSessionData(/*uma_proto_unused=*/nullptr);
+  ProvideCurrentSessionData();
 
   histogram_tester.ExpectUniqueSample(
-      FamilyUserMetricsProvider::kFamilyUserLogSegmentHistogramName,
-      log_segment, 1);
+      FamilyUserMetricsProvider::GetHistogramNameForTesting(), log_segment, 1);
 }
 
 INSTANTIATE_TEST_SUITE_P(
@@ -129,3 +157,29 @@ INSTANTIATE_TEST_SUITE_P(
                     FamilyUserMetricsProvider::LogSegment::kSupervisedStudent,
                     FamilyUserMetricsProvider::LogSegment::kStudentAtHome,
                     FamilyUserMetricsProvider::LogSegment::kOther));
+
+class FamilyUserMetricsProviderGuestModeTest
+    : public MixinBasedInProcessBrowserTest {
+ public:
+  FamilyUserMetricsProviderGuestModeTest() {
+    scoped_feature_list_.InitAndEnableFeature(
+        chromeos::kFamilyUserMetricsProvider);
+  }
+
+ private:
+  chromeos::GuestSessionMixin guest_session_mixin_{&mixin_host_};
+
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+// Prevents a regression to crbug/1137352.
+IN_PROC_BROWSER_TEST_F(FamilyUserMetricsProviderGuestModeTest,
+                       NoCrashInGuestMode) {
+  base::HistogramTester histogram_tester;
+
+  ProvideCurrentSessionData();
+
+  histogram_tester.ExpectUniqueSample(
+      FamilyUserMetricsProvider::GetHistogramNameForTesting(),
+      FamilyUserMetricsProvider::LogSegment::kOther, 1);
+}

components/metrics/metrics_service.h

@@ -182,6 +182,10 @@ class MetricsService : public base::HistogramFlattener {
   // Test hook to safely stage the current log in the log store.
   bool StageCurrentLogForTest();
 
+  DelegatingProvider* GetDelegatingProviderForTesting() {
+    return &delegating_provider_;
+  }
+
  protected:
   // Sets the persistent system profile. Virtual for tests.
   virtual void SetPersistentSystemProfile(const std::string& serialized_proto,