Sync updates for SYNCABLE_OS_PREFS back to old clients

We rely on sync to provide the user's OS settings when setting
up a new Chromebook. However, new devices are not guaranteed
to be running the latest version of Chrome before the user's
first login (it's a poor experience to have to wait to download
a large OS update before being allowed to sign in).

As part of go/split-settings-sync we are migrating OS prefs to
new sync ModelTypes OS_PREFERENCES and OS_PRIORITY_PREFERENCES.
We need to ensure that old clients still get synced updates for
these preferences.

When split sync is disabled, register OS pref names with the
browser pref ModelTypes. This makes sync work just like it did
before the split, and allows OS prefs to always be registered
as SYNCABLE_OS_PREF, without conditionals on the flag.

When split sync is enabled, register OS pref names with the
PrefModelAssociators for both the new ModelType and the old
ModelType. When a pref is changed locally, this sends updates
to the sync server under both ModelTypes.

However, when a sync change is received from the server we
only apply the update if the change is for the new ModelType.
We made a product decision not to sync updates from old clients
to new clients. This helps avoid ping-pong updates between old
and new clients.

Bug: 1013466
Test: added to components_unittests
Test: manually verified that new client with flag enabled
  sends updates to old client (M78), but old client changes
  are ignored on new client.
  updates
Test: manually verified that client with flag disabled
  syncs updates in both directions with old client (M78).

Change-Id: I04c494938d3526c57b8eb2f9c689acb0d4b0a3ed
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1865662
Commit-Queue: James Cook <jamescook@chromium.org>
Auto-Submit: James Cook <jamescook@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#708442}

components/sync_preferences/BUILD.gn

@@ -26,6 +26,10 @@ static_library("sync_preferences") {
     "//services/preferences/public/cpp:service_main",
   ]
 
+  if (is_chromeos) {
+    deps += [ "//chromeos/constants" ]
+  }
+
   if (!is_ios) {
     deps += [ "//components/policy/core/browser" ]
   }
@@ -67,4 +71,8 @@ source_set("unit_tests") {
     "//components/sync:test_support_model",
     "//testing/gtest",
   ]
+
+  if (is_chromeos) {
+    deps += [ "//chromeos/constants" ]
+  }
 }

components/sync_preferences/DEPS

 include_rules = [
+  "+chromeos/constants",
   "+components/policy/core/browser",
   "+components/policy/core/common",
   "+components/pref_registry",

components/sync_preferences/pref_model_associator.cc

@@ -229,6 +229,16 @@ syncer::SyncMergeResult PrefModelAssociator::MergeDataAndStartSyncing(
     InitPrefAndAssociate(syncer::SyncData(), *pref_name_iter, &new_changes);
   }
 
+  for (const std::string& legacy_pref_name : legacy_model_type_preferences_) {
+    // Track preferences for which we have a local user-controlled value. That
+    // could be a value from last run, or a value just set by the initial sync.
+    // We don't call InitPrefAndAssociate because we don't want the initial sync
+    // to trigger outgoing changes -- these prefs are only tracked to send
+    // updates back to older clients.
+    if (pref_service_->GetUserPrefValue(legacy_pref_name))
+      synced_preferences_.insert(legacy_pref_name);
+  }
+
   // Push updates to sync.
   merge_result.set_error(
       sync_processor_->ProcessSyncChanges(FROM_HERE, new_changes));
@@ -474,17 +484,29 @@ void PrefModelAssociator::RegisterPref(const std::string& name) {
   DCHECK(registered_preferences_.count(name) == 0);
   registered_preferences_.insert(name);
 
-  // Make sure data in the local store matches the registered type.
+  // Make sure data in the local store matches the registered type (where "type"
+  // means base::Value data type like string, not ModelType like PREFERENCES).
   // If this results in a modification of the local pref store, we don't want
   // to tell ChromeSync about these -- it's a local anomaly,
   base::AutoReset<bool> processing_changes(&processing_syncer_changes_, true);
   EnforceRegisteredTypeInStore(name);
 }
 
+void PrefModelAssociator::RegisterPrefWithLegacyModelType(
+    const std::string& name) {
+  DCHECK(legacy_model_type_preferences_.count(name) == 0);
+  DCHECK(registered_preferences_.count(name) == 0);
+  legacy_model_type_preferences_.insert(name);
+}
+
 bool PrefModelAssociator::IsPrefRegistered(const std::string& name) const {
   return registered_preferences_.count(name) > 0;
 }
 
+bool PrefModelAssociator::IsLegacyModelTypePref(const std::string& name) const {
+  return legacy_model_type_preferences_.count(name) > 0;
+}
+
 void PrefModelAssociator::ProcessPrefChange(const std::string& name) {
   if (processing_syncer_changes_)
     return;  // These are changes originating from us, ignore.
@@ -500,10 +522,12 @@ void PrefModelAssociator::ProcessPrefChange(const std::string& name) {
   if (!preference)
     return;
 
-  if (!IsPrefRegistered(name)) {
+  if (!IsPrefRegistered(name) && !IsLegacyModelTypePref(name)) {
     // We are not syncing this preference -- this also filters out synced
-    // preferences of the wrong type (priority preference are handled by a
-    // separate associator).
+    // preferences of the wrong type (e.g. priority preference are handled by a
+    // separate associator). Legacy model type preferences are allowed to
+    // continue because we want to push updates to old clients using the
+    // old ModelType.
     return;
   }
 
@@ -559,6 +583,12 @@ void PrefModelAssociator::NotifySyncedPrefObservers(const std::string& path,
   auto observer_iter = synced_pref_observers_.find(path);
   if (observer_iter == synced_pref_observers_.end())
     return;
+  // Don't notify for prefs we are only observing to support old clients.
+  // The PrefModelAssociator for the new ModelType will notify.
+  if (IsLegacyModelTypePref(path)) {
+    DCHECK(!from_sync);
+    return;
+  }
   for (auto& observer : *observer_iter->second)
     observer.OnSyncedPrefChanged(path, from_sync);
 }

components/sync_preferences/pref_model_associator.h

@@ -81,6 +81,9 @@ class PrefModelAssociator : public syncer::SyncableService {
   // begins).
   void RegisterPref(const std::string& name);
 
+  // See |legacy_model_type_preferences_|.
+  void RegisterPrefWithLegacyModelType(const std::string& name);
+
   // Process a local preference change. This can trigger new SyncChanges being
   // sent to the syncer.
   void ProcessPrefChange(const std::string& name);
@@ -108,6 +111,10 @@ class PrefModelAssociator : public syncer::SyncableService {
   // Returns true if the specified preference is registered for syncing.
   bool IsPrefRegistered(const std::string& name) const;
 
+  // See |legacy_model_type_preferences_|.
+  // Exposed for testing.
+  bool IsLegacyModelTypePref(const std::string& name) const;
+
   // Adds a SyncedPrefObserver to watch for changes to a specific pref.
   void AddSyncedPrefObserver(const std::string& name,
                              SyncedPrefObserver* observer);
@@ -188,6 +195,13 @@ class PrefModelAssociator : public syncer::SyncableService {
   // a new sync node.
   PreferenceSet synced_preferences_;
 
+  // Preferences that have migrated to a new ModelType. They are included here
+  // so updates can be sent back to older clients with this old ModelType.
+  // Updates received from older clients will be ignored. The common case is
+  // migration from PREFERENCES to OS_PREFERENCES. This field can be removed
+  // after 10/2020.
+  PreferenceSet legacy_model_type_preferences_;
+
   // The PrefService we are syncing to.
   PrefServiceSyncable* pref_service_ = nullptr;
 

components/sync_preferences/pref_service_syncable.cc

@@ -25,6 +25,10 @@
 #include "services/preferences/public/mojom/preferences.mojom.h"
 #include "services/service_manager/public/cpp/connector.h"
 
+#if defined(OS_CHROMEOS)
+#include "chromeos/constants/chromeos_features.h"
+#endif
+
 namespace sync_preferences {
 
 // TODO(tschumann): Handing out pointers to this in the constructor is an
@@ -236,11 +240,28 @@ void PrefServiceSyncable::AddRegisteredSyncablePreference(
   }
 #if defined(OS_CHROMEOS)
   if (flags & user_prefs::PrefRegistrySyncable::SYNCABLE_OS_PREF) {
+    if (chromeos::features::IsSplitSettingsSyncEnabled()) {
+      // Register the pref under the new ModelType::OS_PREFERENCES.
       os_pref_sync_associator_.RegisterPref(path);
+      // Also register under the old ModelType::PREFERENCES. This ensures that
+      // local changes to OS prefs are also synced to old clients that have the
+      // pref registered as a browser SYNCABLE_PREF.
+      pref_sync_associator_.RegisterPrefWithLegacyModelType(path);
+    } else {
+      // Behave like an old client and treat this pref like it was registered
+      // as a SYNCABLE_PREF under ModelType::PREFERENCES.
+      pref_sync_associator_.RegisterPref(path);
+    }
     return;
   }
   if (flags & user_prefs::PrefRegistrySyncable::SYNCABLE_OS_PRIORITY_PREF) {
+    // See comments for SYNCABLE_OS_PREF above.
+    if (chromeos::features::IsSplitSettingsSyncEnabled()) {
       os_priority_pref_sync_associator_.RegisterPref(path);
+      priority_pref_sync_associator_.RegisterPrefWithLegacyModelType(path);
+    } else {
+      priority_pref_sync_associator_.RegisterPref(path);
+    }
     return;
   }
 #endif

components/sync_preferences/pref_service_syncable_unittest.cc

@@ -35,13 +35,23 @@
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+#if defined(OS_CHROMEOS)
+#include "base/test/scoped_feature_list.h"
+#include "chromeos/constants/chromeos_features.h"
+#include "testing/gmock/include/gmock/gmock-matchers.h"
+#endif
+
+using syncer::ModelType;
+using syncer::ModelTypeSet;
 using syncer::SyncChange;
 using syncer::SyncData;
 using testing::Eq;
 using testing::IsEmpty;
+using testing::Matches;
 using testing::Not;
 using testing::NotNull;
 using testing::SizeIs;
+using testing::UnorderedElementsAre;
 using user_prefs::PrefRegistrySyncable;
 
 namespace sync_preferences {
@@ -60,6 +70,19 @@ const char kDefaultCharsetPrefName[] = "default_charset";
 const char kNonDefaultCharsetValue[] = "foo";
 const char kDefaultCharsetValue[] = "utf-8";
 
+#if defined(OS_CHROMEOS)
+constexpr ModelTypeSet kAllPreferenceModelTypes(
+    syncer::PREFERENCES,
+    syncer::PRIORITY_PREFERENCES,
+    syncer::OS_PREFERENCES,
+    syncer::OS_PRIORITY_PREFERENCES);
+
+MATCHER_P(MatchesModelType, model_type, "") {
+  const syncer::SyncChange& sync_change = arg;
+  return Matches(model_type)(sync_change.sync_data().GetDataType());
+}
+#endif  // defined(OS_CHROMEOS)
+
 class TestSyncProcessorStub : public syncer::SyncChangeProcessor {
  public:
   explicit TestSyncProcessorStub(syncer::SyncChangeList* output)
@@ -96,9 +119,11 @@ class TestSyncedPrefObserver : public SyncedPrefObserver {
 
   void OnSyncedPrefChanged(const std::string& path, bool from_sync) override {
     last_pref_ = path;
+    changed_count_++;
   }
 
   std::string last_pref_;
+  int changed_count_ = 0;
 };
 
 syncer::SyncChange MakeRemoteChange(int64_t id,
@@ -128,6 +153,20 @@ syncer::SyncChange MakeRemoteChange(int64_t id,
                           syncer::ModelType::PREFERENCES);
 }
 
+// Creates SyncData for a remote pref change.
+SyncData CreateRemoteSyncData(int64_t id,
+                              const std::string& name,
+                              const base::Value& value) {
+  std::string serialized;
+  JSONStringValueSerializer json(&serialized);
+  EXPECT_TRUE(json.Serialize(value));
+  sync_pb::EntitySpecifics one;
+  sync_pb::PreferenceSpecifics* pref_one = one.mutable_preference();
+  pref_one->set_name(name);
+  pref_one->set_value(serialized);
+  return SyncData::CreateRemoteData(id, one);
+}
+
 class PrefServiceSyncableTest : public testing::Test {
  public:
   PrefServiceSyncableTest()
@@ -154,15 +193,8 @@ class PrefServiceSyncableTest : public testing::Test {
   void AddToRemoteDataList(const std::string& name,
                            const base::Value& value,
                            syncer::SyncDataList* out) {
-    std::string serialized;
-    JSONStringValueSerializer json(&serialized);
-    ASSERT_TRUE(json.Serialize(value));
-    sync_pb::EntitySpecifics one;
-    sync_pb::PreferenceSpecifics* pref_one = one.mutable_preference();
-    pref_one->set_name(name);
-    pref_one->set_value(serialized);
     out->push_back(
-        SyncData::CreateRemoteData(++next_pref_remote_sync_node_id_, one));
+        CreateRemoteSyncData(++next_pref_remote_sync_node_id_, name, value));
   }
 
   void InitWithSyncDataTakeOutput(const syncer::SyncDataList& initial_data,
@@ -557,6 +589,8 @@ TEST_F(PrefServiceSyncableMergeTest, ShouldMergeSelectedDictionaryValues) {
   EXPECT_TRUE(GetPreferenceValue(kDictPrefName).Equals(&expected_dict));
 }
 
+// TODO(jamescook): In production all prefs are registered before the
+// PrefServiceSyncable is created. This test should do the same.
 TEST_F(PrefServiceSyncableMergeTest, KeepPriorityPreferencesSeparately) {
   const std::string pref_name = "testing.priority_pref";
   pref_registry_->RegisterStringPref(
@@ -572,25 +606,6 @@ TEST_F(PrefServiceSyncableMergeTest, KeepPriorityPreferencesSeparately) {
               Eq("priority-default"));
 }
 
-#if defined(OS_CHROMEOS)
-TEST_F(PrefServiceSyncableMergeTest, KeepOsPreferencesSeparately) {
-  const std::string pref_name = "testing.os_pref";
-
-  // Register a pref name as an OS pref.
-  pref_registry_->RegisterStringPref(
-      pref_name, "os-default",
-      user_prefs::PrefRegistrySyncable::SYNCABLE_OS_PREF);
-
-  // Set up sync data as a browser pref.
-  syncer::SyncDataList in;
-  // AddToRemoteDataList() produces sync data for browser prefs.
-  AddToRemoteDataList(pref_name, base::Value("browser-value"), &in);
-  syncer::SyncChangeList out;
-  InitWithSyncDataTakeOutput(in, &out);
-  EXPECT_THAT(GetPreferenceValue(pref_name).GetString(), Eq("os-default"));
-}
-#endif  // defined(OS_CHROMEOS)
-
 class ShouldNotBeNotifedObserver : public SyncedPrefObserver {
  public:
   ShouldNotBeNotifedObserver() {}
@@ -878,7 +893,7 @@ class PrefServiceSyncableChromeOsTest : public testing::Test {
         pref_notifier_(new PrefNotifierImpl),
         user_prefs_(base::MakeRefCounted<TestingPrefStore>()) {}
 
-  void SetUp() override {
+  void CreatePrefService() {
     // Register prefs of various types.
     pref_registry_->RegisterStringPref("unsynced_pref", std::string());
     pref_registry_->RegisterStringPref("browser_pref", std::string(),
@@ -905,24 +920,30 @@ class PrefServiceSyncableChromeOsTest : public testing::Test {
         /*async=*/false);
   }
 
-  void InitSyncForAllTypes() {
-    const syncer::ModelTypeSet types(
-        syncer::PREFERENCES, syncer::PRIORITY_PREFERENCES,
-        syncer::OS_PREFERENCES, syncer::OS_PRIORITY_PREFERENCES);
-    for (syncer::ModelType type : types) {
+  void InitSyncForAllTypes(syncer::SyncChangeList* output = nullptr) {
+    for (ModelType type : kAllPreferenceModelTypes) {
       syncer::SyncDataList empty_data;
       syncer::SyncMergeResult r =
           prefs_->GetSyncableService(type)->MergeDataAndStartSyncing(
-              type, empty_data,
-              std::make_unique<TestSyncProcessorStub>(nullptr),
+              type, empty_data, std::make_unique<TestSyncProcessorStub>(output),
               std::make_unique<syncer::SyncErrorFactoryMock>());
       EXPECT_FALSE(r.error().IsSet());
     }
   }
 
-  void TearDown() override { prefs_.reset(); }
+  ModelTypeSet GetRegisteredModelTypes(const std::string& pref_name) {
+    ModelTypeSet registered_types;
+    for (ModelType type : kAllPreferenceModelTypes) {
+      if (static_cast<PrefModelAssociator*>(prefs_->GetSyncableService(type))
+              ->IsPrefRegistered(pref_name)) {
+        registered_types.Put(type);
+      }
+    }
+    return registered_types;
+  }
 
  protected:
+  base::test::ScopedFeatureList feature_list_;
   scoped_refptr<PrefRegistrySyncable> pref_registry_;
   PrefNotifierImpl* pref_notifier_;  // Owned by |prefs_|.
   scoped_refptr<TestingPrefStore> user_prefs_;
@@ -930,47 +951,49 @@ class PrefServiceSyncableChromeOsTest : public testing::Test {
   std::unique_ptr<PrefServiceSyncable> prefs_;
 };
 
-TEST_F(PrefServiceSyncableChromeOsTest, IsPrefRegistered_Prefs) {
-  auto* associator = static_cast<PrefModelAssociator*>(
-      prefs_->GetSyncableService(syncer::PREFERENCES));
-  EXPECT_FALSE(associator->IsPrefRegistered("unsynced_pref"));
-  EXPECT_TRUE(associator->IsPrefRegistered("browser_pref"));
-  EXPECT_FALSE(associator->IsPrefRegistered("browser_priority_pref"));
-  EXPECT_FALSE(associator->IsPrefRegistered("os_pref"));
-  EXPECT_FALSE(associator->IsPrefRegistered("os_priority_pref"));
-}
-
-TEST_F(PrefServiceSyncableChromeOsTest, IsPrefRegistered_PriorityPrefs) {
-  auto* associator = static_cast<PrefModelAssociator*>(
-      prefs_->GetSyncableService(syncer::PRIORITY_PREFERENCES));
-  EXPECT_FALSE(associator->IsPrefRegistered("unsynced_pref"));
-  EXPECT_FALSE(associator->IsPrefRegistered("browser_pref"));
-  EXPECT_TRUE(associator->IsPrefRegistered("browser_priority_pref"));
-  EXPECT_FALSE(associator->IsPrefRegistered("os_pref"));
-  EXPECT_FALSE(associator->IsPrefRegistered("os_priority_pref"));
+TEST_F(PrefServiceSyncableChromeOsTest, IsPrefRegistered_SplitDisabled) {
+  feature_list_.InitAndDisableFeature(chromeos::features::kSplitSettingsSync);
+  CreatePrefService();
+  EXPECT_TRUE(GetRegisteredModelTypes("unsynced_pref").Empty());
+  EXPECT_EQ(ModelTypeSet(syncer::PREFERENCES),
+            GetRegisteredModelTypes("browser_pref"));
+  EXPECT_EQ(ModelTypeSet(syncer::PRIORITY_PREFERENCES),
+            GetRegisteredModelTypes("browser_priority_pref"));
+  EXPECT_EQ(ModelTypeSet(syncer::PREFERENCES),
+            GetRegisteredModelTypes("os_pref"));
+  EXPECT_EQ(ModelTypeSet(syncer::PRIORITY_PREFERENCES),
+            GetRegisteredModelTypes("os_priority_pref"));
 }
 
-TEST_F(PrefServiceSyncableChromeOsTest, IsPrefRegistered_OsPrefs) {
-  auto* associator = static_cast<PrefModelAssociator*>(
-      prefs_->GetSyncableService(syncer::OS_PREFERENCES));
-  EXPECT_FALSE(associator->IsPrefRegistered("unsynced_pref"));
-  EXPECT_FALSE(associator->IsPrefRegistered("browser_pref"));
-  EXPECT_FALSE(associator->IsPrefRegistered("browser_priority_pref"));
-  EXPECT_TRUE(associator->IsPrefRegistered("os_pref"));
-  EXPECT_FALSE(associator->IsPrefRegistered("os_priority_pref"));
-}
+TEST_F(PrefServiceSyncableChromeOsTest, IsPrefRegistered_SplitEnabled) {
+  feature_list_.InitAndEnableFeature(chromeos::features::kSplitSettingsSync);
+  CreatePrefService();
+  EXPECT_TRUE(GetRegisteredModelTypes("unsynced_pref").Empty());
+  EXPECT_EQ(ModelTypeSet(syncer::PREFERENCES),
+            GetRegisteredModelTypes("browser_pref"));
+  EXPECT_EQ(ModelTypeSet(syncer::PRIORITY_PREFERENCES),
+            GetRegisteredModelTypes("browser_priority_pref"));
+  EXPECT_EQ(ModelTypeSet(syncer::OS_PREFERENCES),
+            GetRegisteredModelTypes("os_pref"));
+  EXPECT_EQ(ModelTypeSet(syncer::OS_PRIORITY_PREFERENCES),
+            GetRegisteredModelTypes("os_priority_pref"));
+
+  // The associator for PREFERENCES knows about OS prefs so that local updates
+  // are synced back to old clients.
+  auto* pref_associator = static_cast<PrefModelAssociator*>(
+      prefs_->GetSyncableService(syncer::PREFERENCES));
+  EXPECT_TRUE(pref_associator->IsLegacyModelTypePref("os_pref"));
 
-TEST_F(PrefServiceSyncableChromeOsTest, IsPrefRegistered_OsPriorityPrefs) {
-  auto* associator = static_cast<PrefModelAssociator*>(
-      prefs_->GetSyncableService(syncer::OS_PRIORITY_PREFERENCES));
-  EXPECT_FALSE(associator->IsPrefRegistered("unsynced_pref"));
-  EXPECT_FALSE(associator->IsPrefRegistered("browser_pref"));
-  EXPECT_FALSE(associator->IsPrefRegistered("browser_priority_pref"));
-  EXPECT_FALSE(associator->IsPrefRegistered("os_pref"));
-  EXPECT_TRUE(associator->IsPrefRegistered("os_priority_pref"));
+  // The associator for PRIORITY_PREFERENCES knows about OS priority prefs so
+  // that local updates are synced back to old clients.
+  auto* priority_associator = static_cast<PrefModelAssociator*>(
+      prefs_->GetSyncableService(syncer::PRIORITY_PREFERENCES));
+  EXPECT_TRUE(priority_associator->IsLegacyModelTypePref("os_priority_pref"));
 }
 
 TEST_F(PrefServiceSyncableChromeOsTest, IsSyncing) {
+  feature_list_.InitAndEnableFeature(chromeos::features::kSplitSettingsSync);
+  CreatePrefService();
   EXPECT_FALSE(prefs_->IsSyncing());
   EXPECT_FALSE(prefs_->IsPrioritySyncing());
   InitSyncForAllTypes();
@@ -979,32 +1002,40 @@ TEST_F(PrefServiceSyncableChromeOsTest, IsSyncing) {
 }
 
 TEST_F(PrefServiceSyncableChromeOsTest, IsPrefSynced_OsPref) {
+  feature_list_.InitAndEnableFeature(chromeos::features::kSplitSettingsSync);
+  CreatePrefService();
   InitSyncForAllTypes();
-  EXPECT_FALSE(prefs_->IsPrefSynced("os_pref"));
+  auto* associator = static_cast<PrefModelAssociator*>(
+      prefs_->GetSyncableService(syncer::OS_PREFERENCES));
+  EXPECT_FALSE(associator->IsPrefSynced("os_pref"));
 
   syncer::SyncChangeList list;
   list.push_back(MakeRemoteChange(1, "os_pref", base::Value("value"),
                                   SyncChange::ACTION_ADD,
                                   syncer::OS_PREFERENCES));
-  prefs_->GetSyncableService(syncer::OS_PREFERENCES)
-      ->ProcessSyncChanges(FROM_HERE, list);
-  EXPECT_TRUE(prefs_->IsPrefSynced("os_pref"));
+  associator->ProcessSyncChanges(FROM_HERE, list);
+  EXPECT_TRUE(associator->IsPrefSynced("os_pref"));
 }
 
 TEST_F(PrefServiceSyncableChromeOsTest, IsPrefSynced_OsPriorityPref) {
+  feature_list_.InitAndEnableFeature(chromeos::features::kSplitSettingsSync);
+  CreatePrefService();
   InitSyncForAllTypes();
-  EXPECT_FALSE(prefs_->IsPrefSynced("os_priority_pref"));
+  auto* associator = static_cast<PrefModelAssociator*>(
+      prefs_->GetSyncableService(syncer::OS_PRIORITY_PREFERENCES));
+  EXPECT_FALSE(associator->IsPrefSynced("os_priority_pref"));
 
   syncer::SyncChangeList list;
   list.push_back(MakeRemoteChange(1, "os_priority_pref", base::Value("value"),
                                   SyncChange::ACTION_ADD,
                                   syncer::OS_PRIORITY_PREFERENCES));
-  prefs_->GetSyncableService(syncer::OS_PRIORITY_PREFERENCES)
-      ->ProcessSyncChanges(FROM_HERE, list);
-  EXPECT_TRUE(prefs_->IsPrefSynced("os_priority_pref"));
+  associator->ProcessSyncChanges(FROM_HERE, list);
+  EXPECT_TRUE(associator->IsPrefSynced("os_priority_pref"));
 }
 
 TEST_F(PrefServiceSyncableChromeOsTest, SyncedPrefObserver_OsPref) {
+  feature_list_.InitAndEnableFeature(chromeos::features::kSplitSettingsSync);
+  CreatePrefService();
   InitSyncForAllTypes();
 
   TestSyncedPrefObserver observer;
@@ -1012,11 +1043,14 @@ TEST_F(PrefServiceSyncableChromeOsTest, SyncedPrefObserver_OsPref) {
 
   prefs_->SetString("os_pref", "value");
   EXPECT_EQ("os_pref", observer.last_pref_);
+  EXPECT_EQ(1, observer.changed_count_);
 
   prefs_->RemoveSyncedPrefObserver("os_pref", &observer);
 }
 
 TEST_F(PrefServiceSyncableChromeOsTest, SyncedPrefObserver_OsPriorityPref) {
+  feature_list_.InitAndEnableFeature(chromeos::features::kSplitSettingsSync);
+  CreatePrefService();
   InitSyncForAllTypes();
 
   TestSyncedPrefObserver observer;
@@ -1024,9 +1058,131 @@ TEST_F(PrefServiceSyncableChromeOsTest, SyncedPrefObserver_OsPriorityPref) {
 
   prefs_->SetString("os_priority_pref", "value");
   EXPECT_EQ("os_priority_pref", observer.last_pref_);
+  EXPECT_EQ(1, observer.changed_count_);
 
   prefs_->RemoveSyncedPrefObserver("os_priority_pref", &observer);
 }
+
+TEST_F(PrefServiceSyncableChromeOsTest,
+       OsPrefChangeSyncedAsBrowserPrefChange_SplitDisabled) {
+  feature_list_.InitAndDisableFeature(chromeos::features::kSplitSettingsSync);
+  CreatePrefService();
+  // Set a non-default value.
+  prefs_->SetString("os_pref", "new_value");
+  // Start syncing.
+  syncer::SyncChangeList output;
+  InitSyncForAllTypes(&output);
+  ASSERT_EQ(1u, output.size());
+  // The OS pref is treated like a browser pref.
+  EXPECT_EQ(syncer::PREFERENCES, output[0].sync_data().GetDataType());
+}
+
+TEST_F(PrefServiceSyncableChromeOsTest,
+       OsPrefChangeSyncedAsOsPrefChange_SplitEnabled) {
+  feature_list_.InitAndEnableFeature(chromeos::features::kSplitSettingsSync);
+  CreatePrefService();
+  // Set a non-default value.
+  prefs_->SetString("os_pref", "new_value");
+  // Start syncing.
+  syncer::SyncChangeList output;
+  InitSyncForAllTypes(&output);
+  ASSERT_EQ(1u, output.size());
+  // The OS pref is treated like an OS pref.
+  EXPECT_EQ(syncer::OS_PREFERENCES, output[0].sync_data().GetDataType());
+
+  // Future changes will be synced back to browser preferences as well.
+  auto* associator = static_cast<PrefModelAssociator*>(
+      prefs_->GetSyncableService(syncer::PREFERENCES));
+  EXPECT_TRUE(associator->IsPrefSynced("os_pref"));
+}
+
+TEST_F(PrefServiceSyncableChromeOsTest,
+       OsPrefChangeMakesSyncChangeForOldClients_SplitEnabled_Update) {
+  feature_list_.InitAndEnableFeature(chromeos::features::kSplitSettingsSync);
+  CreatePrefService();
+  syncer::SyncChangeList changes;
+  InitSyncForAllTypes(&changes);
+  EXPECT_THAT(changes, IsEmpty());
+
+  // Make a local change.
+  prefs_->SetString("os_pref", "new_value");
+
+  // Sync changes are made for the legacy ModelType::PREFERENCES (so old clients
+  // will get updates) and for the current ModelType::OS_PREFERENCES (so new
+  // clients will get updates).
+  EXPECT_THAT(changes,
+              UnorderedElementsAre(MatchesModelType(syncer::PREFERENCES),
+                                   MatchesModelType(syncer::OS_PREFERENCES)));
+
+  // Future changes will be synced back to browser preferences as well.
+  auto* associator = static_cast<PrefModelAssociator*>(
+      prefs_->GetSyncableService(syncer::PREFERENCES));
+  EXPECT_TRUE(associator->IsPrefSynced("os_pref"));
+}
+
+TEST_F(PrefServiceSyncableChromeOsTest,
+       UpdatesFromOldClientsAreIgnored_Startup) {
+  feature_list_.InitAndEnableFeature(chromeos::features::kSplitSettingsSync);
+  CreatePrefService();
+  TestSyncedPrefObserver observer;
+  prefs_->AddSyncedPrefObserver("os_pref", &observer);
+
+  // Simulate an old client that has "os_pref" registered as SYNCABLE_PREF
+  // instead of SYNCABLE_OS_PREF.
+  syncer::SyncDataList list;
+  list.push_back(CreateRemoteSyncData(1, "os_pref", base::Value("new_value")));
+
+  // Simulate the first sync at startup of the legacy browser prefs ModelType.
+  auto* browser_associator = static_cast<PrefModelAssociator*>(
+      prefs_->GetSyncableService(syncer::PREFERENCES));
+  syncer::SyncChangeList outgoing_changes;
+  browser_associator->MergeDataAndStartSyncing(
+      syncer::PREFERENCES, list,
+      std::make_unique<TestSyncProcessorStub>(&outgoing_changes),
+      std::make_unique<syncer::SyncErrorFactoryMock>());
+
+  // No outgoing changes were triggered.
+  EXPECT_TRUE(outgoing_changes.empty());
+
+  // The value from the old client was not applied.
+  EXPECT_NE("new_value", prefs_->GetString("os_pref"));
+
+  // The pref is not considered to be syncing, because it still has its default
+  // value.
+  EXPECT_FALSE(browser_associator->IsPrefSynced("os_pref"));
+
+  // Observers were not notified of changes.
+  EXPECT_EQ(0, observer.changed_count_);
+
+  prefs_->RemoveSyncedPrefObserver("os_pref", &observer);
+}
+
+TEST_F(PrefServiceSyncableChromeOsTest,
+       UpdatesFromOldClientsAreIgnored_Update) {
+  feature_list_.InitAndEnableFeature(chromeos::features::kSplitSettingsSync);
+  CreatePrefService();
+  InitSyncForAllTypes();
+  TestSyncedPrefObserver observer;
+  prefs_->AddSyncedPrefObserver("os_pref", &observer);
+
+  syncer::SyncChangeList list;
+  // Simulate an old client that has "os_pref" registered as SYNCABLE_PREF
+  // instead of SYNCABLE_OS_PREF.
+  list.push_back(MakeRemoteChange(1, "os_pref", base::Value("new_value"),
+                                  SyncChange::ACTION_ADD, syncer::PREFERENCES));
+
+  // Simulate a sync update after startup.
+  prefs_->GetSyncableService(syncer::PREFERENCES)
+      ->ProcessSyncChanges(FROM_HERE, list);
+
+  // Update was not applied.
+  EXPECT_NE("new_value", prefs_->GetString("os_pref"));
+
+  // Observers were not notified of changes.
+  EXPECT_EQ(0, observer.changed_count_);
+
+  prefs_->RemoveSyncedPrefObserver("os_pref", &observer);
+}
 #endif  // defined(OS_CHROMEOS)
 
 }  // namespace