Bug 1423395 - Clamp and truncate microseconds from incoming history visit dates when deduping r?markh draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Tue, 05 Dec 2017 19:40:37 -0500
changeset 712919 ef3956d4a7e35c11b31d440ad97912e6ce436077
parent 712645 5572465c08a9ce0671dcd01c721f9356fcd53a65
child 713181 2409c96cf32661605bbb9d427971f386ed927e2b
push id93487
push userbmo:tchiovoloni@mozilla.com
push dateTue, 19 Dec 2017 03:55:10 +0000
reviewersmarkh
bugs1423395
milestone59.0a1
Bug 1423395 - Clamp and truncate microseconds from incoming history visit dates when deduping r?markh MozReview-Commit-ID: H1lljWcSZLZ
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
@@ -309,17 +309,21 @@ HistoryStore.prototype = {
     try {
       curVisitsAsArray = await PlacesSyncUtils.history.fetchVisitsForURL(record.histUri);
     } catch (e) {
       this._log.error("Error while fetching visits for URL ${record.histUri}", record.histUri);
     }
 
     let i, k;
     for (i = 0; i < curVisitsAsArray.length; i++) {
-      curVisits.add(curVisitsAsArray[i].date + "," + curVisitsAsArray[i].type);
+      // 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}`);
     }
 
     // Walk through the visits, make sure we have sound data, and eliminate
     // dupes. The latter is done by rewriting the array in-place.
     for (i = 0, k = 0; i < record.visits.length; i++) {
       let visit = record.visits[k] = record.visits[i];
 
       if (!visit.date || typeof visit.date != "number" || !Number.isInteger(visit.date)) {
@@ -335,18 +339,17 @@ 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);
 
-      let visitDateAsPRTime = PlacesUtils.toPRTime(visit.date);
-      let visitKey = visitDateAsPRTime + "," + visit.type;
+      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
       // clamped timestamps.
--- a/services/sync/tests/unit/test_history_engine.js
+++ b/services/sync/tests/unit/test_history_engine.js
@@ -1,19 +1,43 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 Cu.import("resource://services-sync/service.js");
 Cu.import("resource://services-sync/engines/history.js");
+Cu.import("resource://services-common/utils.js");
 Cu.import("resource://testing-common/services/sync/utils.js");
 
 add_task(async function setup() {
   initTestLogging("Trace");
 });
 
+async function rawAddVisit(id, uri, visitPRTime, transitionType) {
+  return new Promise((resolve, reject) => {
+    let results = [];
+    let handler = {
+      handleResult(result) {
+        results.push(result);
+      },
+      handleError(resultCode, placeInfo) {
+        do_throw(`updatePlaces gave error ${resultCode}!`);
+      },
+      handleCompletion(count) {
+        resolve({ results, count });
+      }
+    };
+    PlacesUtils.asyncHistory.updatePlaces([{
+      guid: id,
+      uri: typeof uri == "string" ? CommonUtils.makeURI(uri) : uri,
+      visits: [{ visitDate: visitPRTime, transitionType }]
+    }], handler);
+  });
+}
+
+
 add_task(async function test_history_download_limit() {
   let engine = new HistoryEngine(Service);
   await engine.initialize();
 
   let server = await serverForFoo(engine);
   await SyncTestingInfrastructure(server);
 
   let lastSync = Date.now() / 1000;
@@ -103,9 +127,65 @@ add_task(async function test_history_dow
     "place0000008", "place0000009"]);
 
   // Sync again to clear out the backlog.
   engine.lastModified = collection.modified;
   ping = await sync_engine_and_validate_telem(engine, false);
   deepEqual(ping.engines[0].incoming, { applied: 5 });
 
   deepEqual(engine.toFetch, []);
+  await PlacesTestUtils.clearHistory();
 });
+
+add_task(async function test_history_visit_roundtrip() {
+  let engine = new HistoryEngine(Service);
+  await engine.initialize();
+  let server = await serverForFoo(engine);
+  await SyncTestingInfrastructure(server);
+
+  Svc.Obs.notify("weave:engine:start-tracking");
+
+  let id = "aaaaaaaaaaaa";
+  let oneHourMS = 60 * 60 * 1000;
+  // Insert a visit with a non-round microsecond timestamp (e.g. it's not evenly
+  // divisible by 1000). This will typically be the case for visits that occur
+  // during normal navigation.
+  let time = (Date.now() - oneHourMS) * 1000 + 555;
+  // We use the low level updatePlaces api since it lets us provide microseconds
+  let {count} = await rawAddVisit(id, "https://www.example.com", time,
+                                  PlacesUtils.history.TRANSITIONS.TYPED);
+  equal(count, 1);
+  // Check that it was inserted and that we didn't round on the insert.
+  let visits = await PlacesSyncUtils.history.fetchVisitsForURL("https://www.example.com");
+  equal(visits.length, 1);
+  equal(visits[0].date, time);
+
+  let collection = server.user("foo").collection("history");
+
+  // Sync the visit up to the server.
+  await sync_engine_and_validate_telem(engine, false);
+
+  let wbo = collection.wbo(id);
+  let data = JSON.parse(JSON.parse(wbo.payload).ciphertext);
+  // Double-check that we didn't round the visit's timestamp to the nearest
+  // millisecond when uploading.
+  equal(data.visits[0].date, time);
+
+  // Add a remote visit so that we get past the deepEquals check in reconcile
+  // (otherwise the history engine will skip applying this record). The contents
+  // of this visit don't matter, beyond the fact that it needs to exist.
+  data.visits.push({
+    date: (Date.now() - oneHourMS / 2) * 1000,
+    type: PlacesUtils.history.TRANSITIONS.LINK
+  });
+  collection.insertWBO(new ServerWBO(id, encryptPayload(data), Date.now() / 1000 + 10));
+
+  // Force a remote sync.
+  engine.lastSync = Date.now() / 1000 - 30;
+  await sync_engine_and_validate_telem(engine, false);
+
+  // 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();
+});