Bug 1405566 - Clamp future and far past synced history visit dates. r=markh, a=ritu
authorKit Cambridge <kit@yakshaving.ninja>
Wed, 04 Oct 2017 14:16:29 -0700
changeset 432373 dcaaba0bd014
parent 432372 142a0743821c
child 432374 465b880a2af9
push id7946
push userryanvm@gmail.com
push dateWed, 11 Oct 2017 17:47:53 +0000
treeherdermozilla-beta@465b880a2af9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh, ritu
bugs1405566
milestone57.0
Bug 1405566 - Clamp future and far past synced history visit dates. r=markh, a=ritu 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);