Commit a1022547 authored by Mikel Astiz's avatar Mikel Astiz Committed by Commit Bot

Defer initial_sync_done for commit-only types

Instead of assuming IsTrackingMetadata() as soon as the model is ready
to sync, we now do it when:
a) The model is ready to sync.
b) Sync is known to be enabled (either because it just got enabled, or
   because we know from persisted data that is was previously enabled).

This makes commit-only types more similar to regular types, and allows
bridges to leverage the state. For example, while sync is enabled (i.e.
IsTrackingMetadata()), there should always be means to know which
account ID is syncing, which is now exposed in ModelTypeChangeProcessor.

From the bridge's perspective, the only transition from not tracking
metadata, and metadata being tracked (sync enabled), is MergeSyncData().

ModelReadyToSync() now presents the transition from unknown (i.e. unsure
whether sync is enabled or not until sync metadata is read from disk)
to true/false depending on the persisted value (true if sync was running
for the datatype in previous executions of the browser). Just like for
regular (non-commit-only) datatypes.

Bug: 830535
Change-Id: I573e27c2dcde1bd5835c30319065c178eb7b4679
Reviewed-on: https://chromium-review.googlesource.com/c/1136307Reviewed-by: default avatarOlivier Robin <olivierrobin@chromium.org>
Reviewed-by: default avatarMarkus Heintz <markusheintz@chromium.org>
Reviewed-by: default avatarvitaliii <vitaliii@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607207}
parent e9460f5d
......@@ -41,7 +41,7 @@ std::string GetAccountId() {
// impossible to discover otherwise.
return "user@gmail.com";
#else
return "gaia-id-user@gmail.com";
return "gaia_id_for_user@gmail.com";
#endif
}
......
......@@ -76,8 +76,12 @@ ConsentSyncBridgeImpl::CreateMetadataChangeList() {
base::Optional<ModelError> ConsentSyncBridgeImpl::MergeSyncData(
std::unique_ptr<MetadataChangeList> metadata_change_list,
EntityChangeList entity_data) {
NOTREACHED();
return {};
DCHECK(entity_data.empty());
DCHECK(change_processor()->IsTrackingMetadata());
DCHECK(!change_processor()->TrackedAccountId().empty());
ReadAllDataAndResubmit();
return ApplySyncChanges(std::move(metadata_change_list),
std::move(entity_data));
}
base::Optional<ModelError> ConsentSyncBridgeImpl::ApplySyncChanges(
......@@ -119,26 +123,12 @@ std::string ConsentSyncBridgeImpl::GetStorageKey(
return GetStorageKeyFromSpecifics(entity_data.specifics.user_consent());
}
void ConsentSyncBridgeImpl::OnSyncStarting(
const DataTypeActivationRequest& request) {
DCHECK(!request.authenticated_account_id.empty());
DCHECK(syncing_account_id_.empty());
syncing_account_id_ = request.authenticated_account_id;
if (store_ && change_processor()->IsTrackingMetadata()) {
ReadAllDataAndResubmit();
}
}
ModelTypeSyncBridge::StopSyncResponse
ConsentSyncBridgeImpl::ApplyStopSyncChanges(
std::unique_ptr<MetadataChangeList> delete_metadata_change_list) {
// Sync can only be stopped after initialization.
DCHECK(deferred_consents_while_initializing_.empty());
syncing_account_id_.clear();
if (delete_metadata_change_list) {
// Preserve all consents in the store, but delete their metadata, because it
// may become invalid when the sync is reenabled. It is important to report
......@@ -158,7 +148,7 @@ ConsentSyncBridgeImpl::ApplyStopSyncChanges(
}
void ConsentSyncBridgeImpl::ReadAllDataAndResubmit() {
DCHECK(!syncing_account_id_.empty());
DCHECK(!change_processor()->TrackedAccountId().empty());
DCHECK(change_processor()->IsTrackingMetadata());
DCHECK(store_);
store_->ReadAllData(
......@@ -169,7 +159,7 @@ void ConsentSyncBridgeImpl::ReadAllDataAndResubmit() {
void ConsentSyncBridgeImpl::OnReadAllDataToResubmit(
const base::Optional<ModelError>& error,
std::unique_ptr<RecordList> data_records) {
if (syncing_account_id_.empty()) {
if (change_processor()->TrackedAccountId().empty()) {
// Meanwhile the sync has been disabled. We will try next time.
return;
}
......@@ -180,21 +170,29 @@ void ConsentSyncBridgeImpl::OnReadAllDataToResubmit(
return;
}
std::unique_ptr<WriteBatch> batch = store_->CreateWriteBatch();
for (const Record& r : *data_records) {
auto specifics = std::make_unique<UserConsentSpecifics>();
if (specifics->ParseFromString(r.value) &&
specifics->account_id() == syncing_account_id_) {
RecordConsentImpl(std::move(specifics));
if (specifics->ParseFromString(r.value)) {
if (specifics->account_id() == change_processor()->TrackedAccountId()) {
change_processor()->Put(r.id, MoveToEntityData(std::move(specifics)),
batch->GetMetadataChangeList());
}
}
}
store_->CommitWriteBatch(std::move(batch),
base::BindOnce(&ConsentSyncBridgeImpl::OnCommit,
weak_ptr_factory_.GetWeakPtr()));
}
void ConsentSyncBridgeImpl::RecordConsent(
std::unique_ptr<UserConsentSpecifics> specifics) {
// TODO(vitaliii): Sanity-check specifics->account_id() against
// syncing_account_id_, maybe DCHECK.
// change_processor()->TrackedAccountId(), maybe DCHECK.
DCHECK(!specifics->account_id().empty());
if (change_processor()->IsTrackingMetadata()) {
if (store_) {
RecordConsentImpl(std::move(specifics));
return;
}
......@@ -207,17 +205,23 @@ std::string ConsentSyncBridgeImpl::GetStorageKeyFromSpecificsForTest(
return GetStorageKeyFromSpecifics(specifics);
}
std::unique_ptr<ModelTypeStore> ConsentSyncBridgeImpl::StealStoreForTest() {
return std::move(store_);
}
void ConsentSyncBridgeImpl::RecordConsentImpl(
std::unique_ptr<UserConsentSpecifics> specifics) {
DCHECK(store_);
DCHECK(change_processor()->IsTrackingMetadata());
std::string storage_key = GetStorageKeyFromSpecifics(*specifics);
std::unique_ptr<WriteBatch> batch = store_->CreateWriteBatch();
batch->WriteData(storage_key, specifics->SerializeAsString());
change_processor()->Put(storage_key, MoveToEntityData(std::move(specifics)),
batch->GetMetadataChangeList());
if (specifics->account_id() == change_processor()->TrackedAccountId()) {
change_processor()->Put(storage_key, MoveToEntityData(std::move(specifics)),
batch->GetMetadataChangeList());
}
store_->CommitWriteBatch(std::move(batch),
base::BindOnce(&ConsentSyncBridgeImpl::OnCommit,
weak_ptr_factory_.GetWeakPtr()));
......@@ -229,7 +233,6 @@ ConsentSyncBridgeImpl::GetControllerDelegate() {
}
void ConsentSyncBridgeImpl::ProcessQueuedEvents() {
DCHECK(change_processor()->IsTrackingMetadata());
for (std::unique_ptr<sync_pb::UserConsentSpecifics>& event :
deferred_consents_while_initializing_) {
RecordConsentImpl(std::move(event));
......@@ -260,8 +263,11 @@ void ConsentSyncBridgeImpl::OnReadAllMetadata(
change_processor()->ReportError(*error);
} else {
change_processor()->ModelReadyToSync(std::move(metadata_batch));
DCHECK(change_processor()->IsTrackingMetadata());
if (!syncing_account_id_.empty()) {
if (!change_processor()->TrackedAccountId().empty()) {
// We resubmit all data in case the client crashed immediately after
// MergeSyncData(), where submissions are supposed to happen and
// metadata populated. This would be simpler if MergeSyncData() were
// asynchronous.
ReadAllDataAndResubmit();
}
ProcessQueuedEvents();
......
......@@ -30,7 +30,6 @@ class ConsentSyncBridgeImpl : public ConsentSyncBridge,
~ConsentSyncBridgeImpl() override;
// ModelTypeSyncBridge implementation.
void OnSyncStarting(const DataTypeActivationRequest& request) override;
std::unique_ptr<MetadataChangeList> CreateMetadataChangeList() override;
base::Optional<ModelError> MergeSyncData(
std::unique_ptr<MetadataChangeList> metadata_change_list,
......@@ -53,6 +52,7 @@ class ConsentSyncBridgeImpl : public ConsentSyncBridge,
static std::string GetStorageKeyFromSpecificsForTest(
const sync_pb::UserConsentSpecifics& specifics);
std::unique_ptr<ModelTypeStore> StealStoreForTest();
private:
void RecordConsentImpl(
......@@ -91,9 +91,6 @@ class ConsentSyncBridgeImpl : public ConsentSyncBridge,
std::vector<std::unique_ptr<sync_pb::UserConsentSpecifics>>
deferred_consents_while_initializing_;
// Empty if sync not running.
std::string syncing_account_id_;
base::WeakPtrFactory<ConsentSyncBridgeImpl> weak_ptr_factory_;
DISALLOW_COPY_AND_ASSIGN(ConsentSyncBridgeImpl);
......
......@@ -96,8 +96,7 @@ class ReadingListStoreTest : public testing::Test,
.WillByDefault(testing::Return(true));
ClearState();
reading_list_store_ = std::make_unique<ReadingListStore>(
base::BindOnce(&syncer::ModelTypeStoreTestUtil::MoveStoreToCallback,
std::move(store_)),
syncer::ModelTypeStoreTestUtil::MoveStoreToFactory(std::move(store_)),
processor_.CreateForwardingProcessor());
model_ = std::make_unique<ReadingListModelImpl>(nullptr, nullptr, &clock_);
reading_list_store_->SetReadingListModel(model_.get(), this, &clock_);
......
......@@ -198,8 +198,7 @@ class DeviceInfoSyncBridgeTest : public testing::Test,
ASSERT_TRUE(store_);
bridge_ = std::make_unique<DeviceInfoSyncBridge>(
provider_.get(),
base::BindOnce(&ModelTypeStoreTestUtil::MoveStoreToCallback,
std::move(store_)),
ModelTypeStoreTestUtil::MoveStoreToFactory(std::move(store_)),
mock_processor_.CreateForwardingProcessor());
bridge_->AddObserver(this);
}
......
......@@ -84,12 +84,14 @@ ModelTypeStoreTestUtil::FactoryForInMemoryStoreForTest() {
}
// static
void ModelTypeStoreTestUtil::MoveStoreToCallback(
std::unique_ptr<ModelTypeStore> store,
ModelType type,
ModelTypeStore::InitCallback callback) {
ASSERT_TRUE(store);
std::move(callback).Run(/*error=*/base::nullopt, std::move(store));
OnceModelTypeStoreFactory ModelTypeStoreTestUtil::MoveStoreToFactory(
std::unique_ptr<ModelTypeStore> store) {
return base::BindOnce(
[](std::unique_ptr<ModelTypeStore> store, ModelType type,
ModelTypeStore::InitCallback callback) {
std::move(callback).Run(/*error=*/base::nullopt, std::move(store));
},
std::move(store));
}
// static
......
......@@ -22,11 +22,10 @@ class ModelTypeStoreTestUtil {
// Creates a factory callback to synchronously return in memory stores.
static RepeatingModelTypeStoreFactory FactoryForInMemoryStoreForTest();
// Can be curried with an owned store object to allow passing an already
// created store to a service constructor in a unit test.
static void MoveStoreToCallback(std::unique_ptr<ModelTypeStore> store,
ModelType type,
ModelTypeStore::InitCallback callback);
// Returns a once-factory that returns an already created store to a service
// constructor in a unit test.
static OnceModelTypeStoreFactory MoveStoreToFactory(
std::unique_ptr<ModelTypeStore> store);
// Returns a callback that constructs a store that forwards all calls to
// |target|. |*target| must outlive the returned factory as well any store
......
......@@ -133,6 +133,9 @@ void ClientTagBasedModelTypeProcessor::ModelReadyToSync(
}
model_type_state_ = batch->GetModelTypeState();
} else {
// In older versions of the binary, commit-only types did not persist
// initial_sync_done(). So this branch can be exercised for commit-only
// types exactly once as an upgrade flow.
// TODO(crbug.com/872360): This DCHECK can currently trigger if the user's
// persisted Sync metadata is in an inconsistent state.
DCHECK(commit_only_ || batch->TakeAllMetadata().empty())
......@@ -140,9 +143,6 @@ void ClientTagBasedModelTypeProcessor::ModelReadyToSync(
// First time syncing; initialize metadata.
model_type_state_.mutable_progress_marker()->set_data_type_id(
GetSpecificsFieldNumberFromModelType(type_));
// For commit-only types, no updates are expected and hence we can consider
// initial_sync_done().
model_type_state_.set_initial_sync_done(commit_only_);
}
ConnectIfReady();
......@@ -185,9 +185,6 @@ void ClientTagBasedModelTypeProcessor::ConnectIfReady() {
// The model is still ready to sync (with the same |bridge_|) - replay
// the initialization.
model_ready_to_sync_ = true;
// For commit-only types, no updates are expected and hence we can
// consider initial_sync_done().
model_type_state_.set_initial_sync_done(commit_only_);
// Notify the bridge sync is starting to simulate an enable event.
bridge_->OnSyncStarting(activation_request_);
break;
......@@ -203,10 +200,19 @@ void ClientTagBasedModelTypeProcessor::ConnectIfReady() {
}
}
// Cache GUID verification guarantees the user is the same.
// Cache GUID verification earlier above guarantees the user is the same.
model_type_state_.set_authenticated_account_id(
activation_request_.authenticated_account_id);
// For commit-only types, no updates are expected and hence we can consider
// initial_sync_done(), reflecting that sync is enabled.
if (commit_only_ && !model_type_state_.initial_sync_done()) {
sync_pb::ModelTypeState model_type_state = model_type_state_;
model_type_state.set_initial_sync_done(true);
OnFullUpdateReceived(model_type_state, UpdateResponseDataList());
DCHECK(IsTrackingMetadata());
}
auto activation_response = std::make_unique<DataTypeActivationResponse>();
activation_response->model_type_state = model_type_state_;
activation_response->type_processor =
......
......@@ -2218,6 +2218,32 @@ TEST_F(CommitOnlyClientTagBasedModelTypeProcessorTest,
EXPECT_EQ("PersistedAccountId", type_processor()->TrackedAccountId());
}
TEST_F(CommitOnlyClientTagBasedModelTypeProcessorTest,
ShouldCallMergeWhenSyncEnabled) {
ModelReadyToSync();
ASSERT_EQ("", type_processor()->TrackedAccountId());
ASSERT_EQ(0, bridge()->merge_call_count());
OnSyncStarting();
EXPECT_EQ(1, bridge()->merge_call_count());
}
TEST_F(CommitOnlyClientTagBasedModelTypeProcessorTest,
ShouldNotCallMergeAfterRestart) {
std::unique_ptr<MetadataBatch> metadata_batch = db().CreateMetadataBatch();
sync_pb::ModelTypeState model_type_state(metadata_batch->GetModelTypeState());
model_type_state.set_initial_sync_done(true);
model_type_state.set_authenticated_account_id("PersistedAccountId");
metadata_batch->SetModelTypeState(model_type_state);
type_processor()->ModelReadyToSync(std::move(metadata_batch));
// Even prior to starting sync, the account ID should already be tracked.
ASSERT_EQ("PersistedAccountId", type_processor()->TrackedAccountId());
// When sync gets started, MergeSyncData() should not be called.
OnSyncStarting("PersistedAccountId");
ASSERT_EQ(0, bridge()->merge_call_count());
}
// Test that commit only types are deleted after commit response.
TEST_F(CommitOnlyClientTagBasedModelTypeProcessorTest,
ShouldCommitAndDeleteWhenAcked) {
......
......@@ -81,6 +81,8 @@ class UserEventServiceImplTest : public testing::Test {
sync_service_.SetPreferredDataTypes({HISTORY_DELETE_DIRECTIVES});
ON_CALL(mock_processor_, IsTrackingMetadata())
.WillByDefault(testing::Return(true));
ON_CALL(mock_processor_, TrackedAccountId())
.WillByDefault(testing::Return("account_id"));
}
std::unique_ptr<UserEventSyncBridge> MakeBridge() {
......
......@@ -90,8 +90,12 @@ UserEventSyncBridge::CreateMetadataChangeList() {
base::Optional<ModelError> UserEventSyncBridge::MergeSyncData(
std::unique_ptr<MetadataChangeList> metadata_change_list,
EntityChangeList entity_data) {
NOTREACHED();
return {};
DCHECK(entity_data.empty());
DCHECK(change_processor()->IsTrackingMetadata());
DCHECK(!change_processor()->TrackedAccountId().empty());
ReadAllDataAndResubmit();
return ApplySyncChanges(std::move(metadata_change_list),
std::move(entity_data));
}
base::Optional<ModelError> UserEventSyncBridge::ApplySyncChanges(
......@@ -145,25 +149,11 @@ std::string UserEventSyncBridge::GetStorageKey(const EntityData& entity_data) {
return GetStorageKeyFromSpecifics(entity_data.specifics.user_event());
}
void UserEventSyncBridge::OnSyncStarting(
const DataTypeActivationRequest& request) {
DCHECK(!request.authenticated_account_id.empty());
DCHECK(syncing_account_id_.empty());
syncing_account_id_ = request.authenticated_account_id;
if (store_ && change_processor()->IsTrackingMetadata()) {
ReadAllDataAndResubmit();
}
}
ModelTypeSyncBridge::StopSyncResponse UserEventSyncBridge::ApplyStopSyncChanges(
std::unique_ptr<MetadataChangeList> delete_metadata_change_list) {
// Sync can only be stopped after initialization.
DCHECK(deferred_user_events_while_initializing_.empty());
syncing_account_id_.clear();
if (delete_metadata_change_list) {
// Delete everything except user consents. With DICE the signout may happen
// frequently. It is important to report all user consents, thus, they are
......@@ -207,7 +197,6 @@ void UserEventSyncBridge::OnReadAllDataToDelete(
}
void UserEventSyncBridge::ReadAllDataAndResubmit() {
DCHECK(!syncing_account_id_.empty());
DCHECK(change_processor()->IsTrackingMetadata());
DCHECK(store_);
store_->ReadAllData(
......@@ -218,7 +207,7 @@ void UserEventSyncBridge::ReadAllDataAndResubmit() {
void UserEventSyncBridge::OnReadAllDataToResubmit(
const base::Optional<ModelError>& error,
std::unique_ptr<RecordList> data_records) {
if (syncing_account_id_.empty()) {
if (change_processor()->TrackedAccountId().empty()) {
// Meanwhile the sync has been disabled. We will try next time.
return;
}
......@@ -229,24 +218,32 @@ void UserEventSyncBridge::OnReadAllDataToResubmit(
return;
}
std::unique_ptr<WriteBatch> batch = store_->CreateWriteBatch();
for (const Record& r : *data_records) {
auto specifics = std::make_unique<UserEventSpecifics>();
if (specifics->ParseFromString(r.value) &&
specifics->event_case() ==
UserEventSpecifics::EventCase::kUserConsent &&
specifics->user_consent().account_id() == syncing_account_id_) {
RecordUserEventImpl(std::move(specifics));
specifics->user_consent().account_id() ==
change_processor()->TrackedAccountId()) {
change_processor()->Put(r.id, MoveToEntityData(std::move(specifics)),
batch->GetMetadataChangeList());
}
}
store_->CommitWriteBatch(std::move(batch),
base::BindOnce(&UserEventSyncBridge::OnCommit,
weak_ptr_factory_.GetWeakPtr()));
}
void UserEventSyncBridge::RecordUserEvent(
std::unique_ptr<UserEventSpecifics> specifics) {
// TODO(vitaliii): Sanity-check specifics->user_consent().account_id() against
// syncing_account_id_, maybe DCHECK.
// change_processor()->TrackedAccountId(), maybe DCHECK.
DCHECK(!specifics->has_user_consent() ||
!specifics->user_consent().account_id().empty());
if (change_processor()->IsTrackingMetadata()) {
if (store_) {
RecordUserEventImpl(std::move(specifics));
return;
}
......@@ -260,10 +257,18 @@ std::string UserEventSyncBridge::GetStorageKeyFromSpecificsForTest(
return GetStorageKeyFromSpecifics(specifics);
}
std::unique_ptr<ModelTypeStore> UserEventSyncBridge::StealStoreForTest() {
return std::move(store_);
}
void UserEventSyncBridge::RecordUserEventImpl(
std::unique_ptr<UserEventSpecifics> specifics) {
DCHECK(store_);
DCHECK(change_processor()->IsTrackingMetadata());
if (specifics->event_case() != UserEventSpecifics::EventCase::kUserConsent &&
!change_processor()->IsTrackingMetadata()) {
return;
}
std::string storage_key = GetStorageKeyFromSpecifics(*specifics);
// There are two scenarios we need to guard against here. First, the given
......@@ -286,15 +291,22 @@ void UserEventSyncBridge::RecordUserEventImpl(
std::unique_ptr<WriteBatch> batch = store_->CreateWriteBatch();
batch->WriteData(storage_key, specifics->SerializeAsString());
change_processor()->Put(storage_key, MoveToEntityData(std::move(specifics)),
batch->GetMetadataChangeList());
// For consents, only Put() if account ID matches, and unconditionally for
// other event types.
if (specifics->event_case() != UserEventSpecifics::EventCase::kUserConsent ||
specifics->user_consent().account_id() ==
change_processor()->TrackedAccountId()) {
DCHECK(change_processor()->IsTrackingMetadata());
change_processor()->Put(storage_key, MoveToEntityData(std::move(specifics)),
batch->GetMetadataChangeList());
}
store_->CommitWriteBatch(std::move(batch),
base::BindOnce(&UserEventSyncBridge::OnCommit,
weak_ptr_factory_.GetWeakPtr()));
}
void UserEventSyncBridge::ProcessQueuedEvents() {
DCHECK(change_processor()->IsTrackingMetadata());
for (std::unique_ptr<sync_pb::UserEventSpecifics>& event :
deferred_user_events_while_initializing_) {
RecordUserEventImpl(std::move(event));
......@@ -324,8 +336,7 @@ void UserEventSyncBridge::OnReadAllMetadata(
change_processor()->ReportError(*error);
} else {
change_processor()->ModelReadyToSync(std::move(metadata_batch));
DCHECK(change_processor()->IsTrackingMetadata());
if (!syncing_account_id_.empty()) {
if (!change_processor()->TrackedAccountId().empty()) {
ReadAllDataAndResubmit();
}
ProcessQueuedEvents();
......
......@@ -31,7 +31,6 @@ class UserEventSyncBridge : public ModelTypeSyncBridge {
~UserEventSyncBridge() override;
// ModelTypeSyncBridge implementation.
void OnSyncStarting(const DataTypeActivationRequest& request) override;
std::unique_ptr<MetadataChangeList> CreateMetadataChangeList() override;
base::Optional<ModelError> MergeSyncData(
std::unique_ptr<MetadataChangeList> metadata_change_list,
......@@ -50,6 +49,7 @@ class UserEventSyncBridge : public ModelTypeSyncBridge {
static std::string GetStorageKeyFromSpecificsForTest(
const sync_pb::UserEventSpecifics& specifics);
std::unique_ptr<ModelTypeStore> StealStoreForTest();
private:
void RecordUserEventImpl(
......@@ -99,9 +99,6 @@ class UserEventSyncBridge : public ModelTypeSyncBridge {
GlobalIdMapper* global_id_mapper_;
// Empty if sync not running.
std::string syncing_account_id_;
base::WeakPtrFactory<UserEventSyncBridge> weak_ptr_factory_;
DISALLOW_COPY_AND_ASSIGN(UserEventSyncBridge);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment