Bug 1423399 - Avoid duplicating synced history visits older than your most recent 20 r?kitcambridge
MozReview-Commit-ID: 2CAutmuJWhb
--- 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();
+});