Track previous sync enabled state in url consent helper.

When sync state events came in, the current/new value of
IsSyncFeatureEnabled would leak into the old_state value in the url
consent helper. This caused transitions from kInvalid to kDisabled to
be missed, and no associated event was fired to ConsentThrottle. This
missed event caused an invariant in ConsentThrottle to be violated,
triggering a CHECK and crashing the browser.

This change fixes this by only reading IsSyncFeatureEnabled during an
update, and storing it in a member field to be used when calculating
the current state.

(cherry picked from commit d42904f)

Bug: 1483454
Change-Id: I2e41ae4a2c4e9fe9d686fe74cfc9e8d5c939ff22
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4879153
Reviewed-by: Brandon Wylie <wylieb@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: Sky Malice <skym@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Sophie Chang <sophiechang@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1199747}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4890053
Commit-Queue: Krishna Govind <govind@chromium.org>
Reviewed-by: Krishna Govind <govind@chromium.org>
Owners-Override: Krishna Govind <govind@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/5993@{#796}
Cr-Branched-From: 5113507-refs/heads/main@{#1192594}

components/unified_consent/url_keyed_data_collection_consent_helper.cc

@@ -74,6 +74,7 @@ class SyncBasedUrlKeyedDataCollectionConsentHelper
   const bool require_sync_feature_enabled_;
   raw_ptr<syncer::SyncService> sync_service_;
   std::map<syncer::ModelType, syncer::UploadState> sync_data_type_states_;
+  bool sync_feature_state_ = false;
 };
 
 PrefBasedUrlKeyedDataCollectionConsentHelper::
@@ -124,12 +125,12 @@ SyncBasedUrlKeyedDataCollectionConsentHelper::
     sync_service_->RemoveObserver(this);
 }
 
+// Note: This method must only consume cached state (not query anything from
+// SyncService), to ensure that the state-change detection in OnStateChanged()
+// works correctly.
 UrlKeyedDataCollectionConsentHelper::State
 SyncBasedUrlKeyedDataCollectionConsentHelper::GetConsentState() {
-  // TODO(crbug.com/1462552): Find a replacement once IsSyncFeatureEnabled()
-  // starts always returning false.
-  if (require_sync_feature_enabled_ &&
-      (!sync_service_ || !sync_service_->IsSyncFeatureEnabled())) {
+  if (require_sync_feature_enabled_ && !sync_feature_state_) {
     return State::kDisabled;
   }
 
@@ -172,6 +173,12 @@ void SyncBasedUrlKeyedDataCollectionConsentHelper::OnSyncShutdown(
 }
 
 void SyncBasedUrlKeyedDataCollectionConsentHelper::UpdateSyncDataTypeStates() {
+  if (require_sync_feature_enabled_) {
+    // TODO(crbug.com/1462552): Find a replacement once IsSyncFeatureEnabled()
+    // starts always returning false.
+    sync_feature_state_ =
+        sync_service_ && sync_service_->IsSyncFeatureEnabled();
+  }
   for (auto& [model_type, upload_state] : sync_data_type_states_) {
     upload_state = syncer::GetUploadToGoogleState(sync_service_, model_type);
   }

components/unified_consent/url_keyed_data_collection_consent_helper_unittest.cc

@@ -154,6 +154,32 @@ TEST_F(UrlKeyedDataCollectionConsentHelperTest,
   helper->RemoveObserver(this);
 }
 
+TEST_F(UrlKeyedDataCollectionConsentHelperTest,
+       PersonalizedBookmarksDataCollection_IsSyncFeatureEnabled) {
+  std::unique_ptr<UrlKeyedDataCollectionConsentHelper> helper =
+      UrlKeyedDataCollectionConsentHelper::
+          NewPersonalizedBookmarksDataCollectionConsentHelper(
+              &sync_service_,
+              /*require_sync_feature_enabled=*/true);
+  sync_service_.GetUserSettings()->SetSelectedTypes(
+      /*sync_everything=*/false,
+      /*types=*/{syncer::UserSelectableType::kBookmarks});
+  sync_service_.FireOnStateChangeOnAllObservers();
+  EXPECT_TRUE(sync_service_.IsSyncFeatureEnabled());
+  EXPECT_TRUE(helper->IsEnabled());
+
+  helper->AddObserver(this);
+  sync_service_.SetHasSyncConsent(false);
+  EXPECT_FALSE(sync_service_.IsSyncFeatureEnabled());
+  EXPECT_TRUE(helper->IsEnabled());
+  EXPECT_EQ(0U, state_changed_notifications_.size());
+
+  sync_service_.FireOnStateChangeOnAllObservers();
+  EXPECT_FALSE(helper->IsEnabled());
+  EXPECT_EQ(1U, state_changed_notifications_.size());
+  helper->RemoveObserver(this);
+}
+
 TEST_F(UrlKeyedDataCollectionConsentHelperTest,
        PersonalizedBookmarksDataCollection_NullSyncService) {
   std::unique_ptr<UrlKeyedDataCollectionConsentHelper> helper =