Improve PasswordStore metrics reporting

This CL does the following changes to the code used by
ChromePasswordManagerClient:

(1) Extracts its platform-independent part into the core component.
    This will be useful when adding these metrics to iOS.
(2) Adds tests for that code.
(3) Replaces an obsolete guard against race-conditions with a
    thread-safe singleton pattern making use of modern C++.

Bug: 887409
Change-Id: Ie9ad57d75df3bcebf491aa3f32c5f4919ca93f5b
Reviewed-on: https://chromium-review.googlesource.com/1251543
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595489}

chrome/browser/password_manager/chrome_password_manager_client.cc

@@ -14,7 +14,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/singleton.h"
 #include "base/metrics/field_trial.h"
-#include "base/metrics/histogram_macros.h"
+#include "base/no_destructor.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browsing_data/browsing_data_helper.h"
@@ -44,13 +44,12 @@
 #include "components/password_manager/core/browser/hsts_query.h"
 #include "components/password_manager/core/browser/log_manager.h"
 #include "components/password_manager/core/browser/log_receiver.h"
-#include "components/password_manager/core/browser/password_bubble_experiment.h"
 #include "components/password_manager/core/browser/password_form_manager_for_ui.h"
 #include "components/password_manager/core/browser/password_manager_internals_service.h"
 #include "components/password_manager/core/browser/password_manager_metrics_util.h"
 #include "components/password_manager/core/browser/password_manager_util.h"
 #include "components/password_manager/core/browser/password_requirements_service.h"
-#include "components/password_manager/core/browser/password_sync_util.h"
+#include "components/password_manager/core/browser/store_metrics_reporter.h"
 #include "components/password_manager/core/common/credential_manager_types.h"
 #include "components/password_manager/core/common/password_manager_features.h"
 #include "components/password_manager/core/common/password_manager_pref_names.h"
@@ -60,7 +59,6 @@
 #include "components/ukm/content/source_url_recorder.h"
 #include "components/version_info/version_info.h"
 #include "content/public/browser/browser_context.h"
-#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/child_process_security_policy.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/navigation_handle.h"
@@ -126,39 +124,6 @@ const SigninManagerBase* GetSigninManager(Profile* profile) {
   return SigninManagerFactory::GetForProfile(profile);
 }
 
-// This routine is called when PasswordManagerClient is constructed.
-// Currently we report metrics only once at startup. We require
-// that this is only ever called from a single thread in order to
-// avoid needing to lock (a static boolean flag is then sufficient to
-// guarantee running only once).
-void ReportMetrics(bool password_manager_enabled,
-                   password_manager::PasswordManagerClient* client,
-                   Profile* profile) {
-  static base::PlatformThreadId initial_thread_id =
-      base::PlatformThread::CurrentId();
-  DCHECK_EQ(base::PlatformThread::CurrentId(), initial_thread_id);
-
-  static bool ran_once = false;
-  if (ran_once)
-    return;
-  ran_once = true;
-
-  password_manager::PasswordStore* store = client->GetPasswordStore();
-  // May be null in tests.
-  if (store) {
-    store->ReportMetrics(
-        password_manager::sync_util::GetSyncUsernameIfSyncingPasswords(
-            GetSyncService(profile), GetSigninManager(profile)),
-        client->GetPasswordSyncState() ==
-            password_manager::SYNCING_WITH_CUSTOM_PASSPHRASE);
-  }
-  UMA_HISTOGRAM_BOOLEAN("PasswordManager.Enabled", password_manager_enabled);
-  UMA_HISTOGRAM_BOOLEAN(
-      "PasswordManager.ShouldShowAutoSignInFirstRunExperience",
-      password_bubble_experiment::ShouldShowAutoSignInPromptFirstRunExperience(
-          profile->GetPrefs()));
-}
-
 #if !defined(OS_ANDROID)
 // Adds |observer| to the input observers of |widget_host|.
 void AddToWidgetInputEventObservers(
@@ -218,7 +183,9 @@ ChromePasswordManagerClient::ChromePasswordManagerClient(
 
   saving_and_filling_passwords_enabled_.Init(
       password_manager::prefs::kCredentialsEnableService, GetPrefs());
-  ReportMetrics(*saving_and_filling_passwords_enabled_, this, profile_);
+  static base::NoDestructor<password_manager::StoreMetricsReporter> reporter(
+      *saving_and_filling_passwords_enabled_, this, GetSyncService(profile_),
+      GetSigninManager(profile_), GetPrefs());
   driver_factory_->RequestSendLoggingAvailability();
 }
 

components/password_manager/core/browser/BUILD.gn

@@ -164,6 +164,8 @@ jumbo_static_library("browser") {
     "sql_table_builder.h",
     "statistics_table.cc",
     "statistics_table.h",
+    "store_metrics_reporter.cc",
+    "store_metrics_reporter.h",
     "suppressed_form_fetcher.cc",
     "suppressed_form_fetcher.h",
     "sync_credentials_filter.cc",
@@ -430,6 +432,7 @@ source_set("unit_tests") {
     "site_affiliation/asset_link_data_unittest.cc",
     "sql_table_builder_unittest.cc",
     "statistics_table_unittest.cc",
+    "store_metrics_reporter_unittest.cc",
     "suppressed_form_fetcher_unittest.cc",
     "sync_credentials_filter_unittest.cc",
     "sync_username_test_base.cc",

components/password_manager/core/browser/store_metrics_reporter.cc

+// Copyright 2018 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/password_manager/core/browser/store_metrics_reporter.h"
+
+#include "base/metrics/histogram_macros.h"
+#include "components/password_manager/core/browser/password_bubble_experiment.h"
+#include "components/password_manager/core/browser/password_manager_client.h"
+#include "components/password_manager/core/browser/password_store.h"
+#include "components/password_manager/core/browser/password_sync_util.h"
+
+namespace password_manager {
+
+StoreMetricsReporter::StoreMetricsReporter(
+    bool password_manager_enabled,
+    PasswordManagerClient* client,
+    const syncer::SyncService* sync_service,
+    const SigninManagerBase* signin_manager,
+    PrefService* prefs) {
+  password_manager::PasswordStore* store = client->GetPasswordStore();
+  // May be null in tests.
+  if (store) {
+    store->ReportMetrics(
+        password_manager::sync_util::GetSyncUsernameIfSyncingPasswords(
+            sync_service, signin_manager),
+        client->GetPasswordSyncState() ==
+            password_manager::SYNCING_WITH_CUSTOM_PASSPHRASE);
+  }
+  UMA_HISTOGRAM_BOOLEAN("PasswordManager.Enabled", password_manager_enabled);
+  UMA_HISTOGRAM_BOOLEAN(
+      "PasswordManager.ShouldShowAutoSignInFirstRunExperience",
+      password_bubble_experiment::ShouldShowAutoSignInPromptFirstRunExperience(
+          prefs));
+}
+
+StoreMetricsReporter::~StoreMetricsReporter() = default;
+
+}  // namespace password_manager

components/password_manager/core/browser/store_metrics_reporter.h

+// Copyright 2018 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_PASSWORD_MANAGER_CORE_BROWSER_STORE_METRICS_REPORTER_H_
+#define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_STORE_METRICS_REPORTER_H_
+
+#include "base/macros.h"
+
+class PrefService;
+class SigninManagerBase;
+
+namespace syncer {
+class SyncService;
+}  //  namespace syncer
+
+namespace password_manager {
+
+class PasswordManagerClient;
+
+// Instantiate this object to report metrics about the contents of the password
+// store. Create a static base::NoDestructor<StoreMetricsReporter> to ensure
+// that metrics are reported only once per process. This is thread-safe, because
+// C++11 guarantees that: "If control enters the declaration concurrently while
+// the variable is being initialized, the concurrent execution shall wait for
+// completion of the initialization." So the reporter is only initialised (and
+// hence reports) once.
+class StoreMetricsReporter {
+ public:
+  // Reports various metrics based on whether password manager is enabled. Uses
+  // |client| to obtain the password store and password syncing state. Uses
+  // |sync_service| and |signin_manager| to obtain the sync username to report
+  // about its presence among saved credentials. Uses the |prefs| to obtain the
+  // state of the first-run-experience bubble.
+  StoreMetricsReporter(bool password_manager_enabled,
+                       PasswordManagerClient* client,
+                       const syncer::SyncService* sync_service,
+                       const SigninManagerBase* signin_manager,
+                       PrefService* prefs);
+
+  ~StoreMetricsReporter();
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(StoreMetricsReporter);
+};
+
+}  // namespace password_manager
+
+#endif  // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_STORE_METRICS_REPORTER_H_

components/password_manager/core/browser/store_metrics_reporter_unittest.cc

+// Copyright 2018 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/password_manager/core/browser/store_metrics_reporter.h"
+
+#include "base/macros.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/test/metrics/histogram_tester.h"
+#include "components/password_manager/core/browser/mock_password_store.h"
+#include "components/password_manager/core/browser/stub_password_manager_client.h"
+#include "components/password_manager/core/browser/sync_username_test_base.h"
+#include "components/password_manager/core/common/password_manager_pref_names.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/testing_pref_service.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using ::testing::Return;
+
+namespace password_manager {
+namespace {
+
+class MockPasswordManagerClient : public StubPasswordManagerClient {
+ public:
+  MOCK_CONST_METHOD0(GetPasswordStore, PasswordStore*());
+  MOCK_CONST_METHOD0(GetPasswordSyncState, SyncState());
+};
+
+class StoreMetricsReporterTest : public SyncUsernameTestBase {
+ public:
+  StoreMetricsReporterTest() {
+    prefs_.registry()->RegisterBooleanPref(
+        password_manager::prefs::kWasAutoSignInFirstRunExperienceShown, false,
+        PrefRegistry::NO_REGISTRATION_FLAGS);
+  }
+
+  ~StoreMetricsReporterTest() override = default;
+
+ protected:
+  MockPasswordManagerClient client_;
+  TestingPrefServiceSimple prefs_;
+  DISALLOW_COPY_AND_ASSIGN(StoreMetricsReporterTest);
+};
+
+// Test that store-independent metrics are reported correctly.
+TEST_F(StoreMetricsReporterTest, StoreIndependentMetrics) {
+  for (const bool password_manager_enabled : {true, false}) {
+    for (const bool first_run_ui_shown : {true, false}) {
+      SCOPED_TRACE(testing::Message()
+                   << "password_manager_enabled=" << password_manager_enabled
+                   << ", first_run_ui_shown=" << first_run_ui_shown);
+
+      prefs_.SetBoolean(
+          password_manager::prefs::kWasAutoSignInFirstRunExperienceShown,
+          first_run_ui_shown);
+      base::HistogramTester histogram_tester;
+      EXPECT_CALL(client_, GetPasswordStore()).WillOnce(Return(nullptr));
+
+      StoreMetricsReporter reporter(password_manager_enabled, &client_,
+                                    sync_service(), signin_manager(), &prefs_);
+
+      histogram_tester.ExpectBucketCount("PasswordManager.Enabled",
+                                         password_manager_enabled, 1);
+      histogram_tester.ExpectBucketCount(
+          "PasswordManager.ShouldShowAutoSignInFirstRunExperience",
+          !first_run_ui_shown, 1);
+    }
+  }
+}
+
+// Test that sync username and syncing state are passed correctly to the
+// PasswordStore.
+TEST_F(StoreMetricsReporterTest, PasswordStore) {
+  for (const bool syncing_with_passphrase : {true, false}) {
+    SCOPED_TRACE(testing::Message()
+                 << "syncing_with_passphrase=" << syncing_with_passphrase);
+
+    auto store = base::MakeRefCounted<MockPasswordStore>();
+    const auto sync_state =
+        syncing_with_passphrase
+            ? password_manager::SYNCING_WITH_CUSTOM_PASSPHRASE
+            : password_manager::SYNCING_NORMAL_ENCRYPTION;
+    EXPECT_CALL(client_, GetPasswordSyncState()).WillOnce(Return(sync_state));
+    EXPECT_CALL(client_, GetPasswordStore()).WillOnce(Return(store.get()));
+    EXPECT_CALL(*store,
+                ReportMetrics("some.user@gmail.com", syncing_with_passphrase));
+    FakeSigninAs("some.user@gmail.com");
+
+    StoreMetricsReporter reporter(true, &client_, sync_service(),
+                                  signin_manager(), &prefs_);
+    store->ShutdownOnUIThread();
+  }
+}
+
+}  // namespace
+}  // namespace password_manager