Bug 1423395 - Clamp and truncate microseconds from incoming history visit dates when deduping r?markh
MozReview-Commit-ID: H1lljWcSZLZ
--- 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();
+});