Fixing Backup logic when Cloned Install Detected

Changing the client id backup logic so that it does not try to get
the id from the registry if there was a cloned install. This avoids the
race condition specified in crbug/1160366

Bug: 1160366
Change-Id: Iee04a5173ed7713392d15fec42ded18f2b65bc2a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2612110
Commit-Queue: Joshua Berenhaus <joshber@microsoft.com>
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Auto-Submit: Joshua Berenhaus <joshber@microsoft.com>
Reviewed-by: Jesse Doherty <jwd@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#844881}

chrome/browser/metrics/chrome_metrics_service_accessor.h

@@ -133,6 +133,7 @@ class ChromeMetricsServiceAccessor : public metrics::MetricsServiceAccessor {
   friend class ForceFieldTrialsBrowserTest;
   friend class MetricsReportingStateTest;
   friend class metrics::UkmConsentParamBrowserTest;
+  friend class ClonedInstallClientIdResetBrowserTest;
   FRIEND_TEST_ALL_PREFIXES(ChromeMetricsServiceAccessorTest,
                            MetricsReportingEnabled);
   FRIEND_TEST_ALL_PREFIXES(ChromeMetricsServicesManagerClientTest,

chrome/browser/metrics/cloned_install_client_id_reset_browsertest.cc

+// Copyright 2021 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 "build/build_config.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
+#include "chrome/installer/util/google_update_settings.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "components/metrics/metrics_pref_names.h"
+#include "components/metrics/metrics_service.h"
+#include "components/metrics/metrics_state_manager.h"
+#include "components/metrics_services_manager/metrics_services_manager.h"
+#include "content/public/test/browser_test.h"
+
+#if defined(OS_WIN)
+#include "base/test/test_reg_util_win.h"
+#endif  // defined(OS_WIN)
+
+namespace {
+const char kInitialClientId[] = "11111111-2222-aaaa-bbbb-cccccccccccc";
+}  // namespace
+
+class ClonedInstallClientIdResetBrowserTest : public InProcessBrowserTest {
+ public:
+  ClonedInstallClientIdResetBrowserTest() = default;
+  ~ClonedInstallClientIdResetBrowserTest() override = default;
+
+  void SetUp() override {
+    // Make metrics reporting work same as in Chrome branded builds, for test
+    // consistency between Chromium and Chrome builds.
+    ChromeMetricsServiceAccessor::SetForceIsMetricsReportingEnabledPrefLookup(
+        true);
+    // Based on GetParam(), either enable or disable metrics reporting.
+    ChromeMetricsServiceAccessor::SetMetricsAndCrashReportingForTesting(
+        &metrics_enabled_);
+
+// On windows, the registry is used for client info backups.
+#if defined(OS_WIN)
+    ASSERT_NO_FATAL_FAILURE(
+        registry_override_.OverrideRegistry(HKEY_LOCAL_MACHINE));
+    ASSERT_NO_FATAL_FAILURE(
+        registry_override_.OverrideRegistry(HKEY_CURRENT_USER));
+#endif  // defined(OS_WIN)
+
+    metrics::ClientInfo client_info;
+    client_info.client_id = kInitialClientId;
+    GoogleUpdateSettings::StoreMetricsClientInfo(client_info);
+
+    InProcessBrowserTest::SetUp();
+  }
+
+  metrics::MetricsService* metrics_service() {
+    return g_browser_process->GetMetricsServicesManager()->GetMetricsService();
+  }
+
+  PrefService* local_state() { return g_browser_process->local_state(); }
+
+ private:
+  bool metrics_enabled_ = true;
+
+#if defined(OS_WIN)
+  registry_util::RegistryOverrideManager registry_override_;
+#endif  // defined(OS_WIN)
+};
+
+IN_PROC_BROWSER_TEST_F(ClonedInstallClientIdResetBrowserTest,
+                       PRE_TestClonedInstallClientIdReset) {
+  local_state()->SetBoolean(metrics::prefs::kMetricsResetIds, true);
+  EXPECT_EQ(kInitialClientId, metrics_service()->GetClientId());
+}
+
+IN_PROC_BROWSER_TEST_F(ClonedInstallClientIdResetBrowserTest,
+                       TestClonedInstallClientIdReset) {
+  EXPECT_NE(kInitialClientId, metrics_service()->GetClientId());
+  EXPECT_FALSE(local_state()->GetBoolean(metrics::prefs::kMetricsResetIds));
+}

chrome/test/BUILD.gn

@@ -1126,6 +1126,7 @@ if (!is_android) {
       "../browser/media_galleries/fileapi/media_file_validator_browsertest.cc",
       "../browser/media_galleries/media_galleries_dialog_controller_mock.cc",
       "../browser/media_galleries/media_galleries_dialog_controller_mock.h",
+      "../browser/metrics/cloned_install_client_id_reset_browsertest.cc",
       "../browser/metrics/desktop_session_duration/audible_contents_tracker_browsertest.cc",
       "../browser/metrics/metrics_reporting_state_browsertest.cc",
       "../browser/metrics/metrics_service_browsertest.cc",

components/metrics/metrics_state_manager.cc

@@ -360,6 +360,12 @@ void MetricsStateManager::BackUpCurrentClientInfo() {
 }
 
 std::unique_ptr<ClientInfo> MetricsStateManager::LoadClientInfo() {
+  // If a cloned install was detected, loading ClientInfo from backup will be
+  // a race condition with clearing the backup. Skip all backup reads for this
+  // session.
+  if (metrics_ids_were_reset_)
+    return nullptr;
+
   std::unique_ptr<ClientInfo> client_info = load_client_info_.Run();
 
   // The GUID retrieved should be valid unless retrieval failed.
@@ -406,7 +412,8 @@ void MetricsStateManager::ResetMetricsIDsIfNecessary() {
   local_state_->ClearPref(prefs::kMetricsClientID);
   EntropyState::ClearPrefs(local_state_);
 
-  // Also clear the backed up client info.
+  // Also clear the backed up client info. This is asynchronus; any reads
+  // shortly after may retrieve the old ClientInfo from the backup.
   store_client_info_.Run(ClientInfo());
 }
 

components/metrics/metrics_state_manager_unittest.cc

@@ -85,6 +85,9 @@ class MetricsStateManagerTest : public testing::Test {
         client_info.reporting_enabled_date;
   }
 
+  // The number of times that the code tries to load ClientInfo.
+  int client_info_load_count_ = 0;
+
  protected:
   TestingPrefServiceSimple prefs_;
 
@@ -118,6 +121,7 @@ class MetricsStateManagerTest : public testing::Test {
 
   // Hands out a copy of |fake_client_info_backup_| if it is set.
   std::unique_ptr<ClientInfo> LoadFakeClientInfoBackup() {
+    ++client_info_load_count_;
     if (!fake_client_info_backup_)
       return nullptr;
 
@@ -207,6 +211,7 @@ TEST_F(MetricsStateManagerTest, ResetMetricsIDs) {
     EXPECT_NE(kInitialClientId, state_manager->client_id());
     EXPECT_TRUE(state_manager->metrics_ids_were_reset_);
     EXPECT_EQ(kInitialClientId, state_manager->previous_client_id_);
+    EXPECT_EQ(0, client_info_load_count_);
 
     state_manager->GetLowEntropySource();
 
@@ -238,6 +243,7 @@ TEST_F(MetricsStateManagerTest, ForceClientIdCreation) {
               test_begin_time_);
 
     ASSERT_TRUE(stored_client_info_backup_);
+    EXPECT_EQ(1, client_info_load_count_);
     EXPECT_EQ(state_manager->client_id(),
               stored_client_info_backup_->client_id);
     EXPECT_EQ(kFakeInstallationDate,
@@ -257,6 +263,7 @@ TEST_F(MetricsStateManagerTest,
 
   ASSERT_TRUE(stored_client_info_backup_);
   EXPECT_NE(0, stored_client_info_backup_->installation_date);
+  EXPECT_EQ(1, client_info_load_count_);
 }
 
 #if !defined(OS_WIN)
@@ -283,6 +290,7 @@ TEST_F(MetricsStateManagerTest, ProvisionalClientId_PromotedToClientId) {
   EXPECT_EQ(client_id, prefs_.GetString(prefs::kMetricsClientID));
   EXPECT_TRUE(state_manager->provisional_client_id_.empty());
   EXPECT_EQ(low_entropy_source, state_manager->GetLowEntropySource());
+  EXPECT_EQ(1, client_info_load_count_);
 }
 
 TEST_F(MetricsStateManagerTest, ProvisionalClientId_NotPersisted) {
@@ -349,6 +357,7 @@ TEST_F(MetricsStateManagerTest, LoadPrefs) {
     state_manager->ForceClientIdCreation();
     EXPECT_FALSE(stored_client_info_backup_);
     EXPECT_EQ(client_info.client_id, state_manager->client_id());
+    EXPECT_EQ(0, client_info_load_count_);
   }
 }
 
@@ -376,6 +385,7 @@ TEST_F(MetricsStateManagerTest, PreferPrefs) {
 
     // The backup should not be modified.
     ASSERT_FALSE(stored_client_info_backup_);
+    EXPECT_EQ(0, client_info_load_count_);
   }
 }
 
@@ -410,6 +420,7 @@ TEST_F(MetricsStateManagerTest, RestoreBackup) {
               prefs_.GetInt64(prefs::kMetricsReportingEnabledTimestamp));
 
     EXPECT_TRUE(stored_client_info_backup_);
+    EXPECT_EQ(1, client_info_load_count_);
   }
 }
 
@@ -437,6 +448,7 @@ TEST_F(MetricsStateManagerTest, ResetBackup) {
     EXPECT_TRUE(state_manager->metrics_ids_were_reset_);
     EXPECT_EQ(client_info.client_id, state_manager->previous_client_id_);
     EXPECT_TRUE(stored_client_info_backup_);
+    EXPECT_EQ(0, client_info_load_count_);
 
     // The installation date should not have been affected.
     EXPECT_EQ(client_info.installation_date,
@@ -536,6 +548,7 @@ TEST_F(MetricsStateManagerTest, CheckProviderResetIds) {
   EXPECT_NE(client_info.client_id, state_manager->client_id());
   EXPECT_TRUE(state_manager->metrics_ids_were_reset_);
   EXPECT_EQ(client_info.client_id, state_manager->previous_client_id_);
+  EXPECT_EQ(0, client_info_load_count_);
 
   std::unique_ptr<MetricsProvider> provider = state_manager->GetProvider();
   SystemProfileProto system_profile;