chromeos: Fix SplitSettingsSync sync pref migration

Fix a bug where an existing user could auto-update to M85, get
randomized into the SplitSettingsSync field trial, but end up
with OS sync disabled. This results in apps, OS preferences,
Wi-Fi configs and wallpaper not syncing.

The root cause is that when we migrate their per-data-type sync
prefs (e.g. apps -> OS apps, preferences -> OS preferences) we
were not enabling the global OS sync feature pref. That pref
has a "safe" default value of false, so OS sync was never enabled.

This does not affect new users, because they see the OOBE sync
consent dialog, which enables OS sync if the user opts in.

This CL also fixes migration of the OS wallpaper sync pref, which
should be set if the user was previously syncing both apps and
themes.

Some of the regression came from this CL, though migration wasn't
quite right before either:
https://chromium-review.googlesource.com/c/chromium/src/+/2197489

We noticed this in M85 because that's the first milestone that
contains the SplitSettingsSync trial.

Bug: 1116046
Test: added to unit_tests

Change-Id: Ie40fc2b4bdc194a16b7b756c82f1e2bc258c60ae
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2354724
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#798307}

chrome/browser/chromeos/login/session/user_session_manager.cc

@@ -1434,7 +1434,10 @@ void UserSessionManager::InitProfilePreferences(
 void UserSessionManager::UserProfileInitialized(Profile* profile,
                                                 bool is_incognito_profile,
                                                 const AccountId& account_id) {
-  os_sync_util::MigrateOsSyncPreferences(profile->GetPrefs());
+  // Only migrate sync prefs for existing users. New users are given the choice
+  // to turn on OS sync in OOBE, so they get the default sync pref values.
+  if (!IsNewProfile(profile))
+    os_sync_util::MigrateOsSyncPreferences(profile->GetPrefs());
 
   // http://crbug/866790: After Supervised Users are deprecated, remove this.
   bool is_supervised_user =

chrome/browser/chromeos/sync/os_sync_util.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/chromeos/sync/os_sync_util.h"
 
 #include "base/metrics/histogram_functions.h"
+#include "chrome/browser/ui/webui/settings/chromeos/pref_names.h"
 #include "chromeos/constants/chromeos_features.h"
 #include "chromeos/constants/chromeos_pref_names.h"
 #include "components/prefs/pref_service.h"
@@ -16,13 +17,16 @@ namespace {
 // Returns true if the prefs were migrated.
 bool MaybeMigratePreferences(PrefService* prefs) {
   // Migration code can be removed when SplitSettingsSync has been fully
-  // deployed to stable channel, likely in July 2020. When doing this, change
+  // deployed to stable channel, likely December 2020. When doing this, change
   // the pref kOsSyncFeatureEnabled to default to true and delete the pref
   // kOsSyncPrefsMigrated.
   if (!chromeos::features::IsSplitSettingsSyncEnabled()) {
     // Reset the migration flag because this might be a rollback of the feature.
     // We want migration to happen again when the feature is enabled.
     prefs->SetBoolean(syncer::prefs::kOsSyncPrefsMigrated, false);
+    // Reset the OS sync pref to its default state, such that we get the same
+    // migration behavior next time SplitSettingsSync is enabled.
+    prefs->SetBoolean(syncer::prefs::kOsSyncFeatureEnabled, false);
     return false;
   }
 
@@ -32,13 +36,31 @@ bool MaybeMigratePreferences(PrefService* prefs) {
 
   // OS sync model types get their initial state from the corresponding browser
   // model types.
-  prefs->SetBoolean(
-      syncer::prefs::kSyncAllOsTypes,
-      prefs->GetBoolean(syncer::prefs::kSyncKeepEverythingSynced));
-  prefs->SetBoolean(syncer::prefs::kSyncOsApps,
-                    prefs->GetBoolean(syncer::prefs::kSyncApps));
-  prefs->SetBoolean(syncer::prefs::kSyncOsPreferences,
-                    prefs->GetBoolean(syncer::prefs::kSyncPreferences));
+  bool sync_all = prefs->GetBoolean(syncer::prefs::kSyncKeepEverythingSynced);
+  prefs->SetBoolean(syncer::prefs::kSyncAllOsTypes, sync_all);
+
+  bool sync_apps = prefs->GetBoolean(syncer::prefs::kSyncApps);
+  prefs->SetBoolean(syncer::prefs::kSyncOsApps, sync_apps);
+
+  bool sync_preferences = prefs->GetBoolean(syncer::prefs::kSyncPreferences);
+  prefs->SetBoolean(syncer::prefs::kSyncOsPreferences, sync_preferences);
+
+  // Wallpaper requires both theme sync (called "Themes & Wallpaper" in sync
+  // settings) and app sync (to actually sync the data from the wallpaper app).
+  bool sync_wallpaper =
+      sync_apps && prefs->GetBoolean(syncer::prefs::kSyncThemes);
+  prefs->SetBoolean(chromeos::settings::prefs::kSyncOsWallpaper,
+                    sync_wallpaper);
+
+  // No need to migrate Wi-Fi. There's not a separate OS pref for it.
+  bool sync_wifi = prefs->GetBoolean(syncer::prefs::kSyncWifiConfigurations);
+
+  // Enable the OS sync feature if any OS data type is enabled. Otherwise the
+  // user would stop syncing a type that they were syncing before.
+  if (sync_all || sync_apps || sync_preferences || sync_wallpaper ||
+      sync_wifi) {
+    prefs->SetBoolean(syncer::prefs::kOsSyncFeatureEnabled, true);
+  }
 
   prefs->SetBoolean(syncer::prefs::kOsSyncPrefsMigrated, true);
   return true;

chrome/browser/chromeos/sync/os_sync_util_unittest.cc

@@ -6,6 +6,8 @@
 
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
+#include "chrome/browser/ui/webui/settings/chromeos/os_settings_ui.h"
+#include "chrome/browser/ui/webui/settings/chromeos/pref_names.h"
 #include "chromeos/constants/chromeos_features.h"
 #include "components/sync/base/pref_names.h"
 #include "components/sync/base/sync_prefs.h"
@@ -13,12 +15,14 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace sp = syncer::prefs;
+namespace csp = chromeos::settings::prefs;
 
 class OsSyncUtilTest : public testing::Test {
  public:
   OsSyncUtilTest() {
     feature_list_.InitAndEnableFeature(chromeos::features::kSplitSettingsSync);
     syncer::SyncPrefs::RegisterProfilePrefs(prefs_.registry());
+    chromeos::settings::OSSettingsUI::RegisterProfilePrefs(prefs_.registry());
   }
 
   base::test::ScopedFeatureList feature_list_;
@@ -28,6 +32,7 @@ class OsSyncUtilTest : public testing::Test {
 TEST_F(OsSyncUtilTest, SimpleMigration) {
   os_sync_util::MigrateOsSyncPreferences(&prefs_);
   EXPECT_TRUE(prefs_.GetBoolean(sp::kOsSyncPrefsMigrated));
+  EXPECT_TRUE(prefs_.GetBoolean(sp::kOsSyncFeatureEnabled));
   EXPECT_TRUE(prefs_.GetBoolean(sp::kSyncAllOsTypes));
 }
 
@@ -36,6 +41,7 @@ TEST_F(OsSyncUtilTest, MigrationWithIndividualBrowserTypes) {
   prefs_.SetBoolean(sp::kSyncKeepEverythingSynced, false);
   prefs_.SetBoolean(sp::kSyncApps, true);
   prefs_.SetBoolean(sp::kSyncPreferences, true);
+  prefs_.SetBoolean(sp::kSyncThemes, true);
 
   os_sync_util::MigrateOsSyncPreferences(&prefs_);
 
@@ -43,6 +49,45 @@ TEST_F(OsSyncUtilTest, MigrationWithIndividualBrowserTypes) {
   EXPECT_FALSE(prefs_.GetBoolean(sp::kSyncAllOsTypes));
   EXPECT_TRUE(prefs_.GetBoolean(sp::kSyncOsApps));
   EXPECT_TRUE(prefs_.GetBoolean(sp::kSyncOsPreferences));
+  EXPECT_TRUE(prefs_.GetBoolean(csp::kSyncOsWallpaper));
+}
+
+TEST_F(OsSyncUtilTest, MigrationForWallpaperRequiresApps) {
+  prefs_.SetBoolean(sp::kSyncKeepEverythingSynced, false);
+  ASSERT_FALSE(prefs_.GetBoolean(sp::kSyncApps));
+  prefs_.SetBoolean(sp::kSyncThemes, true);
+  os_sync_util::MigrateOsSyncPreferences(&prefs_);
+  EXPECT_FALSE(prefs_.GetBoolean(csp::kSyncOsWallpaper));
+}
+
+TEST_F(OsSyncUtilTest, SyncAppsEnablesOsSyncFeature) {
+  prefs_.SetBoolean(sp::kSyncKeepEverythingSynced, false);
+  prefs_.SetBoolean(sp::kSyncApps, true);
+  os_sync_util::MigrateOsSyncPreferences(&prefs_);
+  EXPECT_TRUE(prefs_.GetBoolean(sp::kOsSyncFeatureEnabled));
+}
+
+TEST_F(OsSyncUtilTest, SyncPreferencesEnablesOsSyncFeature) {
+  prefs_.SetBoolean(sp::kSyncKeepEverythingSynced, false);
+  prefs_.SetBoolean(sp::kSyncPreferences, true);
+  os_sync_util::MigrateOsSyncPreferences(&prefs_);
+  EXPECT_TRUE(prefs_.GetBoolean(sp::kOsSyncFeatureEnabled));
+}
+
+TEST_F(OsSyncUtilTest, SyncWallpaperEnablesOsSyncFeature) {
+  prefs_.SetBoolean(sp::kSyncKeepEverythingSynced, false);
+  prefs_.SetBoolean(sp::kSyncApps, true);
+  prefs_.SetBoolean(sp::kSyncThemes, true);
+  os_sync_util::MigrateOsSyncPreferences(&prefs_);
+  ASSERT_TRUE(prefs_.GetBoolean(csp::kSyncOsWallpaper));
+  EXPECT_TRUE(prefs_.GetBoolean(sp::kOsSyncFeatureEnabled));
+}
+
+TEST_F(OsSyncUtilTest, SyncWifiEnablesOsSyncFeature) {
+  prefs_.SetBoolean(sp::kSyncKeepEverythingSynced, false);
+  prefs_.SetBoolean(sp::kSyncWifiConfigurations, true);
+  os_sync_util::MigrateOsSyncPreferences(&prefs_);
+  EXPECT_TRUE(prefs_.GetBoolean(sp::kOsSyncFeatureEnabled));
 }
 
 TEST_F(OsSyncUtilTest, MigrationOnlyHappensOnce) {
@@ -72,6 +117,7 @@ TEST_F(OsSyncUtilTest, Rollback) {
   // Do initial migration.
   os_sync_util::MigrateOsSyncPreferences(&prefs_);
   EXPECT_TRUE(prefs_.GetBoolean(sp::kOsSyncPrefsMigrated));
+  EXPECT_TRUE(prefs_.GetBoolean(sp::kOsSyncFeatureEnabled));
 
   {
     // Simulate disabling the feature (e.g. disabling via Finch).
@@ -81,6 +127,7 @@ TEST_F(OsSyncUtilTest, Rollback) {
 
     // OS sync is marked as not migrated.
     EXPECT_FALSE(prefs_.GetBoolean(sp::kOsSyncPrefsMigrated));
+    EXPECT_FALSE(prefs_.GetBoolean(sp::kOsSyncFeatureEnabled));
   }
 
   // Simulate re-enabling the feature.
@@ -91,6 +138,7 @@ TEST_F(OsSyncUtilTest, Rollback) {
 
     // OS sync is marked as migrated.
     EXPECT_TRUE(prefs_.GetBoolean(sp::kOsSyncPrefsMigrated));
+    EXPECT_TRUE(prefs_.GetBoolean(sp::kOsSyncFeatureEnabled));
   }
 }