Bug 1423399 - Avoid duplicating synced history visits older than your most recent 20 r?kitcambridge draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Mon, 18 Dec 2017 19:40:38 -0500
changeset 713788 4f3af013007e113c1f087c913b9d7dadbf17f3c8
parent 713743 62dd5404cf55e29412d5fff8fe9105076b1ca437
child 744427 44255cb098bce563bd21ddfe0ba0f8ccdb98929d
push id93746
push userbmo:tchiovoloni@mozilla.com
push dateWed, 20 Dec 2017 23:39:10 +0000
reviewerskitcambridge
bugs1423399
milestone59.0a1
Bug 1423399 - Avoid duplicating synced history visits older than your most recent 20 r?kitcambridge MozReview-Commit-ID: 2CAutmuJWhb
services/sync/modules/engines/history.js
services/sync/tests/unit/test_history_engine.js
--- a/services/sync/modules/engines/history.js
+++ b/services/sync/modules/engines/history.js
@@ -314,16 +314,22 @@ HistoryStore.prototype = {
     // can simply check for containment below.
     let curVisitsAsArray = [];
     let curVisits = new Set();
     try {
       curVisitsAsArray = await PlacesSyncUtils.history.fetchVisitsForURL(record.histUri);
     } catch (e) {
       this._log.error("Error while fetching visits for URL ${record.histUri}", record.histUri);
     }
+    let oldestAllowed = PlacesSyncUtils.bookmarks.EARLIEST_BOOKMARK_TIMESTAMP;
+    if (curVisitsAsArray.length == 20) {
+      let oldestVisit = curVisitsAsArray[curVisitsAsArray.length - 1];
+      oldestAllowed = PlacesSyncUtils.history.clampVisitDate(
+        PlacesUtils.toDate(oldestVisit.date).getTime());
+    }
 
     let i, k;
     for (i = 0; i < curVisitsAsArray.length; i++) {
       // Same logic as used in the loop below to generate visitKey.
       let {date, type} = curVisitsAsArray[i];
       let dateObj = PlacesUtils.toDate(date);
       let millis = PlacesSyncUtils.history.clampVisitDate(dateObj).getTime();
       curVisits.add(`${millis},${type}`);
@@ -347,16 +353,21 @@ HistoryStore.prototype = {
         continue;
       }
 
       // Dates need to be integers. Future and far past dates are clamped to the
       // current date and earliest sensible date, respectively.
       let originalVisitDate = PlacesUtils.toDate(Math.round(visit.date));
       visit.date = PlacesSyncUtils.history.clampVisitDate(originalVisitDate);
 
+      if (visit.date.getTime() < oldestAllowed) {
+        // Visit is older than the oldest visit we have, and we have so many
+        // visits for this uri that we hit our limit when inserting.
+        continue;
+      }
       let visitKey = `${visit.date.getTime()},${visit.type}`;
       if (curVisits.has(visitKey)) {
         // Visit is a dupe, don't increment 'k' so the element will be
         // overwritten.
         continue;
       }
 
       // Note the visit key, so that we don't add duplicate visits with
--- a/services/sync/tests/unit/test_history_engine.js
+++ b/services/sync/tests/unit/test_history_engine.js
@@ -184,8 +184,73 @@ add_task(async function test_history_vis
 
   // Make sure that we didn't duplicate the visit when inserting. (Prior to bug
   // 1423395, we would insert a duplicate visit, where the timestamp was
   // effectively `Math.round(microsecondTimestamp / 1000) * 1000`.)
   visits = await PlacesSyncUtils.history.fetchVisitsForURL("https://www.example.com");
   equal(visits.length, 2);
   await PlacesTestUtils.clearHistory();
 });
+
+add_task(async function test_history_visit_dedupe_old() {
+  let engine = new HistoryEngine(Service);
+  await engine.initialize();
+  let server = await serverForFoo(engine);
+  await SyncTestingInfrastructure(server);
+
+  Svc.Obs.notify("weave:engine:start-tracking");
+
+  await PlacesUtils.history.insert({
+    url: "https://www.example.com",
+    visits: Array.from({ length: 25 }, (_, index) => ({
+      transition: PlacesUtils.history.TRANSITION_LINK,
+      date: new Date(Date.UTC(2017, 10, 1 + index)),
+    }))
+  });
+
+  let recentVisits = await PlacesSyncUtils.history.fetchVisitsForURL("https://www.example.com");
+  equal(recentVisits.length, 20);
+  let {visits: allVisits, guid} = await PlacesUtils.history.fetch("https://www.example.com", {
+    includeVisits: true
+  });
+  equal(allVisits.length, 25);
+
+  let collection = server.user("foo").collection("history");
+
+  await sync_engine_and_validate_telem(engine, false);
+
+  let wbo = collection.wbo(guid);
+  let data = JSON.parse(JSON.parse(wbo.payload).ciphertext);
+
+  data.visits.push(
+    // Add a couple remote visit equivalent to some old visits we have already
+    {
+      date: Date.UTC(2017, 10, 1) * 1000, // Nov 1, 2017
+      type: PlacesUtils.history.TRANSITIONS.LINK
+    }, {
+      date: Date.UTC(2017, 10, 2) * 1000, // Nov 2, 2017
+      type: PlacesUtils.history.TRANSITIONS.LINK
+    },
+    // Add a couple new visits to make sure we are still applying them.
+    {
+      date: Date.UTC(2017, 11, 4) * 1000, // Dec 4, 2017
+      type: PlacesUtils.history.TRANSITIONS.LINK
+    }, {
+      date: Date.UTC(2017, 11, 5) * 1000, // Dec 5, 2017
+      type: PlacesUtils.history.TRANSITIONS.LINK
+    }
+  );
+
+  collection.insertWBO(new ServerWBO(guid, encryptPayload(data), Date.now() / 1000 + 10));
+  engine.lastSync = Date.now() / 1000 - 30;
+  await sync_engine_and_validate_telem(engine, false);
+
+  allVisits = (await PlacesUtils.history.fetch("https://www.example.com", {
+    includeVisits: true
+  })).visits;
+
+  equal(allVisits.length, 27);
+  ok(allVisits.find(x => x.date.getTime() === Date.UTC(2017, 11, 4)),
+     "Should contain the Dec. 4th visit");
+  ok(allVisits.find(x => x.date.getTime() === Date.UTC(2017, 11, 5)),
+     "Should contain the Dec. 5th visit");
+  await PlacesTestUtils.clearHistory();
+});