--- a/services/sync/modules/engines/history.js
+++ b/services/sync/modules/engines/history.js
@@ -270,26 +270,33 @@ HistoryStore.prototype = {
if (!visit.type ||
!Object.values(PlacesUtils.history.TRANSITIONS).includes(visit.type)) {
this._log.warn("Encountered record with invalid visit type: " +
visit.type + "; ignoring.");
continue;
}
- // Dates need to be integers.
- visit.date = Math.round(visit.date);
+ // 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 (curVisits.indexOf(visit.date + "," + visit.type) != -1) {
+ let visitDateAsPRTime = PlacesUtils.toPRTime(visit.date);
+ let visitKey = visitDateAsPRTime + "," + visit.type;
+ if (curVisits.indexOf(visitKey) != -1) {
// Visit is a dupe, don't increment 'k' so the element will be
// overwritten.
continue;
}
- visit.date = PlacesUtils.toDate(visit.date);
+ // Note the visit key, so that we don't add duplicate visits with
+ // clamped timestamps.
+ curVisits.push(visitKey);
+
visit.transition = visit.type;
k += 1;
}
record.visits.length = k; // truncate array
// No update if there aren't any visits to apply.
// mozIAsyncHistory::updatePlaces() wants at least one visit.
// In any case, the only thing we could change would be the title
--- a/services/sync/tests/unit/test_history_store.js
+++ b/services/sync/tests/unit/test_history_store.js
@@ -1,12 +1,13 @@
/* Any copyright is dedicated to the Public Domain.
http://creativecommons.org/publicdomain/zero/1.0/ */
Cu.import("resource://testing-common/PlacesTestUtils.jsm");
+Cu.import("resource://gre/modules/PlacesSyncUtils.jsm");
Cu.import("resource://gre/modules/XPCOMUtils.jsm");
Cu.import("resource://services-common/async.js");
Cu.import("resource://services-common/utils.js");
Cu.import("resource://services-sync/engines/history.js");
Cu.import("resource://services-sync/service.js");
Cu.import("resource://services-sync/util.js");
const TIMESTAMP1 = (Date.now() - 103406528) * 1000;
@@ -54,16 +55,22 @@ function promiseOnVisitObserved() {
Ci.nsINavHistoryObserver,
Ci.nsINavHistoryObserver_MOZILLA_1_9_1_ADDITIONS,
Ci.nsISupportsWeakReference
])
}, true);
});
}
+function isDateApproximately(actual, expected, skewMillis = 1000) {
+ let lowerBound = expected - skewMillis;
+ let upperBound = expected + skewMillis;
+ return actual >= lowerBound && actual <= upperBound;
+}
+
var engine = new HistoryEngine(Service);
Async.promiseSpinningly(engine.initialize());
var store = engine._store;
async function applyEnsureNoFailures(records) {
do_check_eq((await store.applyIncomingBatch(records)).length, 0);
}
var fxuri, fxguid, tburi, tbguid;
@@ -251,16 +258,90 @@ add_task(async function test_invalid_rec
await applyEnsureNoFailures([
{id: Utils.makeGUID(),
histUri: "http://getfirebug.com",
title: "Get Firebug!",
visits: []}
]);
});
+add_task(async function test_clamp_visit_dates() {
+ let futureVisitTime = Date.now() + 5 * 60 * 1000;
+ let recentVisitTime = Date.now() - 5 * 60 * 1000;
+
+ await applyEnsureNoFailures([{
+ id: "visitAAAAAAA",
+ histUri: "http://example.com/a",
+ title: "A",
+ visits: [{
+ date: "invalidDate",
+ type: Ci.nsINavHistoryService.TRANSITION_LINK,
+ }],
+ }, {
+ id: "visitBBBBBBB",
+ histUri: "http://example.com/b",
+ title: "B",
+ visits: [{
+ date: 100,
+ type: Ci.nsINavHistoryService.TRANSITION_TYPED,
+ }, {
+ date: 250,
+ type: Ci.nsINavHistoryService.TRANSITION_TYPED,
+ }, {
+ date: recentVisitTime * 1000,
+ type: Ci.nsINavHistoryService.TRANSITION_TYPED,
+ }],
+ }, {
+ id: "visitCCCCCCC",
+ histUri: "http://example.com/c",
+ title: "D",
+ visits: [{
+ date: futureVisitTime * 1000,
+ type: Ci.nsINavHistoryService.TRANSITION_BOOKMARK,
+ }],
+ }, {
+ id: "visitDDDDDDD",
+ histUri: "http://example.com/d",
+ title: "D",
+ visits: [{
+ date: recentVisitTime * 1000,
+ type: Ci.nsINavHistoryService.TRANSITION_DOWNLOAD,
+ }],
+ }]);
+
+ let visitsForA = await PlacesSyncUtils.history.fetchVisitsForURL(
+ "http://example.com/a");
+ deepEqual(visitsForA, [], "Should ignore visits with invalid dates");
+
+ let visitsForB = await PlacesSyncUtils.history.fetchVisitsForURL(
+ "http://example.com/b");
+ deepEqual(visitsForB, [{
+ date: recentVisitTime * 1000,
+ type: Ci.nsINavHistoryService.TRANSITION_TYPED,
+ }, {
+ // We should clamp visit dates older than original Mosaic release.
+ date: PlacesSyncUtils.bookmarks.EARLIEST_BOOKMARK_TIMESTAMP * 1000,
+ type: Ci.nsINavHistoryService.TRANSITION_TYPED,
+ }], "Should record clamped visit and valid visit for B");
+
+ let visitsForC = await PlacesSyncUtils.history.fetchVisitsForURL(
+ "http://example.com/c");
+ equal(visitsForC.length, 1, "Should record clamped future visit for C");
+ let visitDateForC = PlacesUtils.toDate(visitsForC[0].date);
+ ok(isDateApproximately(visitDateForC, Date.now()),
+ "Should clamp future visit date for C to now");
+
+ let visitsForD = await PlacesSyncUtils.history.fetchVisitsForURL(
+ "http://example.com/d");
+ deepEqual(visitsForD, [{
+ date: recentVisitTime * 1000,
+ type: Ci.nsINavHistoryService.TRANSITION_DOWNLOAD,
+ }], "Should not clamp valid visit dates");
+});
+
add_task(async function test_remove() {
_("Remove an existent record and a non-existent from the store.");
await applyEnsureNoFailures([{id: fxguid, deleted: true},
{id: Utils.makeGUID(), deleted: true}]);
do_check_false((await store.itemExists(fxguid)));
let queryres = queryHistoryVisits(fxuri);
do_check_eq(queryres.length, 0);