Oobe: Fix issue with metrics client id reset after sign in

Issue is due to there's a small time difference between StatsReportingController::SetWithService and pref is properly set. We should only propagate the value from service until it is correctly set.

Bug: 1154947
Change-Id: I977249dd25e1402246b383401c9deef4ea8ebc56
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2587065Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Roman Sorokin [CET] <rsorokin@chromium.org>
Commit-Queue: Yunke Zhou <yunkez@google.com>
Cr-Commit-Position: refs/heads/master@{#838606}

chrome/browser/chromeos/settings/stats_reporting_controller.cc

@@ -85,9 +85,12 @@ void StatsReportingController::SetEnabled(Profile* profile, bool enabled) {
 bool StatsReportingController::IsEnabled() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   bool value = false;
-  if (GetOwnershipStatus() == DeviceSettingsService::OWNERSHIP_NONE &&
+  if ((GetOwnershipStatus() == DeviceSettingsService::OWNERSHIP_NONE ||
+       GetOwnershipStatus() == DeviceSettingsService::OWNERSHIP_UNKNOWN ||
+       is_value_being_set_with_service_) &&
       GetPendingValue(&value)) {
-    // Return the pending value if it exists and we are sure there is no owner:
+    // Return the pending value if it exists and we are sure there is no owner
+    // or the value has not been stored correctly in signed store:
     return value;
   }
   // Otherwise, always return the value from the signed store.
@@ -116,6 +119,20 @@ void StatsReportingController::OnOwnershipTaken(
   }
 }
 
+void StatsReportingController::OnSignedPolicyStored(bool success) {
+  if (!success)
+    return;
+  bool pending_value;
+  bool signed_value;
+  if (GetPendingValue(&pending_value) && GetSignedStoredValue(&signed_value) &&
+      pending_value == signed_value) {
+    is_value_being_set_with_service_ = false;
+    owner_settings_service_observation_.Reset();
+    ClearPendingValue();
+    NotifyObservers();
+  }
+}
+
 StatsReportingController::StatsReportingController(PrefService* local_state)
     : local_state_(local_state) {
   setting_subscription_ = CrosSettings::Get()->AddSettingsObserver(
@@ -162,9 +179,11 @@ void StatsReportingController::SetWithService(
     bool enabled) {
   VLOG(1) << "SetWithService(" << std::boolalpha << enabled << ")";
   if (service && service->IsOwner()) {
+    if (!owner_settings_service_observation_.IsObserving())
+      owner_settings_service_observation_.Observe(service);
+    is_value_being_set_with_service_ = true;
     service->SetBoolean(kStatsReportingPref, enabled);
-    ClearPendingValue();
-    NotifyObservers();
+    local_state_->SetBoolean(kPendingPref, enabled);
   } else {
     // Do nothing since we are not the owner.
     LOG(WARNING) << "Changing settings from non-owner, setting="

chrome/browser/chromeos/settings/stats_reporting_controller.h

@@ -7,9 +7,11 @@
 
 #include "base/callback_list.h"
 #include "base/memory/weak_ptr.h"
+#include "base/scoped_observation.h"
 #include "base/sequence_checker.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/chromeos/settings/device_settings_service.h"
+#include "components/ownership/owner_settings_service.h"
 
 class PrefRegistrySimple;
 class PrefService;
@@ -39,7 +41,8 @@ namespace chromeos {
 // IsEnabled will return the pending value until ownership is taken and the
 // pending value is written - from then on it will return the signed, stored
 // value from CrosSettings.
-class StatsReportingController {
+class StatsReportingController
+    : public ownership::OwnerSettingsService::Observer {
  public:
   // Manage singleton instance.
   static void Initialize(PrefService* local_state);
@@ -68,11 +71,14 @@ class StatsReportingController {
   // ownership.
   void OnOwnershipTaken(ownership::OwnerSettingsService* service);
 
+  // ownership::OwnerSettingsService::Observer implementation:
+  void OnSignedPolicyStored(bool success) override;
+
  private:
   friend class StatsReportingControllerTest;
 
   explicit StatsReportingController(PrefService* local_state);
-  ~StatsReportingController();
+  ~StatsReportingController() override;
 
   // Delegates immediately to SetWithService if |service| is ready, otherwise
   // runs SetWithService asynchronously once |service| is ready.
@@ -125,6 +131,17 @@ class StatsReportingController {
   base::CallbackList<void(void)> callback_list_;
   base::CallbackListSubscription setting_subscription_;
 
+  // Indicates if the setting value is in the process of being set with the
+  // service. There is a small period of time needed between start saving the
+  // value and before the value is stored correctly in the service. We should
+  // not use the setting value from the service if it is still in the process
+  // of being saved.
+  bool is_value_being_set_with_service_ = false;
+
+  base::ScopedObservation<ownership::OwnerSettingsService,
+                          ownership::OwnerSettingsService::Observer>
+      owner_settings_service_observation_{this};
+
   base::WeakPtrFactory<StatsReportingController> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(StatsReportingController);

chrome/browser/chromeos/settings/stats_reporting_controller_unittest.cc

@@ -122,10 +122,8 @@ TEST_F(StatsReportingControllerTest, GetAndSet_OwnershipUnknown) {
 
   std::unique_ptr<TestingProfile> user = CreateUser(no_keys);
   StatsReportingController::Get()->SetEnabled(user.get(), true);
-  // A pending value is written in case there is no owner, in which case it will
-  // be written properly when ownership is taken - but we don't read the
-  // pending value, in case there actually is an owner already.
-  EXPECT_FALSE(StatsReportingController::Get()->IsEnabled());
+  // Read from the pending value when ownership is not taken.
+  EXPECT_TRUE(StatsReportingController::Get()->IsEnabled());
   ExpectThatPendingValueIs(true);
   ExpectThatSignedStoredValueIs(false);
 
@@ -180,12 +178,12 @@ TEST_F(StatsReportingControllerTest, GetAndSet_OwnershipTaken) {
   StatsReportingController::Get()->SetEnabled(owner.get(), true);
   EXPECT_TRUE(StatsReportingController::Get()->IsEnabled());
   EXPECT_TRUE(value_at_last_notification_);
-  ExpectThatPendingValueIsNotSet();
+  ExpectThatPendingValueIs(true);
   ExpectThatSignedStoredValueIs(true);
 
   StatsReportingController::Get()->SetEnabled(owner.get(), false);
   EXPECT_FALSE(StatsReportingController::Get()->IsEnabled());
-  ExpectThatPendingValueIsNotSet();
+  ExpectThatPendingValueIs(false);
   ExpectThatSignedStoredValueIs(false);
 }
 
@@ -221,8 +219,8 @@ TEST_F(StatsReportingControllerTest, SetBeforeOwnershipTaken) {
   // Before device is owned, setting the value means writing a pending value:
   std::unique_ptr<TestingProfile> pre_ownership_user = CreateUser(no_keys);
   StatsReportingController::Get()->SetEnabled(pre_ownership_user.get(), true);
-  EXPECT_FALSE(StatsReportingController::Get()->IsEnabled());
-  EXPECT_FALSE(value_at_last_notification_);
+  EXPECT_TRUE(StatsReportingController::Get()->IsEnabled());
+  EXPECT_TRUE(value_at_last_notification_);
   ExpectThatPendingValueIs(true);
   ExpectThatSignedStoredValueIs(false);
 
@@ -237,7 +235,7 @@ TEST_F(StatsReportingControllerTest, SetBeforeOwnershipTaken) {
       OwnerSettingsServiceChromeOSFactory::GetForBrowserContext(owner.get()));
   EXPECT_TRUE(StatsReportingController::Get()->IsEnabled());
   EXPECT_TRUE(value_at_last_notification_);
-  ExpectThatPendingValueIsNotSet();
+  ExpectThatPendingValueIs(true);
   ExpectThatSignedStoredValueIs(true);
 }