Bug 1405566 - Clamp future and far past synced history visit dates. r=markh
authorKit Cambridge <kit@yakshaving.ninja>
Wed, 04 Oct 2017 14:16:29 -0700
changeset 384642 85b73d4c4c9cf8792096b16db0c5a711b1e0ee24
parent 384641 843d4559d5916e0548c4bc6cd85238fa187cf63f
child 384643 442e8020e2acc12d945c979d70d09da04e0107b9
push id52706
push userkcambridge@mozilla.com
push dateThu, 05 Oct 2017 05:17:15 +0000
treeherderautoland@85b73d4c4c9c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh
bugs1405566
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1405566 - Clamp future and far past synced history visit dates. r=markh MozReview-Commit-ID: Gs0DpTTu1Ab
services/sync/modules/engines/history.js
services/sync/tests/unit/test_history_store.js
toolkit/components/places/PlacesSyncUtils.jsm
--- 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);
 
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -71,16 +71,35 @@ function* chunkArray(array, chunkLength)
   let startIndex = 0;
   while (startIndex < array.length) {
     yield array.slice(startIndex, startIndex += chunkLength);
   }
 }
 
 const HistorySyncUtils = PlacesSyncUtils.history = Object.freeze({
   /**
+   * Clamps a history visit date between the current date and the earliest
+   * sensible date.
+   *
+   * @param {Date} visitDate
+   *        The visit date.
+   * @return {Date} The clamped visit date.
+   */
+  clampVisitDate(visitDate) {
+    let currentDate = new Date();
+    if (visitDate > currentDate) {
+      return currentDate;
+    }
+    if (visitDate < BookmarkSyncUtils.EARLIEST_BOOKMARK_TIMESTAMP) {
+      return new Date(BookmarkSyncUtils.EARLIEST_BOOKMARK_TIMESTAMP);
+    }
+    return visitDate;
+  },
+
+  /**
    * Fetches the frecency for the URL provided
    *
    * @param url
    * @returns {Number} The frecency of the given url
    */
   async fetchURLFrecency(url) {
     let canonicalURL = PlacesUtils.SYNC_BOOKMARK_VALIDATORS.url(url);