[Sync] Preserve UserConsent when Sync is disabled.

Previously, all user events were deleted when Sync was disabled. After
this CL, user consent events are preserved in persistent store. This CL
does *not* include their garbage collection and attempting to send them again.


Bug: 781765
Change-Id: I6fdb7488da81324d782f28745299301b551a712c
Reviewed-on: https://chromium-review.googlesource.com/986514
Commit-Queue: vitaliii <vitaliii@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548767}

components/sync/user_events/user_event_sync_bridge.cc

@@ -152,8 +152,41 @@ void UserEventSyncBridge::ApplyDisableSyncChanges(
     std::unique_ptr<MetadataChangeList> delete_metadata_change_list) {
   // Sync can only be disabled after initialization.
   DCHECK(deferred_user_events_while_initializing_.empty());
-  // No data should be retained through sign out.
-  store_->DeleteAllDataAndMetadata(base::DoNothing());
+  // Delete everything except user consents. With DICE the signout may happen
+  // frequently. It is important to report all user consents, thus, they are
+  // persisted for some time even after signout.
+  store_->ReadAllData(base::BindOnce(
+      &UserEventSyncBridge::OnReadAllDataToDelete, base::AsWeakPtr(this),
+      std::move(delete_metadata_change_list)));
+}
+
+void UserEventSyncBridge::OnReadAllDataToDelete(
+    std::unique_ptr<MetadataChangeList> delete_metadata_change_list,
+    const base::Optional<ModelError>& error,
+    std::unique_ptr<RecordList> data_records) {
+  if (error) {
+    LOG(WARNING) << "OnReadAllDataToDelete received a model error: "
+                 << error->ToString();
+    return;
+  }
+
+  std::unique_ptr<WriteBatch> batch = store_->CreateWriteBatch();
+  // We delete all the metadata (even for consents), because it may become
+  // invalid when the sync is reenabled. This may lead to the same consent
+  // being reported multiple times, which is allowed.
+  batch->TakeMetadataChangesFrom(std::move(delete_metadata_change_list));
+
+  UserEventSpecifics specifics;
+  for (const Record& r : *data_records) {
+    if (!specifics.ParseFromString(r.value) ||
+        specifics.event_case() != UserEventSpecifics::EventCase::kUserConsent) {
+      batch->DeleteData(r.id);
+    }
+  }
+
+  store_->CommitWriteBatch(
+      std::move(batch),
+      base::BindOnce(&UserEventSyncBridge::OnCommit, base::AsWeakPtr(this)));
 }
 
 void UserEventSyncBridge::RecordUserEvent(
@@ -216,6 +249,10 @@ void UserEventSyncBridge::OnStoreCreated(
     return;
   }
 
+  // TODO(vitaliii): Attempt to resubmit persistently stored user consents
+  // (probably not immediately on startup).
+  // TODO(vitaliii): Garbage collect old user consents if sync is disabled.
+
   store_ = std::move(store);
   store_->ReadAllMetadata(base::BindOnce(
       &UserEventSyncBridge::OnReadAllMetadata, base::AsWeakPtr(this)));

components/sync/user_events/user_event_sync_bridge.h

@@ -63,6 +63,10 @@ class UserEventSyncBridge : public ModelTypeSyncBridge {
   void OnReadAllData(DataCallback callback,
                      const base::Optional<ModelError>& error,
                      std::unique_ptr<ModelTypeStore::RecordList> data_records);
+  void OnReadAllDataToDelete(
+      std::unique_ptr<MetadataChangeList> delete_metadata_change_list,
+      const base::Optional<ModelError>& error,
+      std::unique_ptr<ModelTypeStore::RecordList> data_records);
 
   void HandleGlobalIdChange(int64_t old_global_id, int64_t new_global_id);
 

components/sync/user_events/user_event_sync_bridge_unittest.cc

@@ -36,6 +36,7 @@ using testing::SaveArg;
 using testing::SizeIs;
 using testing::UnorderedElementsAre;
 using testing::_;
+using WriteBatch = ModelTypeStore::WriteBatch;
 
 MATCHER_P(MatchesUserEvent, expected, "") {
   if (!arg.has_user_event()) {
@@ -101,7 +102,7 @@ class UserEventSyncBridgeTest : public testing::Test {
         mock_processor_.CreateForwardingProcessor(), &test_global_id_mapper_);
     ON_CALL(*processor(), IsTrackingMetadata()).WillByDefault(Return(true));
     ON_CALL(*processor(), DisableSync()).WillByDefault(Invoke([this]() {
-      bridge_->ApplyDisableSyncChanges({});
+      bridge_->ApplyDisableSyncChanges(WriteBatch::CreateMetadataChangeList());
     }));
   }
 
@@ -195,10 +196,30 @@ TEST_F(UserEventSyncBridgeTest, DisableSync) {
 
   EXPECT_CALL(*processor(), DisableSync());
   bridge()->DisableSync();
+  // The bridge may asynchronously query the store to choose what to delete.
+  base::RunLoop().RunUntilIdle();
 
   EXPECT_THAT(GetAllData(), IsEmpty());
 }
 
+TEST_F(UserEventSyncBridgeTest, DisableSyncShouldKeepConsents) {
+  UserEventSpecifics user_consent_specifics(CreateSpecifics(2u, 2u, 2u));
+  auto* consent = user_consent_specifics.mutable_user_consent();
+  consent->set_feature(UserEventSpecifics::UserConsent::CHROME_SYNC);
+  bridge()->RecordUserEvent(
+      std::make_unique<UserEventSpecifics>(user_consent_specifics));
+  ASSERT_THAT(GetAllData(), SizeIs(1));
+
+  EXPECT_CALL(*processor(), DisableSync());
+  bridge()->DisableSync();
+  // The bridge may asynchronously query the store to choose what to delete.
+  base::RunLoop().RunUntilIdle();
+
+  // User consent specific must be persisted when sync is disabled.
+  EXPECT_THAT(GetAllData(),
+              ElementsAre(Pair(_, MatchesUserEvent(user_consent_specifics))));
+}
+
 TEST_F(UserEventSyncBridgeTest, MultipleRecords) {
   std::set<std::string> unique_storage_keys;
   EXPECT_CALL(*processor(), DoPut(_, _, _))