summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--chrome/browser/sync/glue/sync_backend_host_impl.cc8
-rw-r--r--chrome/browser/sync/glue/sync_backend_host_impl_unittest.cc75
-rw-r--r--sync/engine/syncer_unittest.cc8
-rw-r--r--sync/syncable/directory.cc19
-rw-r--r--sync/syncable/directory.h3
-rw-r--r--sync/syncable/directory_unittest.cc2
-rw-r--r--sync/syncable/syncable_unittest.cc10
-rw-r--r--sync/test/engine/test_syncable_utils.cc7
-rw-r--r--sync/test/engine/test_syncable_utils.h3
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
generated by cgit v1.1 at 2024-09-09 21:13:33 +0000