Untrack/drop delete directives only if committed

syncer::WriteNode::Drop() guarantees that if the entity hasn't been
committed to the server, the call gets ignored (i.e. the entity isn't
actually dropped).

Let's mimic the same behavior in the pseudo-USS codepath.
Unfortunately, in order to do so, interface ModelTypeChangeProcessor
needs to be extended.

A TODO is introduced to improve this implementation in the future, when
pseudo-USS is fully launched. This logic could be simplified a lot as
well as the overall complexity of the SyncableService (logic
implemented in DeleteDirectiveHandler) if everything was moved to
history's backend thread.

Bug: 870624
Change-Id: Ic064d809371c49f4281d986b4b30bdb3fa37844a
Reviewed-on: https://chromium-review.googlesource.com/c/1326001
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606452}

components/sync/model/fake_model_type_change_processor.cc

@@ -43,6 +43,11 @@ void FakeModelTypeChangeProcessor::UntrackEntity(
 void FakeModelTypeChangeProcessor::UntrackEntityForStorageKey(
     const std::string& storage_key) {}
 
+bool FakeModelTypeChangeProcessor::IsEntityUnsynced(
+    const std::string& storage_key) {
+  return false;
+}
+
 void FakeModelTypeChangeProcessor::OnModelStarting(
     ModelTypeSyncBridge* bridge) {}
 

components/sync/model/fake_model_type_change_processor.h

@@ -37,6 +37,7 @@ class FakeModelTypeChangeProcessor : public ModelTypeChangeProcessor {
                         MetadataChangeList* metadata_change_list) override;
   void UntrackEntity(const EntityData& entity_data) override;
   void UntrackEntityForStorageKey(const std::string& storage_key) override;
+  bool IsEntityUnsynced(const std::string& storage_key) override;
   void OnModelStarting(ModelTypeSyncBridge* bridge) override;
   void ModelReadyToSync(std::unique_ptr<MetadataBatch> batch) override;
   bool IsTrackingMetadata() override;

components/sync/model/mock_model_type_change_processor.cc

@@ -48,6 +48,10 @@ class ForwardingModelTypeChangeProcessor : public ModelTypeChangeProcessor {
     other_->UntrackEntityForStorageKey(storage_key);
   }
 
+  bool IsEntityUnsynced(const std::string& storage_key) override {
+    return other_->IsEntityUnsynced(storage_key);
+  }
+
   void OnModelStarting(ModelTypeSyncBridge* bridge) override {
     other_->OnModelStarting(bridge);
   }
@@ -110,6 +114,9 @@ void MockModelTypeChangeProcessor::DelegateCallsByDefaultTo(
   ON_CALL(*this, UntrackEntityForStorageKey(_))
       .WillByDefault(Invoke(
           delegate, &ModelTypeChangeProcessor::UntrackEntityForStorageKey));
+  ON_CALL(*this, IsEntityUnsynced(_))
+      .WillByDefault(
+          Invoke(delegate, &ModelTypeChangeProcessor::IsEntityUnsynced));
   ON_CALL(*this, OnModelStarting(_))
       .WillByDefault(
           Invoke(delegate, &ModelTypeChangeProcessor::OnModelStarting));

components/sync/model/mock_model_type_change_processor.h

@@ -33,6 +33,7 @@ class MockModelTypeChangeProcessor : public ModelTypeChangeProcessor {
   MOCK_METHOD1(UntrackEntity, void(const EntityData& entity_data));
   MOCK_METHOD1(UntrackEntityForStorageKey,
                void(const std::string& storage_key));
+  MOCK_METHOD1(IsEntityUnsynced, bool(const std::string& storage_key));
   MOCK_METHOD1(OnModelStarting, void(ModelTypeSyncBridge* bridge));
   MOCK_METHOD1(ModelReadyToSync, void(std::unique_ptr<MetadataBatch> batch));
   MOCK_METHOD0(IsTrackingMetadata, bool());

components/sync/model/model_type_change_processor.h

@@ -66,6 +66,14 @@ class ModelTypeChangeProcessor {
   // |storage_key| is unknown.
   virtual void UntrackEntityForStorageKey(const std::string& storage_key) = 0;
 
+  // Returns true if a tracked entity has local changes. A commit may or may not
+  // be in progress at this time.
+  // TODO(mastiz): The only user of this is HISTORY_DELETE_DIRECTIVES which
+  // needs it for a rather questionable reason. Revisit this, for example by
+  // moving the SyncableService to history's backend thread, and leveraging
+  // USS's ability to delete local data upcon commit completion.
+  virtual bool IsEntityUnsynced(const std::string& storage_key) = 0;
+
   // Pass the pointer to the processor so that the processor can notify the
   // bridge of various events; |bridge| must not be nullptr and must outlive
   // this object.

components/sync/model_impl/client_tag_based_model_type_processor.cc

@@ -502,6 +502,16 @@ void ClientTagBasedModelTypeProcessor::UntrackEntityForStorageKey(
   storage_key_to_tag_hash_.erase(iter);
 }
 
+bool ClientTagBasedModelTypeProcessor::IsEntityUnsynced(
+    const std::string& storage_key) {
+  ProcessorEntityTracker* entity = GetEntityForStorageKey(storage_key);
+  if (entity == nullptr) {
+    return false;
+  }
+
+  return entity->IsUnsynced();
+}
+
 void ClientTagBasedModelTypeProcessor::NudgeForCommitIfNeeded() {
   // Don't bother sending anything if there's no one to send to.
   if (!IsConnected())

components/sync/model_impl/client_tag_based_model_type_processor.h

@@ -68,6 +68,7 @@ class ClientTagBasedModelTypeProcessor : public ModelTypeProcessor,
                         MetadataChangeList* metadata_change_list) override;
   void UntrackEntity(const EntityData& entity_data) override;
   void UntrackEntityForStorageKey(const std::string& storage_key) override;
+  bool IsEntityUnsynced(const std::string& storage_key) override;
   void OnModelStarting(ModelTypeSyncBridge* bridge) override;
   void ModelReadyToSync(std::unique_ptr<MetadataBatch> batch) override;
   bool IsTrackingMetadata() override;

components/sync/model_impl/syncable_service_based_bridge.cc

@@ -86,9 +86,9 @@ base::Optional<ModelError> ConvertToModelError(const SyncError& sync_error) {
 
 // Object to propagate local changes to the bridge, which will ultimately
 // propagate them to the server.
-class ChangeProcessorImpl : public SyncChangeProcessor {
+class LocalChangeProcessor : public SyncChangeProcessor {
  public:
-  ChangeProcessorImpl(
+  LocalChangeProcessor(
       ModelType type,
       const base::RepeatingCallback<void(const base::Optional<ModelError>&)>&
           error_callback,
@@ -107,7 +107,7 @@ class ChangeProcessorImpl : public SyncChangeProcessor {
     DCHECK(other);
   }
 
-  ~ChangeProcessorImpl() override {}
+  ~LocalChangeProcessor() override {}
 
   SyncError ProcessSyncChanges(const base::Location& from_here,
                                const SyncChangeList& change_list) override {
@@ -189,16 +189,21 @@ class ChangeProcessorImpl : public SyncChangeProcessor {
           DCHECK(!storage_key.empty())
               << " from " << change.location().ToString();
 
-          in_memory_store_->erase(storage_key);
-          batch->DeleteData(storage_key);
-
           if (IsActOnceDataType(type_)) {
+            if (other_->IsEntityUnsynced(storage_key)) {
+              // Ignore the local deletion if the entity hasn't been committed
+              // yet, similarly to how WriteNode::Drop() does it.
+              continue;
+            }
             batch->GetMetadataChangeList()->ClearMetadata(storage_key);
             other_->UntrackEntityForStorageKey(storage_key);
           } else {
             other_->Delete(storage_key, batch->GetMetadataChangeList());
           }
 
+          in_memory_store_->erase(storage_key);
+          batch->DeleteData(storage_key);
+
           break;
         }
       }
@@ -250,7 +255,7 @@ class ChangeProcessorImpl : public SyncChangeProcessor {
   ModelTypeChangeProcessor* const other_;
   SEQUENCE_CHECKER(sequence_checker_);
 
-  DISALLOW_COPY_AND_ASSIGN(ChangeProcessorImpl);
+  DISALLOW_COPY_AND_ASSIGN(LocalChangeProcessor);
 };
 
 class SyncErrorFactoryImpl : public SyncErrorFactory {
@@ -296,8 +301,8 @@ SyncableServiceBasedBridge::SyncableServiceBasedBridge(
 
 SyncableServiceBasedBridge::~SyncableServiceBasedBridge() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  // Stop the syncable service to make sure instances of ChangeProcessorImpl are
-  // not continued to be used.
+  // Stop the syncable service to make sure instances of LocalChangeProcessor
+  // are not continued to be used.
   if (syncable_service_started_) {
     syncable_service_->StopSyncing(type_);
   }
@@ -358,14 +363,14 @@ base::Optional<ModelError> SyncableServiceBasedBridge::MergeSyncData(
   auto error_callback =
       base::BindRepeating(&SyncableServiceBasedBridge::ReportErrorIfSet,
                           weak_ptr_factory_.GetWeakPtr());
-  auto processor_impl = std::make_unique<ChangeProcessorImpl>(
+  auto local_change_processor = std::make_unique<LocalChangeProcessor>(
       type_, error_callback, store_.get(), &in_memory_store_, cryptographer_,
       change_processor());
 
   const base::Optional<ModelError> merge_error = ConvertToModelError(
       syncable_service_
           ->MergeDataAndStartSyncing(
-              type_, initial_sync_data, std::move(processor_impl),
+              type_, initial_sync_data, std::move(local_change_processor),
               std::make_unique<SyncErrorFactoryImpl>(type_))
           .error());
 
@@ -508,6 +513,18 @@ SyncableServiceBasedBridge::ApplySyncChangesWithNewEncryptionRequirements(
                                    std::move(entity_changes), std::move(batch));
 }
 
+// static
+std::unique_ptr<SyncChangeProcessor>
+SyncableServiceBasedBridge::CreateLocalChangeProcessorForTesting(
+    ModelType type,
+    ModelTypeStore* store,
+    std::map<std::string, sync_pb::PersistedEntityData>* in_memory_store,
+    ModelTypeChangeProcessor* other) {
+  return std::make_unique<LocalChangeProcessor>(
+      type, /*error_callback=*/base::DoNothing(), store, in_memory_store,
+      /*cryptographer=*/nullptr, other);
+}
+
 void SyncableServiceBasedBridge::OnStoreCreated(
     const base::Optional<ModelError>& error,
     std::unique_ptr<ModelTypeStore> store) {
@@ -618,14 +635,14 @@ void SyncableServiceBasedBridge::MaybeStartSyncableService() {
   auto error_callback =
       base::BindRepeating(&SyncableServiceBasedBridge::ReportErrorIfSet,
                           weak_ptr_factory_.GetWeakPtr());
-  auto processor_impl = std::make_unique<ChangeProcessorImpl>(
+  auto local_change_processor = std::make_unique<LocalChangeProcessor>(
       type_, error_callback, store_.get(), &in_memory_store_, cryptographer_,
       change_processor());
 
   const base::Optional<ModelError> merge_error = ConvertToModelError(
       syncable_service_
           ->MergeDataAndStartSyncing(
-              type_, initial_sync_data, std::move(processor_impl),
+              type_, initial_sync_data, std::move(local_change_processor),
               std::make_unique<SyncErrorFactoryImpl>(type_))
           .error());
 

components/sync/model_impl/syncable_service_based_bridge.h

@@ -93,6 +93,14 @@ class SyncableServiceBasedBridge : public ModelTypeSyncBridge {
       std::unique_ptr<MetadataChangeList> metadata_change_list,
       EntityChangeList entity_changes) override;
 
+  // For testing.
+  static std::unique_ptr<SyncChangeProcessor>
+  CreateLocalChangeProcessorForTesting(
+      ModelType type,
+      ModelTypeStore* store,
+      std::map<std::string, sync_pb::PersistedEntityData>* in_memory_store,
+      ModelTypeChangeProcessor* other);
+
  private:
   void OnStoreCreated(const base::Optional<ModelError>& error,
                       std::unique_ptr<ModelTypeStore> store);

components/sync/model_impl/syncable_service_based_bridge_unittest.cc

@@ -22,6 +22,7 @@
 #include "components/sync/model/sync_merge_result.h"
 #include "components/sync/model/syncable_service.h"
 #include "components/sync/model_impl/client_tag_based_model_type_processor.h"
+#include "components/sync/protocol/persisted_entity_data.pb.h"
 #include "components/sync/protocol/sync.pb.h"
 #include "components/sync/test/engine/mock_model_type_worker.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -690,5 +691,77 @@ TEST_F(SyncableServiceBasedBridgeTest, ShouldReencryptDataUponKeyChange) {
   StartSyncing();
 }
 
+TEST(SyncableServiceBasedBridgeLocalChangeProcessorTest,
+     ShouldDropIfCommitted) {
+  const std::string kClientTagHash = "clienttaghash1";
+
+  base::MessageLoop message_loop;
+  std::unique_ptr<ModelTypeStore> store =
+      ModelTypeStoreTestUtil::CreateInMemoryStoreForTest();
+  std::map<std::string, sync_pb::PersistedEntityData> in_memory_store;
+  testing::NiceMock<MockModelTypeChangeProcessor> mock_processor;
+
+  in_memory_store[kClientTagHash] = sync_pb::PersistedEntityData();
+
+  std::unique_ptr<SyncChangeProcessor> sync_change_processor =
+      SyncableServiceBasedBridge::CreateLocalChangeProcessorForTesting(
+          HISTORY_DELETE_DIRECTIVES, store.get(), &in_memory_store,
+          &mock_processor);
+
+  EXPECT_CALL(mock_processor, IsEntityUnsynced(kClientTagHash))
+      .WillOnce(Return(false));
+  EXPECT_CALL(mock_processor, UntrackEntityForStorageKey(kClientTagHash));
+
+  sync_pb::EntitySpecifics specifics;
+  specifics.mutable_history_delete_directive();
+
+  SyncChangeList change_list;
+  change_list.push_back(SyncChange(
+      FROM_HERE, SyncChange::ACTION_DELETE,
+      SyncData::CreateRemoteData(/*id=*/1, specifics,
+                                 /*last_modified_time=*/base::Time::Now(),
+                                 /*client_tag_hash=*/kClientTagHash)));
+
+  sync_change_processor->ProcessSyncChanges(FROM_HERE, change_list);
+
+  EXPECT_EQ(0U, in_memory_store.count(kClientTagHash));
+}
+
+TEST(SyncableServiceBasedBridgeLocalChangeProcessorTest,
+     ShouldNotDropIfUnsynced) {
+  const std::string kClientTagHash = "clienttaghash1";
+
+  base::MessageLoop message_loop;
+  std::unique_ptr<ModelTypeStore> store =
+      ModelTypeStoreTestUtil::CreateInMemoryStoreForTest();
+  std::map<std::string, sync_pb::PersistedEntityData> in_memory_store;
+  testing::NiceMock<MockModelTypeChangeProcessor> mock_processor;
+
+  in_memory_store[kClientTagHash] = sync_pb::PersistedEntityData();
+
+  std::unique_ptr<SyncChangeProcessor> sync_change_processor =
+      SyncableServiceBasedBridge::CreateLocalChangeProcessorForTesting(
+          HISTORY_DELETE_DIRECTIVES, store.get(), &in_memory_store,
+          &mock_processor);
+
+  EXPECT_CALL(mock_processor, IsEntityUnsynced(kClientTagHash))
+      .WillOnce(Return(true));
+  EXPECT_CALL(mock_processor, UntrackEntityForStorageKey(_)).Times(0);
+
+  sync_pb::EntitySpecifics specifics;
+  specifics.mutable_history_delete_directive();
+
+  SyncChangeList change_list;
+  change_list.push_back(SyncChange(
+      FROM_HERE, SyncChange::ACTION_DELETE,
+      SyncData::CreateRemoteData(/*id=*/1, specifics,
+                                 /*last_modified_time=*/base::Time::Now(),
+                                 /*client_tag_hash=*/kClientTagHash)));
+
+  sync_change_processor->ProcessSyncChanges(FROM_HERE, change_list);
+
+  EXPECT_EQ(1U, in_memory_store.count(kClientTagHash));
+}
+
 }  // namespace
 }  // namespace syncer