Special handling of preference backup to not change local values in b…

…ackup mode.

This is to avoid restoring hijacked preferences due to backup in case they get
into backup DB.

BUG=387997

Review URL: https://codereview.chromium.org/354453003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@280093 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/sync/profile_sync_service.cc

@@ -2575,3 +2575,10 @@ GURL ProfileSyncService::GetSyncServiceURL(
   }
   return result;
 }
+
+void ProfileSyncService::StartStopBackupForTesting() {
+  if (backend_mode_ == BACKUP)
+    ShutdownImpl(browser_sync::SyncBackendHost::STOP_AND_CLAIM_THREAD);
+  else
+    backup_rollback_controller_.Start(base::TimeDelta());
+}

chrome/browser/sync/profile_sync_service.h

@@ -780,6 +780,8 @@ class ProfileSyncService : public ProfileSyncServiceBase,
   // Return the base URL of the Sync Server.
   static GURL GetSyncServiceURL(const base::CommandLine& command_line);
 
+  void StartStopBackupForTesting();
+
  protected:
   // Helper to configure the priority data types.
   void ConfigurePriorityDataTypes();

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

@@ -4,12 +4,15 @@
 
 #include "base/command_line.h"
 #include "base/message_loop/message_loop.h"
+#include "base/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/profile_sync_service.h"
 #include "chrome/browser/sync/test/integration/bookmarks_helper.h"
+#include "chrome/browser/sync/test/integration/preferences_helper.h"
 #include "chrome/browser/sync/test/integration/sync_integration_test_util.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/common/pref_names.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "sync/test/fake_server/fake_server_verifier.h"
 
@@ -41,9 +44,10 @@ class BackupModeChecker {
   explicit BackupModeChecker(ProfileSyncService* service,
                              base::TimeDelta timeout)
       : pss_(service),
-        expiration_(base::TimeTicks::Now() + timeout) {}
+        timeout_(timeout) {}
 
   bool Wait() {
+    expiration_ = base::TimeTicks::Now() + timeout_;
     base::MessageLoop::current()->PostDelayedTask(
         FROM_HERE,
         base::Bind(&BackupModeChecker::PeriodicCheck, base::Unretained(this)),
@@ -70,6 +74,7 @@ class BackupModeChecker {
   }
 
   ProfileSyncService* pss_;
+  base::TimeDelta timeout_;
   base::TimeTicks expiration_;
 };
 
@@ -131,3 +136,54 @@ IN_PROC_BROWSER_TEST_F(SingleClientBackupRollbackTest,
   const BookmarkNode* url2 = tier1_b->GetChild(0);
   ASSERT_EQ(GURL("http://www.nhl.com"), url2->url());
 }
+
+#if defined(ENABLE_PRE_SYNC_BACKUP)
+#define MAYBE_TestPrefBackupRollback TestPrefBackupRollback
+#else
+#define MAYBE_TestPrefBackupRollback DISABLED_TestPrefBackupRollback
+#endif
+// Verify local preferences are not affected by preferences in backup DB under
+// backup mode.
+IN_PROC_BROWSER_TEST_F(SingleClientBackupRollbackTest,
+                       MAYBE_TestPrefBackupRollback) {
+  const char kUrl1[] = "http://www.google.com";
+  const char kUrl2[] = "http://map.google.com";
+  const char kUrl3[] = "http://plus.google.com";
+
+  ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
+
+  preferences_helper::ChangeStringPref(0, prefs::kHomePage, kUrl1);
+
+  BackupModeChecker checker(GetSyncService(0),
+                            base::TimeDelta::FromSeconds(15));
+  ASSERT_TRUE(checker.Wait());
+
+  // Shut down backup, then change preference.
+  GetSyncService(0)->StartStopBackupForTesting();
+  preferences_helper::ChangeStringPref(0, prefs::kHomePage, kUrl2);
+
+  // Restart backup. Preference shouldn't change after backup starts.
+  GetSyncService(0)->StartStopBackupForTesting();
+  ASSERT_TRUE(checker.Wait());
+  ASSERT_EQ(kUrl2,
+            preferences_helper::GetPrefs(0)->GetString(prefs::kHomePage));
+
+  // Start sync and change preference.
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  preferences_helper::ChangeStringPref(0, prefs::kHomePage, kUrl3);
+  ASSERT_TRUE(AwaitCommitActivityCompletion(GetSyncService((0))));
+  ASSERT_TRUE(ModelMatchesVerifier(0));
+
+  // Let server return rollback command on next sync request.
+  GetFakeServer()->TriggerError(sync_pb::SyncEnums::USER_ROLLBACK);
+
+  // Make another change to trigger downloading of rollback command.
+  preferences_helper::ChangeStringPref(0, prefs::kHomePage, "");
+
+  // Wait for sync to switch to backup mode after finishing rollback.
+  ASSERT_TRUE(checker.Wait());
+
+  // Verify preference is restored.
+  ASSERT_EQ(kUrl2,
+            preferences_helper::GetPrefs(0)->GetString(prefs::kHomePage));
+}

sync/internal_api/sync_backup_manager.cc

@@ -4,6 +4,7 @@
 
 #include "sync/internal_api/sync_backup_manager.h"
 
+#include "sync/internal_api/public/read_node.h"
 #include "sync/internal_api/public/write_transaction.h"
 #include "sync/syncable/directory.h"
 #include "sync/syncable/mutable_entry.h"
@@ -52,6 +53,9 @@ void SyncBackupManager::Init(
   GetUserShare()->directory->CollectMetaHandleCounts(
       &status_.num_entries_by_type,
       &status_.num_to_delete_entries_by_type);
+
+  HideSyncPreference(PRIORITY_PREFERENCES);
+  HideSyncPreference(PREFERENCES);
 }
 
 void SyncBackupManager::SaveChanges() {
@@ -113,6 +117,34 @@ void SyncBackupManager::NormalizeEntries() {
   unsynced_.clear();
 }
 
+void SyncBackupManager::HideSyncPreference(ModelType type) {
+  WriteTransaction trans(FROM_HERE, GetUserShare());
+  ReadNode pref_root(&trans);
+  if (BaseNode::INIT_OK != pref_root.InitTypeRoot(type))
+    return;
+
+  std::vector<int64> pref_ids;
+  pref_root.GetChildIds(&pref_ids);
+  for (uint32 i = 0; i < pref_ids.size(); ++i) {
+    syncable::MutableEntry entry(trans.GetWrappedWriteTrans(),
+                                 syncable::GET_BY_HANDLE, pref_ids[i]);
+    if (entry.good()) {
+      // HACKY: Set IS_DEL to true to remove entry from parent-children
+      // index so that it's not returned when syncable service asks
+      // for sync data. Syncable service then creates entry for local
+      // model. Then the existing entry is undeleted and set to local value
+      // because it has the same unique client tag.
+      entry.PutIsDel(true);
+      entry.PutIsUnsynced(false);
+
+      // Don't persist on disk so that if backup is aborted before receiving
+      // local preference values, values in sync DB are saved.
+      GetUserShare()->directory->UnmarkDirtyEntry(
+          trans.GetWrappedWriteTrans(), &entry);
+    }
+  }
+}
+
 void SyncBackupManager::RegisterDirectoryTypeDebugInfoObserver(
     syncer::TypeDebugInfoObserver* observer) {}
 

sync/internal_api/sync_backup_manager.h

@@ -61,6 +61,12 @@ class SYNC_EXPORT_PRIVATE SyncBackupManager : public SyncRollbackManagerBase {
   // entries.
   void NormalizeEntries();
 
+  // Manipulate preference nodes so that they'll be overwritten by local
+  // preference values during model association, i.e. local wins instead of
+  // server wins. This is for preventing backup from changing preferences in
+  // case backup DB has hijacked preferences.
+  void HideSyncPreference(ModelType pref_type);
+
   // Handles of unsynced entries caused by local model changes.
   std::set<int64> unsynced_;
 

sync/internal_api/sync_backup_manager_unittest.cc

@@ -41,7 +41,7 @@ class SyncBackupManagerTest : public testing::Test {
                   NULL, NULL);
     manager->ConfigureSyncer(
           CONFIGURE_REASON_NEW_CLIENT,
-          ModelTypeSet(PREFERENCES),
+          ModelTypeSet(SEARCH_ENGINES),
           ModelTypeSet(), ModelTypeSet(), ModelTypeSet(),
           ModelSafeRoutingInfo(),
           base::Bind(&OnConfigDone, true),
@@ -67,14 +67,14 @@ TEST_F(SyncBackupManagerTest, NormalizeAndPersist) {
   scoped_ptr<SyncBackupManager> manager(new SyncBackupManager);
   InitManager(manager.get());
 
-  CreateEntry(manager->GetUserShare(), PREFERENCES, "test");
+  CreateEntry(manager->GetUserShare(), SEARCH_ENGINES, "test");
 
   {
     // New entry is local and unsynced at first.
     ReadTransaction trans(FROM_HERE, manager->GetUserShare());
     ReadNode pref(&trans);
     EXPECT_EQ(BaseNode::INIT_OK,
-              pref.InitByClientTagLookup(PREFERENCES, "test"));
+              pref.InitByClientTagLookup(SEARCH_ENGINES, "test"));
     EXPECT_FALSE(pref.GetEntry()->GetId().ServerKnows());
     EXPECT_TRUE(pref.GetEntry()->GetIsUnsynced());
   }
@@ -86,7 +86,7 @@ TEST_F(SyncBackupManagerTest, NormalizeAndPersist) {
     ReadTransaction trans(FROM_HERE, manager->GetUserShare());
     ReadNode pref(&trans);
     EXPECT_EQ(BaseNode::INIT_OK,
-              pref.InitByClientTagLookup(PREFERENCES, "test"));
+              pref.InitByClientTagLookup(SEARCH_ENGINES, "test"));
     EXPECT_TRUE(pref.GetEntry()->GetId().ServerKnows());
     EXPECT_FALSE(pref.GetEntry()->GetIsUnsynced());
   }
@@ -99,7 +99,7 @@ TEST_F(SyncBackupManagerTest, NormalizeAndPersist) {
     ReadTransaction trans(FROM_HERE, manager->GetUserShare());
     ReadNode pref(&trans);
     EXPECT_EQ(BaseNode::INIT_OK,
-              pref.InitByClientTagLookup(PREFERENCES, "test"));
+              pref.InitByClientTagLookup(SEARCH_ENGINES, "test"));
     EXPECT_TRUE(pref.GetEntry()->GetId().ServerKnows());
     EXPECT_FALSE(pref.GetEntry()->GetIsUnsynced());
   }

sync/syncable/directory.cc

@@ -1446,5 +1446,10 @@ void Directory::AppendChildHandles(const ScopedKernelLock& lock,
   }
 }
 
+void Directory::UnmarkDirtyEntry(WriteTransaction* trans, Entry* entry) {
+  CHECK(trans);
+  entry->kernel_->clear_dirty(&kernel_->dirty_metahandles);
+}
+
 }  // namespace syncable
 }  // namespace syncer

sync/syncable/directory.h

@@ -19,6 +19,7 @@
 #include "sync/internal_api/public/util/report_unrecoverable_error_function.h"
 #include "sync/internal_api/public/util/weak_handle.h"
 #include "sync/syncable/dir_open_result.h"
+#include "sync/syncable/entry.h"
 #include "sync/syncable/entry_kernel.h"
 #include "sync/syncable/metahandle_set.h"
 #include "sync/syncable/parent_child_index.h"
@@ -404,6 +405,11 @@ class SYNC_EXPORT Directory {
       const sync_pb::AttachmentIdProto& attachment_id_proto,
       Metahandles* result);
 
+  // Change entry to not dirty. Used in special case when we don't want to
+  // persist modified entry on disk. e.g. SyncBackupManager uses this to
+  // preserve sync preferences in DB on disk.
+  void UnmarkDirtyEntry(WriteTransaction* trans, Entry* entry);
+
  protected:  // for friends, mainly used by Entry constructors
   virtual EntryKernel* GetEntryByHandle(int64 handle);
   virtual EntryKernel* GetEntryByHandle(int64 metahandle,