[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

9 files changed, 108 insertions, 27 deletions

diff --git a/chrome/browser/sync/glue/sync_backend_host_impl.cc b/chrome/browser/sync/glue/sync_backend_host_impl.cc
index 87d294e..1f500ee 100644
--- a/chrome/browser/sync/glue/sync_backend_host_impl.cc
+++ b/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);
diff --git a/chrome/browser/sync/glue/sync_backend_host_impl_unittest.cc b/chrome/browser/sync/glue/sync_backend_host_impl_unittest.cc
index 8bd0e7c..a41e111 100644
--- a/chrome/browser/sync/glue/sync_backend_host_impl_unittest.cc
+++ b/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
diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc
index 6f891e8d..160ed9c 100644
--- a/sync/engine/syncer_unittest.cc
+++ b/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");
diff --git a/sync/syncable/directory.cc b/sync/syncable/directory.cc
index be489b2..1af5474 100644
--- a/sync/syncable/directory.cc
+++ b/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
diff --git a/sync/syncable/directory.h b/sync/syncable/directory.h
index 2830eec..01baac7 100644
--- a/sync/syncable/directory.h
+++ b/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
diff --git a/sync/syncable/directory_unittest.cc b/sync/syncable/directory_unittest.cc
index efc97e6..fabdc47 100644
--- a/sync/syncable/directory_unittest.cc
+++ b/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");
diff --git a/sync/syncable/syncable_unittest.cc b/sync/syncable/syncable_unittest.cc
index ef0c9dd..7e1aad5 100644
--- a/sync/syncable/syncable_unittest.cc
+++ b/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());
 
diff --git a/sync/test/engine/test_syncable_utils.cc b/sync/test/engine/test_syncable_utils.cc
index 8eb247f..aeb4c66 100644
--- a/sync/test/engine/test_syncable_utils.cc
+++ b/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
diff --git a/sync/test/engine/test_syncable_utils.h b/sync/test/engine/test_syncable_utils.h
index 7f162f7..5975e12 100644
--- a/sync/test/engine/test_syncable_utils.h
+++ b/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