[MERGE] Clear dogfood_group pref when signing out of a profile

This ensures that when signing out of a profile, or disabling sync, a user's dogfood group memberships will be updated (and therefore their Finch experiment eligibility will be updated on the next restart). (cherry picked from commit 26f1f82) Bug: 1458979 Change-Id: I762d19edccec4712f69ada973365ccb1e7292113 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4660998 Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Reviewed-by: Marc Treib <treib@chromium.org> Auto-Submit: James Lee <ljjlee@google.com> Commit-Queue: James Lee <ljjlee@google.com> Cr-Original-Commit-Position: refs/heads/main@{#1165958} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4670517 Commit-Queue: Sylvain Defresne <sdefresne@chromium.org> Reviewed-by: David Roger <droger@chromium.org> Reviewed-by: Colin Blundell <blundell@chromium.org> Commit-Queue: Colin Blundell <blundell@chromium.org> Cr-Commit-Position: refs/branch-heads/5845@{#384} Cr-Branched-From: 5a5dff6-refs/heads/main@{#1160321}
chromium · Jul 10, 2023 · a5a68d9 · a5a68d9
1 parent 9081d5d
commit a5a68d9
Show file tree

Hide file tree

Showing 10 changed files with 99 additions and 4 deletions.
diff --git a/chrome/browser/sync/chrome_sync_client.cc b/chrome/browser/sync/chrome_sync_client.cc
@@ -20,6 +20,7 @@
 #include "chrome/browser/favicon/favicon_service_factory.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/invalidation/profile_invalidation_provider_factory.h"
+#include "chrome/browser/metrics/variations/google_groups_updater_service_factory.h"
 #include "chrome/browser/password_manager/account_password_store_factory.h"
 #include "chrome/browser/password_manager/password_store_factory.h"
 #include "chrome/browser/power_bookmarks/power_bookmark_service_factory.h"
@@ -89,6 +90,7 @@
 #include "components/sync_preferences/pref_service_syncable.h"
 #include "components/sync_sessions/session_sync_service.h"
 #include "components/sync_user_events/user_event_service.h"
+#include "components/variations/service/google_groups_updater_service.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/storage_partition.h"
@@ -757,6 +759,12 @@ syncer::SyncTypePreferenceProvider* ChromeSyncClient::GetPreferenceProvider() {
 
 void ChromeSyncClient::OnLocalSyncTransportDataCleared() {
   metrics::ClearDemographicsPrefs(profile_->GetPrefs());
+
+  GoogleGroupsUpdaterService* google_groups_updater =
+      GoogleGroupsUpdaterServiceFactory::GetForBrowserContext(profile_);
+  if (google_groups_updater != nullptr) {
+    google_groups_updater->ClearSigninScopedState();
+  }
 }
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)

diff --git a/chrome/browser/sync/sync_service_factory.cc b/chrome/browser/sync/sync_service_factory.cc
@@ -19,6 +19,7 @@
 #include "chrome/browser/favicon/favicon_service_factory.h"
 #include "chrome/browser/gcm/gcm_profile_service_factory.h"
 #include "chrome/browser/history/history_service_factory.h"
+#include "chrome/browser/metrics/variations/google_groups_updater_service_factory.h"
 #include "chrome/browser/password_manager/account_password_store_factory.h"
 #include "chrome/browser/password_manager/password_store_factory.h"
 #include "chrome/browser/power_bookmarks/power_bookmark_service_factory.h"
@@ -226,6 +227,9 @@ SyncServiceFactory::SyncServiceFactory()
   DependsOn(DeviceInfoSyncServiceFactory::GetInstance());
   DependsOn(FaviconServiceFactory::GetInstance());
   DependsOn(gcm::GCMProfileServiceFactory::GetInstance());
+  // Sync needs this service to still be present when the sync engine is
+  // disabled, so that preferences can be cleared.
+  DependsOn(GoogleGroupsUpdaterServiceFactory::GetInstance());
   DependsOn(HistoryServiceFactory::GetInstance());
   DependsOn(IdentityManagerFactory::GetInstance());
   DependsOn(ModelTypeStoreServiceFactory::GetInstance());

diff --git a/components/variations/service/google_groups_updater_service.cc b/components/variations/service/google_groups_updater_service.cc
@@ -73,6 +73,19 @@ void GoogleGroupsUpdaterService::RegisterProfilePrefs(
   );
 }
 
+void GoogleGroupsUpdaterService::ClearSigninScopedState() {
+  source_prefs_->ClearPref(
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+      variations::kOsDogfoodGroupsSyncPrefName
+#else
+      variations::kDogfoodGroupsSyncPrefName
+#endif
+  );
+
+  // UpdateGoogleGroups() will be called via the PrefChangeRegistrar, and will
+  // propagate this change to local state.
+}
+
 void GoogleGroupsUpdaterService::UpdateGoogleGroups() {
   // Get the current value of the local state dict.
   ScopedDictPrefUpdate target_prefs_update(

diff --git a/components/variations/service/google_groups_updater_service.h b/components/variations/service/google_groups_updater_service.h
@@ -42,12 +42,18 @@ class GoogleGroupsUpdaterService : public KeyedService {
 
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 
+  // Clears state that should only exist for a signed in syncing user.
+  // This should be called when the user signs out or disables sync, as this
+  // state is server-mastered state that is a property of the account rather
+  // than local state that is a property of the client.
+  void ClearSigninScopedState();
+
  private:
   // Update the group memberships in `target_prefs_`.
   // Called when `source_prefs_` have been initialized or modified.
   void UpdateGoogleGroups();
 
-  // The preferences to write to.
+  // The preferences to write to. These are the local-state prefs.
   // Preferences are guaranteed to outlive keyed services, so this reference
   // will stay valid for the lifetime of this service.
   const raw_ref<PrefService> target_prefs_;
@@ -57,7 +63,7 @@ class GoogleGroupsUpdaterService : public KeyedService {
   // across Chrome restarts).
   const std::string key_;
 
-  // The preferences to read from.
+  // The preferences to read from. These are the profile prefs.
   // Preferences are guaranteed to outlive keyed services, so this reference
   // will stay valid for the lifetime of this service.
   const raw_ref<PrefService> source_prefs_;

diff --git a/components/variations/service/google_groups_updater_service_unittest.cc b/components/variations/service/google_groups_updater_service_unittest.cc
@@ -51,6 +51,18 @@ class GoogleGroupsUpdaterServiceTest : public ::testing::Test {
                           std::move(groups_dict));
   }
 
+  void CheckSourcePrefCleared() {
+    EXPECT_TRUE(source_prefs_
+                    .GetList(
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+                        variations::kOsDogfoodGroupsSyncPrefName
+#else
+                        variations::kDogfoodGroupsSyncPrefName
+#endif
+                        )
+                    .empty());
+  }
+
   void CheckTargetPref(std::vector<std::string> expected_groups) {
     base::Value::List expected_list;
     for (const std::string& group : expected_groups) {
@@ -107,3 +119,26 @@ TEST_F(GoogleGroupsUpdaterServiceTest,
   SetSourcePref({"123", "789"});
   CheckTargetPref({"123", "789"});
 }
+
+TEST_F(GoogleGroupsUpdaterServiceTest, ClearProfilePrefsNotPreviouslySet) {
+  GoogleGroupsUpdaterService google_groups_updater(target_prefs_, key_,
+                                                   source_prefs_);
+  // This just checks that ClearSigninScopedState() deals with the case where
+  // the source pref was unset (i.e. is a no-op and doesn't crash).
+  google_groups_updater.ClearSigninScopedState();
+
+  CheckTargetPref({});
+}
+
+TEST_F(GoogleGroupsUpdaterServiceTest, ClearProfilePrefsClearsTargetPref) {
+  GoogleGroupsUpdaterService google_groups_updater(target_prefs_, key_,
+                                                   source_prefs_);
+  SetSourcePref({"123", "456"});
+  CheckTargetPref({"123", "456"});
+
+  google_groups_updater.ClearSigninScopedState();
+
+  // Check the source and target prefs have been cleared.
+  CheckSourcePrefCleared();
+  CheckTargetPref({});
+}
diff --git a/ios/chrome/browser/browser_state/BUILD.gn b/ios/chrome/browser/browser_state/BUILD.gn
@@ -102,6 +102,7 @@ source_set("browser_state") {
     "//ios/chrome/browser/language",
     "//ios/chrome/browser/mailto_handler:mailto_handler_factory",
     "//ios/chrome/browser/metrics",
+    "//ios/chrome/browser/metrics:google_groups_updater",
     "//ios/chrome/browser/net",
     "//ios/chrome/browser/optimization_guide",
     "//ios/chrome/browser/passwords",

diff --git a/ios/chrome/browser/metrics/BUILD.gn b/ios/chrome/browser/metrics/BUILD.gn
@@ -32,11 +32,25 @@ source_set("accessor") {
     "//ios/chrome/browser/shared/model/application_context",
   ]
 }
-source_set("metrics") {
+
+source_set("google_groups_updater") {
   configs += [ "//build/config/compiler:enable_arc" ]
   sources = [
     "google_groups_updater_service_factory.h",
     "google_groups_updater_service_factory.mm",
+  ]
+  deps = [
+    "//base",
+    "//components/keyed_service/ios",
+    "//components/variations/service",
+    "//ios/chrome/browser/shared/model/application_context",
+    "//ios/chrome/browser/shared/model/browser_state",
+  ]
+}
+
+source_set("metrics") {
+  configs += [ "//build/config/compiler:enable_arc" ]
+  sources = [
     "incognito_usage_app_state_agent.h",
     "incognito_usage_app_state_agent.mm",
     "incognito_web_state_observer.h",
@@ -104,11 +118,11 @@ source_set("metrics") {
     "//ios/chrome/browser/default_browser:utils",
     "//ios/chrome/browser/history",
     "//ios/chrome/browser/ntp:features",
-    "//ios/chrome/browser/shared/model/paths",
     "//ios/chrome/browser/shared/coordinator/scene:scene_state_header",
     "//ios/chrome/browser/shared/model/application_context",
     "//ios/chrome/browser/shared/model/browser",
     "//ios/chrome/browser/shared/model/browser_state",
+    "//ios/chrome/browser/shared/model/paths",
     "//ios/chrome/browser/shared/model/prefs:pref_names",
     "//ios/chrome/browser/shared/model/url:constants",
     "//ios/chrome/browser/shared/model/web_state_list",

diff --git a/ios/chrome/browser/sync/BUILD.gn b/ios/chrome/browser/sync/BUILD.gn
@@ -57,6 +57,7 @@ source_set("sync") {
     "//components/sync_sessions",
     "//components/sync_user_events",
     "//components/trusted_vault",
+    "//components/variations/service",
     "//components/version_info",
     "//google_apis",
     "//ios/chrome/browser/bookmarks",
@@ -68,6 +69,7 @@ source_set("sync") {
     "//ios/chrome/browser/gcm/instance_id",
     "//ios/chrome/browser/history",
     "//ios/chrome/browser/invalidation",
+    "//ios/chrome/browser/metrics:google_groups_updater",
     "//ios/chrome/browser/passwords:store_factory",
     "//ios/chrome/browser/power_bookmarks",
     "//ios/chrome/browser/reading_list",

diff --git a/ios/chrome/browser/sync/ios_chrome_sync_client.mm b/ios/chrome/browser/sync/ios_chrome_sync_client.mm
@@ -33,13 +33,15 @@
 #import "components/sync/service/sync_service.h"
 #import "components/sync_sessions/session_sync_service.h"
 #import "components/sync_user_events/user_event_service.h"
+#import "components/variations/service/google_groups_updater_service.h"
 #import "ios/chrome/browser/bookmarks/account_bookmark_sync_service_factory.h"
 #import "ios/chrome/browser/bookmarks/local_or_syncable_bookmark_sync_service_factory.h"
 #import "ios/chrome/browser/consent_auditor/consent_auditor_factory.h"
 #import "ios/chrome/browser/dom_distiller/dom_distiller_service_factory.h"
 #import "ios/chrome/browser/favicon/favicon_service_factory.h"
 #import "ios/chrome/browser/history/history_service_factory.h"
 #import "ios/chrome/browser/invalidation/ios_chrome_profile_invalidation_provider_factory.h"
+#import "ios/chrome/browser/metrics/google_groups_updater_service_factory.h"
 #import "ios/chrome/browser/passwords/ios_chrome_account_password_store_factory.h"
 #import "ios/chrome/browser/passwords/ios_chrome_password_store_factory.h"
 #import "ios/chrome/browser/power_bookmarks/power_bookmark_service_factory.h"
@@ -239,4 +241,10 @@
 void IOSChromeSyncClient::OnLocalSyncTransportDataCleared() {
   DCHECK_CURRENTLY_ON(web::WebThread::UI);
   metrics::ClearDemographicsPrefs(browser_state_->GetPrefs());
+
+  GoogleGroupsUpdaterService* google_groups_updater =
+      GoogleGroupsUpdaterServiceFactory::GetForBrowserState(browser_state_);
+  if (google_groups_updater != nullptr) {
+    google_groups_updater->ClearSigninScopedState();
+  }
 }
diff --git a/ios/chrome/browser/sync/sync_service_factory.mm b/ios/chrome/browser/sync/sync_service_factory.mm
@@ -32,6 +32,7 @@
 #import "ios/chrome/browser/favicon/favicon_service_factory.h"
 #import "ios/chrome/browser/gcm/ios_chrome_gcm_profile_service_factory.h"
 #import "ios/chrome/browser/history/history_service_factory.h"
+#import "ios/chrome/browser/metrics/google_groups_updater_service_factory.h"
 #import "ios/chrome/browser/passwords/ios_chrome_account_password_store_factory.h"
 #import "ios/chrome/browser/passwords/ios_chrome_password_store_factory.h"
 #import "ios/chrome/browser/reading_list/reading_list_model_factory.h"
@@ -104,6 +105,9 @@
   DependsOn(ChromeAccountManagerServiceFactory::GetInstance());
   DependsOn(ConsentAuditorFactory::GetInstance());
   DependsOn(DeviceInfoSyncServiceFactory::GetInstance());
+  // Sync needs this service to still be present when the sync engine is
+  // disabled, so that preferences can be cleared.
+  DependsOn(GoogleGroupsUpdaterServiceFactory::GetInstance());
   DependsOn(ios::AboutSigninInternalsFactory::GetInstance());
   DependsOn(ios::AccountBookmarkModelFactory::GetInstance());
   DependsOn(ios::AccountBookmarkSyncServiceFactory::GetInstance());