History Sync: Don't upload foreign visits

A syncing client should only ever upload its own visits to the server,
never those coming from other clients.
Generally, foreign visits should never get updated in the history DB
anyway (except through Sync), but it some cases it might happen (e.g.
data migrations/cleanups, or maybe just bugs elsewhere). In that case,
it's better not to upload any such changes, to avoid messing up other
clients.
This CL adds an appropriate check plus a unit test.

Bug: 1395826
Change-Id: I91491de0dc9d75d05534007f5d62a6edca805bff
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4083546
Commit-Queue: Marc Treib <treib@chromium.org>
Auto-Submit: Marc Treib <treib@chromium.org>
Reviewed-by: Victor Vianna <victorvianna@google.com>
Commit-Queue: Victor Vianna <victorvianna@google.com>
Cr-Commit-Position: refs/heads/main@{#1080312}

components/history/core/browser/sync/history_sync_bridge.cc

@@ -76,7 +76,7 @@ enum class SyncHistoryDatabaseError {
   // Deprecated (call sites were removed):
   // kOnURLVisitedGetVisit = 4,
   // kOnURLsDeletedReadMetadata = 5,
-  kOnVisitUpdatedGetURL = 6,
+  // kOnVisitUpdatedGetURL = 6,
   kGetAllDataReadMetadata = 7,
   kMaxValue = kGetAllDataReadMetadata
 };
@@ -424,6 +424,8 @@ absl::optional<SpecificsError> GetSpecificsError(
     return SpecificsError::kMissingRequiredFields;
   }
 
+  // TODO(crbug.com/1364576): Filter out URLs that shouldn't be synced.
+
   base::Time visit_time = GetVisitTime(specifics);
 
   // Already-expired visits are not valid. (They wouldn't really cause any harm,
@@ -862,6 +864,13 @@ void HistorySyncBridge::MaybeCommit(const VisitRow& visit_row) {
     return;
   }
 
+  // If this visit originally came from a different device, don't update it.
+  // This shouldn't usually happen, but if it does happen for some reason (e.g.
+  // due to a bug elsewhere), better not to mess up other clients.
+  if (!visit_row.originator_cache_guid.empty()) {
+    return;
+  }
+
   // All conditions are fulfilled - convert the visit into Sync's format and
   // send it on.
   std::vector<std::unique_ptr<syncer::EntityData>> entity_data_list =

components/history/core/browser/sync/history_sync_bridge_unittest.cc

@@ -887,6 +887,52 @@ TEST_F(HistorySyncBridgeTest, UploadsUpdatedLocalVisit) {
       visit_duration);
 }
 
+TEST_F(HistorySyncBridgeTest, DoesNotUploadUpdatedForeignVisit) {
+  sync_pb::HistorySpecifics remote_entity =
+      CreateSpecifics(base::Time::Now() - base::Minutes(1), "remote_cache_guid",
+                      GURL("https://remote.com"));
+
+  // Start Sync, so the remote data gets written to the local DB.
+  ApplyInitialSyncChanges({remote_entity});
+  ASSERT_EQ(backend()->GetVisits().size(), 1u);
+
+  VisitRow visit_row = backend()->GetVisits()[0];
+  const std::string storage_key =
+      HistorySyncMetadataDatabase::StorageKeyFromVisitTime(
+          visit_row.visit_time);
+
+  // The visit is known in the processor (representing the server state).
+  ASSERT_EQ(processor()->GetEntities().size(), 1u);
+  ASSERT_EQ(processor()->GetEntities().count(storage_key), 1u);
+  ASSERT_FALSE(processor()->IsEntityUnsynced(storage_key));
+  ASSERT_EQ(processor()
+                ->GetEntities()
+                .at(storage_key)
+                .specifics.history()
+                .visit_duration_micros(),
+            0);
+
+  // Update the foreign visit locally. Generally, foreign visits shouldn't get
+  // updated on this device, but some other code interacting with the history DB
+  // might do it (probably mistakenly).
+  visit_row.visit_duration = base::Seconds(10);
+  ASSERT_TRUE(backend()->UpdateVisit(visit_row));
+  bridge()->OnVisitUpdated(visit_row);
+
+  // The updated visit should *not* have been sent to the processor - the entity
+  // in the processor should *not* be unsynced, and its visit duration should
+  // still be 0.
+  ASSERT_EQ(processor()->GetEntities().size(), 1u);
+  ASSERT_EQ(processor()->GetEntities().count(storage_key), 1u);
+  EXPECT_FALSE(processor()->IsEntityUnsynced(storage_key));
+  EXPECT_EQ(processor()
+                ->GetEntities()
+                .at(storage_key)
+                .specifics.history()
+                .visit_duration_micros(),
+            0);
+}
+
 TEST_F(HistorySyncBridgeTest, UploadsUpdatedUrlTitle) {
   // Start syncing (with no data yet).
   ApplyInitialSyncChanges({});

tools/metrics/histograms/enums.xml

@@ -97639,9 +97639,21 @@ would be helpful to identify which type is being sent.
       label="Error in MergeSyncData() or ApplySyncChanges(): writing metadata"/>
   <int value="2" label="OnDatabaseError() called"/>
   <int value="3" label="Error in LoadMetadata()"/>
-  <int value="4" label="Error in OnURLVisited(): reading visit data"/>
-  <int value="5" label="Error in OnURLsDeleted(): reading metadata"/>
-  <int value="6" label="Error in OnVisitUpdated(): reading URL data"/>
+  <int value="4" label="(obsolete) Error in OnURLVisited(): reading visit data">
+    <obsolete>
+      Was never recorded.
+    </obsolete>
+  </int>
+  <int value="5" label="(obsolete) Error in OnURLsDeleted(): reading metadata">
+    <obsolete>
+      Was never recorded.
+    </obsolete>
+  </int>
+  <int value="6" label="(obsolete) Error in OnVisitUpdated(): reading URL data">
+    <obsolete>
+      Was never recorded.
+    </obsolete>
+  </int>
   <int value="7" label="Error in GetAllData(): reading metadata"/>
 </enum>