[Sync] Rely on directory to decide which types need to be purged

Preivously we relied on the SyncBackendRegistrar's last configured types to
decide which types have been recently disabled. Now we just purge all disabled
types that exist in the directory, as recorded by their progress markers (which
should never be deleted unless the type has been purged).

BUG=386778

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

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

chrome/browser/sync/glue/sync_backend_host_impl.cc

@@ -348,8 +348,6 @@ void SyncBackendHostImpl::ConfigureDataTypes(
   // backend because configuration requests are never aborted; they are retried
   // until they succeed or the backend is shut down.
 
-  syncer::ModelTypeSet previous_types = registrar_->GetLastConfiguredTypes();
-
   syncer::ModelTypeSet disabled_types =
       GetDataTypesInState(DISABLED, config_state_map);
   syncer::ModelTypeSet fatal_types =
@@ -358,10 +356,8 @@ void SyncBackendHostImpl::ConfigureDataTypes(
       GetDataTypesInState(CRYPTO, config_state_map);
   syncer::ModelTypeSet unready_types =
       GetDataTypesInState(UNREADY, config_state_map);
-  disabled_types.PutAll(fatal_types);
 
-  // TODO(zea): These types won't be fully purged if they are subsequently
-  // disabled by the user. Fix that. See crbug.com/386778
+  disabled_types.PutAll(fatal_types);
   disabled_types.PutAll(crypto_types);
   disabled_types.PutAll(unready_types);
 
@@ -404,7 +400,7 @@ void SyncBackendHostImpl::ConfigureDataTypes(
 
   syncer::ModelTypeSet current_types = registrar_->GetLastConfiguredTypes();
   syncer::ModelTypeSet types_to_purge =
-      syncer::Difference(previous_types, current_types);
+      syncer::Difference(syncer::ModelTypeSet::All(), current_types);
   syncer::ModelTypeSet inactive_types =
       GetDataTypesInState(CONFIGURE_INACTIVE, config_state_map);
   types_to_purge.RemoveAll(inactive_types);

chrome/browser/sync/glue/sync_backend_host_impl_unittest.cc

@@ -234,7 +234,8 @@ class SyncBackendHostTest : public testing::Test {
 
   // Synchronously configures the backend's datatypes.
   void ConfigureDataTypes(syncer::ModelTypeSet types_to_add,
-                          syncer::ModelTypeSet types_to_remove) {
+                          syncer::ModelTypeSet types_to_remove,
+                          syncer::ModelTypeSet types_to_unapply) {
     BackendDataTypeConfigurer::DataTypeConfigStateMap config_state_map;
     BackendDataTypeConfigurer::SetDataTypesState(
         BackendDataTypeConfigurer::CONFIGURE_ACTIVE,
@@ -243,6 +244,9 @@ class SyncBackendHostTest : public testing::Test {
     BackendDataTypeConfigurer::SetDataTypesState(
         BackendDataTypeConfigurer::DISABLED,
         types_to_remove, &config_state_map);
+    BackendDataTypeConfigurer::SetDataTypesState(
+        BackendDataTypeConfigurer::UNREADY,
+        types_to_unapply, &config_state_map);
 
     types_to_add.PutAll(syncer::ControlTypes());
     backend_->ConfigureDataTypes(
@@ -315,7 +319,8 @@ TEST_F(SyncBackendHostTest, FirstTimeSync) {
 
   ConfigureDataTypes(enabled_types_,
                      Difference(syncer::ModelTypeSet::All(),
-                                enabled_types_));
+                                enabled_types_),
+                     syncer::ModelTypeSet());
   EXPECT_TRUE(fake_manager_->GetAndResetDownloadedTypes().HasAll(
       Difference(enabled_types_, syncer::ControlTypes())));
   EXPECT_TRUE(fake_manager_->InitialSyncEndedTypes().Equals(enabled_types_));
@@ -341,7 +346,8 @@ TEST_F(SyncBackendHostTest, Restart) {
 
   ConfigureDataTypes(enabled_types_,
                      Difference(syncer::ModelTypeSet::All(),
-                                enabled_types_));
+                                enabled_types_),
+                     syncer::ModelTypeSet());
   EXPECT_TRUE(fake_manager_->GetAndResetDownloadedTypes().Empty());
   EXPECT_TRUE(Intersection(fake_manager_->GetAndResetCleanedTypes(),
                            enabled_types_).Empty());
@@ -378,7 +384,8 @@ TEST_F(SyncBackendHostTest, PartialTypes) {
   // Now do the actual configuration, which should download and apply bookmarks.
   ConfigureDataTypes(enabled_types_,
                      Difference(syncer::ModelTypeSet::All(),
-                                enabled_types_));
+                                enabled_types_),
+                     syncer::ModelTypeSet());
   EXPECT_TRUE(Intersection(fake_manager_->GetAndResetCleanedTypes(),
                            enabled_types_).Empty());
   EXPECT_TRUE(fake_manager_->GetAndResetDownloadedTypes().Equals(
@@ -411,7 +418,8 @@ TEST_F(SyncBackendHostTest, LostDB) {
   // The actual configuration should redownload and apply all the enabled types.
   ConfigureDataTypes(enabled_types_,
                      Difference(syncer::ModelTypeSet::All(),
-                                enabled_types_));
+                                enabled_types_),
+                     syncer::ModelTypeSet());
   EXPECT_TRUE(fake_manager_->GetAndResetDownloadedTypes().HasAll(
       Difference(enabled_types_, syncer::ControlTypes())));
   EXPECT_TRUE(Intersection(fake_manager_->GetAndResetCleanedTypes(),
@@ -428,7 +436,8 @@ TEST_F(SyncBackendHostTest, DisableTypes) {
   fake_manager_->GetAndResetCleanedTypes();
   ConfigureDataTypes(enabled_types_,
                      Difference(syncer::ModelTypeSet::All(),
-                                enabled_types_));
+                                enabled_types_),
+                     syncer::ModelTypeSet());
   EXPECT_TRUE(fake_manager_->GetAndResetDownloadedTypes().Equals(
       enabled_types_));
   EXPECT_TRUE(Intersection(fake_manager_->GetAndResetCleanedTypes(),
@@ -444,7 +453,8 @@ TEST_F(SyncBackendHostTest, DisableTypes) {
   enabled_types_.RemoveAll(disabled_types);
   ConfigureDataTypes(enabled_types_,
                      Difference(syncer::ModelTypeSet::All(),
-                                enabled_types_));
+                                enabled_types_),
+                     syncer::ModelTypeSet());
 
   // Only those datatypes disabled should be cleaned. Nothing should be
   // downloaded.
@@ -463,7 +473,8 @@ TEST_F(SyncBackendHostTest, AddTypes) {
   fake_manager_->GetAndResetCleanedTypes();
   ConfigureDataTypes(enabled_types_,
                      Difference(syncer::ModelTypeSet::All(),
-                                enabled_types_));
+                                enabled_types_),
+                     syncer::ModelTypeSet());
   EXPECT_TRUE(fake_manager_->GetAndResetDownloadedTypes().Equals(
       enabled_types_));
   EXPECT_TRUE(Intersection(fake_manager_->GetAndResetCleanedTypes(),
@@ -478,7 +489,8 @@ TEST_F(SyncBackendHostTest, AddTypes) {
   enabled_types_.PutAll(new_types);
   ConfigureDataTypes(enabled_types_,
                      Difference(syncer::ModelTypeSet::All(),
-                                enabled_types_));
+                                enabled_types_),
+                     syncer::ModelTypeSet());
 
   // Only those datatypes added should be downloaded (plus nigori). Nothing
   // should be cleaned aside from the disabled types.
@@ -499,7 +511,8 @@ TEST_F(SyncBackendHostTest, AddDisableTypes) {
   fake_manager_->GetAndResetCleanedTypes();
   ConfigureDataTypes(enabled_types_,
                      Difference(syncer::ModelTypeSet::All(),
-                                enabled_types_));
+                                enabled_types_),
+                     syncer::ModelTypeSet());
   EXPECT_TRUE(fake_manager_->GetAndResetDownloadedTypes().Equals(
       enabled_types_));
   EXPECT_TRUE(Intersection(fake_manager_->GetAndResetCleanedTypes(),
@@ -518,7 +531,8 @@ TEST_F(SyncBackendHostTest, AddDisableTypes) {
   enabled_types_.RemoveAll(disabled_types);
   ConfigureDataTypes(enabled_types_,
                      Difference(syncer::ModelTypeSet::All(),
-                                enabled_types_));
+                                enabled_types_),
+                     syncer::ModelTypeSet());
 
   // Only those datatypes added should be downloaded (plus nigori). Nothing
   // should be cleaned aside from the disabled types.
@@ -557,7 +571,8 @@ TEST_F(SyncBackendHostTest, NewlySupportedTypes) {
   // Downloads and applies the new types.
   ConfigureDataTypes(enabled_types_,
                      Difference(syncer::ModelTypeSet::All(),
-                                enabled_types_));
+                                enabled_types_),
+                     syncer::ModelTypeSet());
   EXPECT_TRUE(fake_manager_->GetAndResetDownloadedTypes().Equals(
       Union(new_types, syncer::ModelTypeSet(syncer::NIGORI))));
   EXPECT_TRUE(Intersection(fake_manager_->GetAndResetCleanedTypes(),
@@ -601,7 +616,8 @@ TEST_F(SyncBackendHostTest, NewlySupportedTypesWithPartialTypes) {
   // nigori anyways).
   ConfigureDataTypes(enabled_types_,
                      Difference(syncer::ModelTypeSet::All(),
-                                enabled_types_));
+                                enabled_types_),
+                     syncer::ModelTypeSet());
   EXPECT_TRUE(fake_manager_->GetAndResetDownloadedTypes().Equals(
       Union(new_types, partial_types)));
   EXPECT_TRUE(Intersection(fake_manager_->GetAndResetCleanedTypes(),
@@ -732,6 +748,39 @@ TEST_F(SyncBackendHostTest, TestStartupWithOldSyncData) {
   EXPECT_FALSE(base::PathExists(sync_file));
 }
 
+// If bookmarks encounter an error that results in disabling without purging
+// (such as when the type is unready), and then is explicitly disabled, the
+// SyncBackendHost needs to tell the manager to purge the type, even though
+// it's already disabled (crbug.com/386778).
+TEST_F(SyncBackendHostTest, DisableThenPurgeType) {
+  syncer::ModelTypeSet error_types(syncer::BOOKMARKS);
+
+  InitializeBackend(true);
+
+  // First enable the types.
+  ConfigureDataTypes(enabled_types_,
+                     Difference(syncer::ModelTypeSet::All(),
+                                enabled_types_),
+                     syncer::ModelTypeSet());
+
+  // Then mark the error types as unready (disables without purging).
+  ConfigureDataTypes(enabled_types_,
+                     Difference(syncer::ModelTypeSet::All(),
+                                enabled_types_),
+                     error_types);
+  EXPECT_TRUE(fake_manager_->GetTypesWithEmptyProgressMarkerToken(
+      error_types).Empty());
+
+  // Lastly explicitly disable the error types, which should result in a purge.
+  enabled_types_.RemoveAll(error_types);
+  ConfigureDataTypes(enabled_types_,
+                     Difference(syncer::ModelTypeSet::All(),
+                                enabled_types_),
+                     syncer::ModelTypeSet());
+  EXPECT_FALSE(fake_manager_->GetTypesWithEmptyProgressMarkerToken(
+      error_types).Empty());
+}
+
 }  // namespace
 
 }  // namespace browser_sync

sync/engine/syncer_unittest.cc

@@ -1037,6 +1037,10 @@ TEST_F(SyncerTest, TestPurgeWhileUnsynced) {
   // Similar to above, but throw a purge operation into the mix. Bug 49278.
   syncable::Id pref_node_id = TestIdFactory::MakeServer("Tim");
   {
+    directory()->SetDownloadProgress(BOOKMARKS,
+                                     syncable::BuildProgress(BOOKMARKS));
+    directory()->SetDownloadProgress(PREFERENCES,
+                                     syncable::BuildProgress(PREFERENCES));
     WriteTransaction wtrans(FROM_HERE, UNITTEST, directory());
     MutableEntry parent(&wtrans, CREATE, BOOKMARKS, wtrans.root_id(), "Pete");
     ASSERT_TRUE(parent.good());
@@ -1086,6 +1090,8 @@ TEST_F(SyncerTest, TestPurgeWhileUnsynced) {
 TEST_F(SyncerTest, TestPurgeWhileUnapplied) {
   // Similar to above, but for unapplied items. Bug 49278.
   {
+    directory()->SetDownloadProgress(BOOKMARKS,
+                                     syncable::BuildProgress(BOOKMARKS));
     WriteTransaction wtrans(FROM_HERE, UNITTEST, directory());
     MutableEntry parent(&wtrans, CREATE, BOOKMARKS, wtrans.root_id(), "Pete");
     ASSERT_TRUE(parent.good());
@@ -1111,6 +1117,8 @@ TEST_F(SyncerTest, TestPurgeWhileUnapplied) {
 
 TEST_F(SyncerTest, TestPurgeWithJournal) {
   {
+    directory()->SetDownloadProgress(BOOKMARKS,
+                                     syncable::BuildProgress(BOOKMARKS));
     WriteTransaction wtrans(FROM_HERE, UNITTEST, directory());
     MutableEntry parent(&wtrans, syncable::CREATE, BOOKMARKS, wtrans.root_id(),
                         "Pete");

sync/syncable/directory.cc

@@ -58,6 +58,13 @@ void Directory::PersistedKernelInfo::ResetDownloadProgress(
   download_progress[model_type].set_token("");
 }
 
+bool Directory::PersistedKernelInfo::HasEmptyDownloadProgress(
+    ModelType model_type) {
+  const sync_pb::DataTypeProgressMarker& progress_marker =
+      download_progress[model_type];
+  return progress_marker.token().empty();
+}
+
 Directory::SaveChangesSnapshot::SaveChangesSnapshot()
     : kernel_info_status(KERNEL_SHARE_INFO_INVALID) {
 }
@@ -738,6 +745,18 @@ bool Directory::PurgeEntriesWithTypeIn(ModelTypeSet disabled_types,
     {
       ScopedKernelLock lock(this);
 
+      bool found_progress = false;
+      for (ModelTypeSet::Iterator iter = disabled_types.First(); iter.Good();
+           iter.Inc()) {
+        if (!kernel_->persisted_info.HasEmptyDownloadProgress(iter.Get()))
+          found_progress = true;
+      }
+
+      // If none of the disabled types have progress markers, there's nothing to
+      // purge.
+      if (!found_progress)
+        return true;
+
       // We iterate in two passes to avoid a bug in STLport (which is used in
       // the Android build).  There are some versions of that library where a
       // hash_map's iterators can be invalidated when an item is erased from the

sync/syncable/directory.h

@@ -109,6 +109,9 @@ class SYNC_EXPORT Directory {
     // a full download of all objects of the model will be initiated.
     void ResetDownloadProgress(ModelType model_type);
 
+    // Whether a valid progress marker exists for |model_type|.
+    bool HasEmptyDownloadProgress(ModelType model_type);
+
     // Last sync timestamp fetched from the server.
     sync_pb::DataTypeProgressMarker download_progress[MODEL_TYPE_COUNT];
     // Sync-side transaction version per data type. Monotonically incremented

sync/syncable/directory_unittest.cc

@@ -232,6 +232,8 @@ TEST_F(SyncableDirectoryTest, TakeSnapshotGetsMetahandlesToPurge) {
   MetahandleSet expected_purges;
   MetahandleSet all_handles;
   {
+    dir()->SetDownloadProgress(BOOKMARKS, BuildProgress(BOOKMARKS));
+    dir()->SetDownloadProgress(PREFERENCES, BuildProgress(PREFERENCES));
     WriteTransaction trans(FROM_HERE, UNITTEST, dir().get());
     for (int i = 0; i < metas_to_create; i++) {
       MutableEntry e(&trans, CREATE, BOOKMARKS, trans.root_id(), "foo");

sync/syncable/syncable_unittest.cc

@@ -191,13 +191,6 @@ class OnDiskSyncableDirectoryTest : public SyncableDirectoryTest {
   base::FilePath file_path_;
 };
 
-sync_pb::DataTypeProgressMarker BuildProgress(ModelType type) {
-  sync_pb::DataTypeProgressMarker progress;
-  progress.set_token("token");
-  progress.set_data_type_id(GetSpecificsFieldNumberFromModelType(type));
-  return progress;
-}
-
 sync_pb::DataTypeContext BuildContext(ModelType type) {
   sync_pb::DataTypeContext context;
   context.set_context("context");
@@ -518,7 +511,8 @@ TEST_F(OnDiskSyncableDirectoryTest, TestSaveChangesFailure) {
 
 TEST_F(OnDiskSyncableDirectoryTest, TestSaveChangesFailureWithPurge) {
   int64 handle1 = 0;
-  // Set up an item using a regular, saveable directory.
+  // Set up an item and progress marker using a regular, saveable directory.
+  dir()->SetDownloadProgress(BOOKMARKS, BuildProgress(BOOKMARKS));
   {
     WriteTransaction trans(FROM_HERE, UNITTEST, dir().get());
 

sync/test/engine/test_syncable_utils.cc

@@ -90,5 +90,12 @@ void CreateTypeRoot(WriteTransaction* trans,
   node.PutSpecifics(specifics);
 }
 
+sync_pb::DataTypeProgressMarker BuildProgress(ModelType type) {
+  sync_pb::DataTypeProgressMarker progress;
+  progress.set_token("token");
+  progress.set_data_type_id(GetSpecificsFieldNumberFromModelType(type));
+  return progress;
+}
+
 }  // namespace syncable
 }  // namespace syncer

sync/test/engine/test_syncable_utils.h

@@ -11,6 +11,7 @@
 #include <string>
 
 #include "sync/internal_api/public/base/model_type.h"
+#include "sync/protocol/sync.pb.h"
 
 namespace syncer {
 namespace syncable {
@@ -42,6 +43,8 @@ void CreateTypeRoot(WriteTransaction* trans,
                     syncable::Directory *dir,
                     ModelType type);
 
+sync_pb::DataTypeProgressMarker BuildProgress(ModelType type);
+
 }  // namespace syncable
 }  // namespace syncer