Properly distinguish between regular and priority preferences in sync.

This fixes a bug that was introduced with 13ef1e88
While the first patch properly distinguished between the two when processing
local changes it failed doing so when processing remote changes (either
incremental or initial sync changes).

The issue surfaced when a preference got moved between regular and priority
sync preferences. In that scenario, sync has values for that pref in each
flavour and one can overwrite the other.

Bug: 849523, 840332
Change-Id: Ib458c5863bc036dbb2a735ccacd52c6df2c118bb
Reviewed-on: https://chromium-review.googlesource.com/1099381Reviewed-by: Dominic Battré <battre@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Tim Schumann <tschumann@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567255}

chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc

@@ -245,3 +245,30 @@ IN_PROC_BROWSER_TEST_F(TwoClientPreferencesSyncTestWithSelfNotifications,
                             user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
   EXPECT_THAT(GetPrefs(1)->GetBoolean(pref_name), Eq(false));
 }
+
+// Verifies that priority synced preferences and regular sycned preferences are
+// kept separate.
+IN_PROC_BROWSER_TEST_F(TwoClientPreferencesSyncTestWithSelfNotifications,
+                       E2E_ENABLED(ShouldIsolatePriorityPreferences)) {
+  // Register a pref as priority with client0 and regular synced with client1.
+  ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
+  constexpr char pref_name[] = "testing.my-test-preference";
+  GetRegistry(GetProfile(0))
+      ->RegisterStringPref(
+          pref_name, "",
+          user_prefs::PrefRegistrySyncable::SYNCABLE_PRIORITY_PREF);
+  GetRegistry(GetProfile(1))
+      ->RegisterStringPref(pref_name, "",
+                           user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+
+  ChangeStringPref(0, pref_name, "priority value");
+  GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1));
+  EXPECT_THAT(GetPrefs(0)->GetString(pref_name), Eq("priority value"));
+  EXPECT_THAT(GetPrefs(1)->GetString(pref_name), Eq(""));
+
+  ChangeStringPref(1, pref_name, "non-priority value");
+  GetClient(1)->AwaitMutualSyncCycleCompletion(GetClient(0));
+  EXPECT_THAT(GetPrefs(0)->GetString(pref_name), Eq("priority value"));
+  EXPECT_THAT(GetPrefs(1)->GetString(pref_name), Eq("non-priority value"));
+}

components/sync_preferences/pref_model_associator.cc

@@ -80,7 +80,7 @@ void PrefModelAssociator::InitPrefAndAssociate(
     const std::string& pref_name,
     syncer::SyncChangeList* sync_changes) {
   UnknownUserPrefAccessor::PreferenceState local_pref_state =
-      pref_accessor_->GetPreferenceState(pref_name);
+      pref_accessor_->GetPreferenceState(type_, pref_name);
   if (local_pref_state.registration_state ==
           UnknownUserPrefAccessor::RegistrationState::kUnknown ||
       local_pref_state.registration_state ==
@@ -334,7 +334,7 @@ syncer::SyncDataList PrefModelAssociator::GetAllSyncData(
   for (PreferenceSet::const_iterator iter = synced_preferences_.begin();
        iter != synced_preferences_.end(); ++iter) {
     std::string name = *iter;
-    if (pref_accessor_->GetPreferenceState(name).registration_state !=
+    if (pref_accessor_->GetPreferenceState(type_, name).registration_state !=
         UnknownUserPrefAccessor::RegistrationState::kSyncable) {
       continue;
     }
@@ -368,7 +368,7 @@ syncer::SyncError PrefModelAssociator::ProcessSyncChanges(
         GetSpecifics(iter->sync_data());
 
     UnknownUserPrefAccessor::PreferenceState local_pref_state =
-        pref_accessor_->GetPreferenceState(pref_specifics.name());
+        pref_accessor_->GetPreferenceState(type_, pref_specifics.name());
     if (local_pref_state.registration_state ==
         UnknownUserPrefAccessor::RegistrationState::kUnknown) {
       // It is possible that we may receive a change to a preference we do not

components/sync_preferences/pref_model_associator.h

@@ -68,6 +68,9 @@ class PrefModelAssociator : public syncer::SyncableService {
       std::unique_ptr<syncer::SyncErrorFactory> sync_error_factory) override;
   void StopSyncing(syncer::ModelType type) override;
 
+  // TODO(tschumann): Replace the RegisterPref() call with a
+  // VerifyPersistedPrefType() method. All pref registration checks are now
+  // done via the registry; no need to duplicate that concept.
   // Register a preference with the specified name for syncing. We do not care
   // about the type at registration time, but when changes arrive from the
   // syncer, we check if they can be applied and if not drop them.

components/sync_preferences/pref_service_syncable_unittest.cc

@@ -576,6 +576,22 @@ TEST_F(PrefServiceSyncableMergeTest, ReceiveUnknownPrefsValue) {
   EXPECT_THAT(GetPreferenceValue(pref_name).GetString(), Eq("remote_value"));
 }
 
+TEST_F(PrefServiceSyncableMergeTest, KeepPriorityPreferencesSeparately) {
+  base::HistogramTester histogram_tester;
+  const std::string pref_name = "testing.priority_pref";
+  pref_registry_->RegisterStringPref(
+      pref_name, "priority-default",
+      user_prefs::PrefRegistrySyncable::SYNCABLE_PRIORITY_PREF);
+
+  syncer::SyncDataList in;
+  // AddToRemoteDataList() produces sync data for non-priority prefs.
+  AddToRemoteDataList(pref_name, base::Value("non-priority-value"), &in);
+  syncer::SyncChangeList out;
+  InitWithSyncDataTakeOutput(in, &out);
+  EXPECT_THAT(GetPreferenceValue(pref_name).GetString(),
+              Eq("priority-default"));
+}
+
 class ShouldNotBeNotifedObserver : public SyncedPrefObserver {
  public:
   ShouldNotBeNotifedObserver() {}

components/sync_preferences/unknown_user_pref_accessor.cc

@@ -30,9 +30,10 @@ UnknownUserPrefAccessor::~UnknownUserPrefAccessor() {}
 
 UnknownUserPrefAccessor::PreferenceState
 UnknownUserPrefAccessor::GetPreferenceState(
+    syncer::ModelType type,
     const std::string& pref_name) const {
   PreferenceState result;
-  result.registration_state = GetRegistrationState(pref_name);
+  result.registration_state = GetRegistrationState(type, pref_name);
   switch (result.registration_state) {
     case RegistrationState::kUnknown:
     case RegistrationState::kUnknownWhitelisted:
@@ -159,13 +160,32 @@ void UnknownUserPrefAccessor::EnforceRegisteredTypeInStore(
 
 UnknownUserPrefAccessor::RegistrationState
 UnknownUserPrefAccessor::GetRegistrationState(
+    syncer::ModelType type,
     const std::string& pref_name) const {
+  uint32_t type_flag = 0;
+  switch (type) {
+    case syncer::PRIORITY_PREFERENCES:
+      type_flag = user_prefs::PrefRegistrySyncable::SYNCABLE_PRIORITY_PREF;
+      break;
+    case syncer::PREFERENCES:
+      type_flag = user_prefs::PrefRegistrySyncable::SYNCABLE_PREF;
+      break;
+    default:
+      NOTREACHED() << "unexpected model type for preferences: " << type;
+  }
   if (pref_registry_->defaults()->GetValue(pref_name, nullptr)) {
     uint32_t flags = pref_registry_->GetRegistrationFlags(pref_name);
-    if ((flags & user_prefs::PrefRegistrySyncable::SYNCABLE_PREF) ||
-        (flags & user_prefs::PrefRegistrySyncable::SYNCABLE_PRIORITY_PREF)) {
+    if (flags & type_flag) {
       return RegistrationState::kSyncable;
     }
+    // Imagine the case where a preference has been synced as SYNCABLE_PREF
+    // first and then got changed to SYNCABLE_PRIORITY_PREF:
+    // In that situation, it could be argued for both, the preferences to be
+    // considered unknown or not synced. However, as we plan to eventually also
+    // sync unknown preferences, we cannot label them as unknown and treat them
+    // as not synced instead. (The underlying problem is that priority
+    // preferences are a concept only known to sync. The persistent stores don't
+    // distinguish between those two).
     return RegistrationState::kNotSyncable;
   }
   if (pref_registry_->IsWhitelistedLateRegistrationPref(pref_name)) {

components/sync_preferences/unknown_user_pref_accessor.h

@@ -12,6 +12,7 @@
 #include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/macros.h"
+#include "components/sync/base/model_type.h"
 
 class PersistentPrefStore;
 class PrefService;
@@ -59,7 +60,8 @@ class UnknownUserPrefAccessor {
 
   // Computes the state of a preference with name |pref_name| which gives
   // information about whether it's registered and the locally persisted value.
-  PreferenceState GetPreferenceState(const std::string& pref_name) const;
+  PreferenceState GetPreferenceState(syncer::ModelType type,
+                                     const std::string& pref_name) const;
 
   // Removes the value of the preference |pref_name| from the user prefstore.
   // Must not be called for preferences having RegistrationState::kUnknown.
@@ -86,7 +88,8 @@ class UnknownUserPrefAccessor {
   int GetNumberOfSyncingUnknownPrefs() const;
 
  private:
-  RegistrationState GetRegistrationState(const std::string& pref_name) const;
+  RegistrationState GetRegistrationState(syncer::ModelType type,
+                                         const std::string& pref_name) const;
 
   std::set<std::string> synced_unknown_prefs_;
   PrefService* const pref_service_;