Bug 578694 - Dedupe and handle deletions for history records. draft
authorKit Cambridge <kit@yakshaving.ninja>
Wed, 15 Nov 2017 10:45:38 -0800
changeset 702930 bd0c837491691646ed1eaf2c3e3485d9e25e4e07
parent 702922 3d73d64a8f7e3f481cb12ce1346936ce66721d59
child 741602 ad43bf794955a9643e5f59ba1708815467fdcae0
push id90634
push userbmo:kit@mozilla.com
push dateFri, 24 Nov 2017 01:12:49 +0000
bugs578694
milestone59.0a1
Bug 578694 - Dedupe and handle deletions for history records. MozReview-Commit-ID: AJeEpcLPZFu
services/sync/modules/engines/history.js
services/sync/tests/unit/test_history_engine.js
services/sync/tests/unit/test_history_tracker.js
toolkit/components/places/PlacesSyncUtils.jsm
--- a/services/sync/modules/engines/history.js
+++ b/services/sync/modules/engines/history.js
@@ -27,20 +27,61 @@ XPCOMUtils.defineLazyModuleGetter(this, 
                                   "resource://gre/modules/PlacesSyncUtils.jsm");
 
 this.HistoryRec = function HistoryRec(collection, id) {
   CryptoWrapper.call(this, collection, id);
 };
 HistoryRec.prototype = {
   __proto__: CryptoWrapper.prototype,
   _logName: "Sync.Record.History",
-  ttl: HISTORY_TTL
+  ttl: HISTORY_TTL,
+
+  cloneWithVisits(filter) {
+    if (this.deleted) {
+      throw new TypeError("Can't filter visits from tombstone");
+    }
+    let beginDate = "beginDate" in filter ? filter.beginDate.getTime() :
+                    -Infinity;
+    if (Number.isNaN(beginDate)) {
+      throw new TypeError("Can't filter visits with invalid begin date");
+    }
+    let endDate = "endDate" in filter ? filter.endDate.getTime() :
+                  Infinity;
+    if (Number.isNaN(endDate)) {
+      throw new TypeError("Can't filter visits with invalid end date");
+    }
+    let newVisits = [];
+    for (let visit of this.visits) {
+      let originalVisitDate = PlacesUtils.toDate(Math.round(visit.date));
+      let visitDate = PlacesSyncUtils.history.clampVisitDate(originalVisitDate);
+      if (visitDate >= beginDate && visitDate <= endDate) {
+        newVisits.push({
+          type: visit.type,
+          date: PlacesUtils.toPRTime(visitDate),
+        });
+      }
+    }
+    if (!newVisits.length) {
+      return null;
+    }
+    let record = new HistoryRec(this.collection, this.id);
+    record.histUri = this.histUri;
+    record.title = this.title;
+    record.visits = newVisits;
+    return record;
+  },
 };
 
-Utils.deferGetSet(HistoryRec, "cleartext", ["histUri", "title", "visits"]);
+// `deletedAt` is only set for tombstones. It's the client time, in
+// microseconds, when all visits for a URL were deleted on that client. We use
+// this as a hint during reconciliation, to avoid deleting new local visits
+// since the tombstone was written. If it's missing, we'll clear all local
+// visits, including ones that we might not have uploaded yet.
+Utils.deferGetSet(HistoryRec, "cleartext", ["histUri", "title", "visits",
+                                            "deletedAt"]);
 
 
 this.HistoryEngine = function HistoryEngine(service) {
   SyncEngine.call(this, "History", service);
 };
 HistoryEngine.prototype = {
   __proto__: SyncEngine.prototype,
   _recordObj: HistoryRec,
@@ -74,16 +115,142 @@ HistoryEngine.prototype = {
     if (!modifiedGUIDs.length) {
       return {};
     }
 
     let guidsToRemove = await PlacesSyncUtils.history.determineNonSyncableGuids(modifiedGUIDs);
     this._tracker.removeChangedID(...guidsToRemove);
     return this._tracker.changedIDs;
   },
+
+  async _findDupe(record) {
+    let guid = null;
+    try {
+      guid = await PlacesSyncUtils.history.fetchGuidForURL(record.histUri);
+    } catch (ex) {
+      this._log.error("Error fetching GUID for record ${guid}: ${ex}",
+                      { guid: record.id, ex });
+    }
+    return guid;
+  },
+
+  async _createRecord(id) {
+    let record = await super._createRecord(id);
+    if (record.deleted) {
+      let deletedAt = this._modified.getModifiedTimestamp(record.id);
+      if (Number.isFinite(deletedAt)) {
+        record.deletedAt = PlacesUtils.toPRTime(new Date(deletedAt * 1000));
+      }
+    }
+    return record;
+  },
+
+  async _reconcile(record) {
+    let existsLocally = await this._store.itemExists(record.id);
+
+    if (record.deleted) {
+      if (!existsLocally) {
+        this._log.trace("Record ${id} deleted on both sides; nothing to do",
+                        { id: record.id });
+        return false;
+      }
+
+      let locallyModified = this._modified.has(record.id);
+      if (locallyModified) {
+        // If the record is locally changed and remotely deleted, clear all
+        // local visits up to the date of the most recent remotely deleted
+        // visit. This avoids losing new local visits to sites removed from
+        // history on another device.
+        let filter = { guid: record.id };
+        if (typeof record.deletedAt == "number") {
+          // Older tombstones don't have `deletedAt`, in which case we clear all
+          // visits.
+          let remotelyDeletedAt = PlacesUtils.toDate(record.deletedAt);
+          filter.endDate = PlacesSyncUtils.history.clampVisitDate(
+            remotelyDeletedAt);
+        }
+
+        this._log.trace("Record ${id} has new local visits and deleted " +
+                        "remotely; clearing local visits with filter " +
+                        "${filter}", { id: record.id, filter });
+        await PlacesUtils.history.removeVisitsByFilter(filter);
+        let hasRemainingVisits = await PlacesUtils.history.hasVisits(record.id);
+        if (!hasRemainingVisits) {
+          // If we deleted all visits after applying the tombstone, no need to
+          // reupload our local tombstone.
+          this._modified.delete(record.id);
+        }
+        return false;
+      }
+
+      this._log.trace("Record ${id} has no new local visits and deleted " +
+                      "remotely; taking deletion", { id: record.id });
+      return true;
+    }
+
+    if (!existsLocally) {
+      // Check for duplicate records with the same URL and different GUID. This
+      // matches the logic from the base `_reconcile` implementation, except our
+      // `_findDupe` is guaranteed to return a GUID that exists locally, so we
+      // take it.
+      let localDupeGUID = await this._findDupe(record);
+      if (localDupeGUID) {
+        this._log.trace("Local Place ${localDupeGUID} is dupe of remote " +
+                        "record ${id}", { localDupeGUID, id: record.id });
+
+        let dupeLocallyModified = this._modified.has(localDupeGUID);
+        if (dupeLocallyModified) {
+          this._modified.changeID(localDupeGUID, record.id);
+        }
+        await this._switchItemToDupe(localDupeGUID, record);
+        existsLocally = true;
+      } else {
+        this._log.trace("No local dupe for remote record ${id} with ${url}",
+                        { id: record.id, url: record.histUri });
+      }
+    }
+
+    let locallyModified = this._modified.has(record.id);
+    if (locallyModified) {
+      if (!existsLocally) {
+        // If the record is locally deleted and remotely changed, only take all
+        // remote visits newer than the local deletion. This avoids resurrecting
+        // locally deleted visits that still exist remotely.
+        let locallyDeletedAt = this._modified.getModifiedTimestamp(record.id);
+        let beginDate = new Date(locallyDeletedAt * 1000);
+
+        let newRecord = record.cloneWithVisits({ beginDate });
+        if (newRecord) {
+          this._log.trace("Record ${id} deleted locally and has new remote " +
+                          "visits; taking remote visits since ${beginDate}",
+                          { id: record.id, beginDate });
+          await this._store.applyIncomingBatch([newRecord]);
+        } else {
+          this._log.trace("Record ${id} deleted locally and has no new " +
+                          "remote visits since ${beginDate}",
+                          { id: record.id, beginDate });
+        }
+        return false;
+      }
+
+      // Otherwise, the record has new visits locally and remotely. Take the
+      // remote record; we'll merge its visits into Places. Note that we
+      // intentionally *don't* remove the ID from `this._modified`, so that
+      // we still upload new local visits.
+      this._log.trace("Record ${id} exists locally and remotely; taking " +
+                      "remote and merging visits", { id: record.id });
+      return true;
+    }
+
+    // The record isn't modified locally, and has new visits remotely. Take the
+    // remote record and merge its visits into Places.
+    this._log.trace("Record ${id} is new or unmodified locally; taking remote",
+                    { id: record.id });
+    return true;
+  },
 };
 
 function HistoryStore(name, engine) {
   Store.call(this, name, engine);
 
   // Explicitly nullify our references to our cached services so we don't leak
   Svc.Obs.add("places-shutdown", function() {
     for (let query in this._stmts) {
@@ -112,73 +279,25 @@ HistoryStore.prototype = {
     }
 
     this._log.trace("Creating SQL statement: " + query);
     let db = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase)
                         .DBConnection;
     return this._stmts[query] = db.createAsyncStatement(query);
   },
 
-  // Some helper functions to handle GUIDs
-  async setGUID(uri, guid) {
-
-    if (!guid) {
-      guid = Utils.makeGUID();
-    }
-
-    try {
-      await PlacesSyncUtils.history.changeGuid(uri, guid);
-    } catch (e) {
-      this._log.error("Error setting GUID ${guid} for URI ${uri}", guid, uri);
-    }
-
-    return guid;
+  changeItemID(oldID, newID) {
+    return PlacesSyncUtils.history.changeGuid(oldID, newID);
   },
 
-  async GUIDForUri(uri, create) {
-
-    // Use the existing GUID if it exists
-    let guid;
-    try {
-      guid = await PlacesSyncUtils.history.fetchGuidForURL(uri);
-    } catch (e) {
-      this._log.error("Error fetching GUID for URL ${uri}", uri);
-    }
-
-    // If the URI has an existing GUID, return it.
-    if (guid) {
-      return guid;
-    }
-
-    // If the URI doesn't have a GUID and we were indicated to create one.
-    if (create) {
-      return this.setGUID(uri);
-    }
-
-    // If the URI doesn't have a GUID and we didn't create one for it.
-    return null;
-  },
-
-  async changeItemID(oldID, newID) {
-    let info = await PlacesSyncUtils.history.fetchURLInfoForGuid(oldID);
-    if (!info) {
-      throw new Error(`Can't change ID for nonexistent history entry ${oldID}`);
-    }
-    this.setGUID(info.url, newID);
-  },
-
-  async getAllIDs() {
-    let urls = await PlacesSyncUtils.history.getAllURLs({ since: new Date((Date.now() - THIRTY_DAYS_IN_MS)), limit: MAX_HISTORY_UPLOAD });
-
-    let urlsByGUID = {};
-    for (let url of urls) {
-      let guid = await this.GUIDForUri(url, true);
-      urlsByGUID[guid] = url;
-    }
-    return urlsByGUID;
+  getAllIDs() {
+    return PlacesSyncUtils.history.getAllGuids({
+      since: new Date((Date.now() - THIRTY_DAYS_IN_MS)),
+      limit: MAX_HISTORY_UPLOAD,
+    });
   },
 
   async applyIncomingBatch(records) {
     let failed = [];
 
     // Convert incoming records to mozIPlaceInfo objects. Some records can be
     // ignored or handled directly, so we're rewriting the array in-place.
     let i, k;
--- a/services/sync/tests/unit/test_history_engine.js
+++ b/services/sync/tests/unit/test_history_engine.js
@@ -4,16 +4,321 @@
 Cu.import("resource://services-sync/service.js");
 Cu.import("resource://services-sync/engines/history.js");
 Cu.import("resource://testing-common/services/sync/utils.js");
 
 add_task(async function setup() {
   initTestLogging("Trace");
 });
 
+function startTracking() {
+  Svc.Obs.notify("weave:engine:start-tracking");
+}
+
+function stopTracking() {
+  Svc.Obs.notify("weave:engine:stop-tracking");
+}
+
+async function cleanup(engine) {
+  stopTracking();
+  await PlacesUtils.history.clear();
+  engine._tracker.clearChangedIDs();
+  engine._tracker.resetScore();
+  await engine.resetClient();
+  await engine.finalize();
+}
+
+add_task(async function test_history_duping() {
+  let engine = new HistoryEngine(Service);
+  await engine.initialize();
+
+  let server = await serverForFoo(engine);
+  await SyncTestingInfrastructure(server);
+
+  let now = Date.now();
+  let collection = server.user("foo").collection("history");
+
+  await PlacesUtils.history.insertMany([{
+    guid: "placeAAAAAA1",
+    url: "http://example.com/a",
+    title: "Page A (local)",
+    visits: [{
+      transition: PlacesUtils.history.TRANSITIONS.LINK,
+      date: new Date(now - 2500),
+    }, {
+      transition: PlacesUtils.history.TRANSITIONS.TYPED,
+      date: new Date(now - 5000),
+    }],
+  }, {
+    guid: "placeBBBBBBB",
+    url: "http://example.com/b",
+    visits: [{
+      transition: PlacesUtils.history.TRANSITIONS.DOWNLOAD,
+      date: new Date(now - 1250),
+    }, {
+      transition: PlacesUtils.history.TRANSITIONS.LINK,
+      date: new Date(now - 5000),
+    }],
+  }]);
+
+  let payloads = [{
+    id: "placeAAAAAAA",
+    histUri: "http://example.com/a",
+    title: "Page A (remote)",
+    visits: [{
+      type: PlacesUtils.history.TRANSITIONS.TYPED,
+      date: PlacesUtils.toPRTime(new Date(now - 5000)),
+    }, {
+      type: PlacesUtils.history.TRANSITIONS.LINK,
+      date: PlacesUtils.toPRTime(new Date(now - 7500)),
+    }],
+  }, {
+    id: "placeCCCCCCC",
+    histUri: "http://example.com/c",
+    title: "Page C (remote)",
+    visits: [{
+      type: PlacesUtils.history.TRANSITIONS.LINK,
+      date: PlacesUtils.toPRTime(new Date(now - 1000)),
+    }],
+  }];
+  for (let payload of payloads) {
+    collection.insert(payload.id, encryptPayload(payload));
+  }
+
+  engine.lastSync = 1;
+  let ping = await sync_engine_and_validate_telem(engine, false);
+  deepEqual(ping.engines[0].incoming, { applied: 2 });
+  ok(!ping.engines[0].outgoing);
+
+  let visitsForA = await PlacesUtils.history.fetch("http://example.com/a", { includeVisits: true });
+  equal(visitsForA.guid, "placeAAAAAAA", "Should dedupe local A1 to remote A");
+  equal(visitsForA.title, "Page A (remote)", "Should update local title for A");
+  deepEqual(visitsForA.visits, [{
+    transition: PlacesUtils.history.TRANSITIONS.LINK,
+    date: new Date(now - 2500),
+  }, {
+    transition: PlacesUtils.history.TRANSITIONS.TYPED,
+    date: new Date(now - 5000),
+  }, {
+    transition: PlacesUtils.history.TRANSITIONS.LINK,
+    date: new Date(now - 7500),
+  }], "Should merge visits for local A1 and remote A");
+
+  let visitsForA1 = await PlacesUtils.history.fetch("placeAAAAAA1");
+  ok(!visitsForA1, "A1 should not exists locally after deduping");
+
+  let placeForC = await PlacesUtils.history.fetch("http://example.com/c");
+  equal(placeForC.guid, "placeCCCCCCC", "Should create C locally");
+
+  // TODO(kitcambridge): Check server records here.
+
+  await cleanup(engine);
+  await promiseStopServer(server);
+});
+
+add_task(async function test_history_locally_modified_remotely_deleted() {
+  let engine = new HistoryEngine(Service);
+  await engine.initialize();
+
+  let server = await serverForFoo(engine);
+  await SyncTestingInfrastructure(server);
+
+  let now = Date.now();
+  let collection = server.user("foo").collection("history");
+
+  stopTracking();
+
+  _("Add untracked visits for A, B, C");
+  await PlacesUtils.history.insertMany([{
+    guid: "placeAAAAAAA",
+    url: "http://example.com/a",
+    visits: [{
+      transition: PlacesUtils.history.TRANSITIONS.LINK,
+      date: new Date(now - 2000),
+    }],
+  }, {
+    guid: "placeBBBBBBB",
+    url: "http://example.com/b",
+    visits: [{
+      transition: PlacesUtils.history.TRANSITIONS.LINK,
+      date: new Date(now - 4000),
+    }],
+  }, {
+    guid: "placeCCCCCCC",
+    url: "http://example.com/c",
+    visits: [{
+      transition: PlacesUtils.history.TRANSITIONS.LINK,
+      date: new Date(now - 6000),
+    }],
+  }]);
+
+  startTracking();
+
+  _("Add local visits for A, C");
+  await PlacesUtils.history.insertMany([{
+    guid: "placeAAAAAAA",
+    url: "http://example.com/a",
+    visits: [{
+      transition: PlacesUtils.history.TRANSITIONS.LINK,
+      date: new Date(now - 1000),
+    }],
+  }, {
+    guid: "placeCCCCCCC",
+    url: "http://example.com/c",
+    visits: [{
+      transition: PlacesUtils.history.TRANSITIONS.LINK,
+      date: new Date(now - 5000),
+    }],
+  }]);
+
+  let payloads = [{
+    // Deleted before new local visit, so we should delete the old local visit
+    // and keep the one we added above.
+    id: "placeAAAAAAA",
+    deleted: true,
+    deletedAt: PlacesUtils.toPRTime(new Date(now - 2000)),
+  }, {
+    // Deleted after local visit, so we should drop B entirely.
+    id: "placeBBBBBBB",
+    deleted: true,
+    deletedAt: PlacesUtils.toPRTime(new Date(now - 2000)),
+  }, {
+    // No `deletedAt` hint, so we should drop all visits for C, too.
+    id: "placeCCCCCCC",
+    deleted: true,
+  }];
+  for (let payload of payloads) {
+    collection.insert(payload.id, encryptPayload(payload));
+  }
+
+  engine.lastSync = 1;
+  let ping = await sync_engine_and_validate_telem(engine, false);
+  deepEqual(ping.engines[0].incoming, { reconciled: 2 });
+  equal(ping.engines[0].outgoing[0].sent, 1);
+
+  let visitsForA = await PlacesUtils.history.fetch("placeAAAAAAA", { includeVisits: true });
+  deepEqual(visitsForA.visits, [{
+    transition: PlacesUtils.history.TRANSITIONS.LINK,
+    date: new Date(now - 1000),
+  }], "Should keep new local visit for A");
+
+  let visitsForB = await PlacesUtils.history.fetch("placeBBBBBBB");
+  ok(!visitsForB, "Should drop all visits for B");
+
+  let visitsForC = await PlacesUtils.history.fetch("placeCCCCCCC");
+  ok(!visitsForC, "Should drop all visits for C");
+
+  await cleanup(engine);
+  await promiseStopServer(server);
+});
+
+add_task(async function test_history_locally_deleted_remotely_modified() {
+  let engine = new HistoryEngine(Service);
+  await engine.initialize();
+
+  let server = await serverForFoo(engine);
+  await SyncTestingInfrastructure(server);
+
+  let now = Date.now();
+  let collection = server.user("foo").collection("history");
+
+  stopTracking();
+
+  _("Insert local visits for A, B");
+  await PlacesUtils.history.insertMany([{
+    guid: "placeAAAAAAA",
+    url: "http://example.com/a",
+    visits: [{
+      type: PlacesUtils.history.TRANSITIONS.TYPED,
+      date: new Date(now - 5000),
+    }],
+  }, {
+    guid: "placeBBBBBBB",
+    url: "http://example.com/b",
+    visits: [{
+      type: PlacesUtils.history.TRANSITIONS.TYPED,
+      date: new Date(now - 1000),
+    }],
+  }]);
+
+  startTracking();
+
+  // Override `_now` to return a timestamp that's slightly older than the
+  // current time, so that we can insert new remote visits without clamping.
+  engine._tracker._now = () => {
+    return (now - 10) / 1000;
+  };
+
+  _("Delete all visits for A, B");
+  await PlacesUtils.history.remove(["placeAAAAAAA", "placeBBBBBBB"]);
+
+  _("Add visits for A, B on server");
+  let payloads = [{
+    id: "placeAAAAAAA",
+    histUri: "http://example.com/a",
+    title: "Page A (remote)",
+    visits: [{
+      type: PlacesUtils.history.TRANSITIONS.TYPED,
+      date: PlacesUtils.toPRTime(new Date(now - 5000)),
+    }, {
+      // A visit that we don't have locally, but that we should still ignore
+      // because it's older than the cleared time.
+      type: PlacesUtils.history.TRANSITIONS.DOWNLOAD,
+      date: PlacesUtils.toPRTime(new Date(now - 2500)),
+    }, {
+      // A visit that we should take, because it's newer than our tombstone.
+      type: PlacesUtils.history.TRANSITIONS.LINK,
+      date: PlacesUtils.toPRTime(new Date(now - 5)),
+    }],
+  }, {
+    id: "placeBBBBBBB",
+    histUri: "http://example.com/b",
+    title: "Page C (remote)",
+    visits: [{
+      type: PlacesUtils.history.TRANSITIONS.TYPED,
+      date: PlacesUtils.toPRTime(new Date(now - 1000)),
+    }, {
+      type: PlacesUtils.history.TRANSITIONS.TYPED,
+      date: PlacesUtils.toPRTime(new Date(now - 500)),
+    }],
+  }];
+  for (let payload of payloads) {
+    collection.insert(payload.id, encryptPayload(payload));
+  }
+
+  // On the first sync, `pullChanges` fetches all GUIDs from the store, instead
+  // of querying the tracker. This ignores tombstones, so we bump the last sync
+  // time to ensure they're seen during reconciliation.
+  engine.lastSync = 1;
+  let ping = await sync_engine_and_validate_telem(engine, false);
+  deepEqual(ping.engines[0].incoming, { reconciled: 2 });
+  equal(ping.engines[0].outgoing[0].sent, 2);
+
+  let visitsForA = await PlacesUtils.history.fetch("placeAAAAAAA", { includeVisits: true });
+  deepEqual(visitsForA.visits, [{
+    transition: PlacesUtils.history.TRANSITIONS.LINK,
+    date: new Date(now - 5),
+  }], "Should take visit newer than local tombstone");
+
+  let visitsForB = await PlacesUtils.history.fetch("placeBBBBBBB");
+  ok(!visitsForB, "Should drop all remote visits for B");
+
+  // TODO(kitcambridge): Check the server; make sure the tombstones we've
+  // uploaded have `deletedAt` set.
+
+  await cleanup(engine);
+  await promiseStopServer(server);
+});
+
+add_task(async function test_history_both_modified() {
+  // TODO(kitcambridge): Add visits locally and remotely. Add local-only;
+  // verify it's uploaded. Add remote-only, verify it's downloaded. For
+  // conflicts, verify we collapse dupes and merge visits.
+});
+
 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 +408,12 @@ 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 cleanup(engine);
+  await promiseStopServer(server);
 });
--- a/services/sync/tests/unit/test_history_tracker.js
+++ b/services/sync/tests/unit/test_history_tracker.js
@@ -140,17 +140,17 @@ add_task(async function test_start_track
 });
 
 add_task(async function test_track_delete() {
   _("Deletions are tracked.");
 
   // This isn't present because we weren't tracking when it was visited.
   await addVisit("track_delete");
   let uri = CommonUtils.makeURI("http://getfirefox.com/track_delete");
-  let guid = await engine._store.GUIDForUri(uri.spec);
+  let guid = await PlacesSyncUtils.history.fetchGuidForURL(uri.spec);
   await verifyTrackerEmpty();
 
   await startTracking();
   let visitRemovedPromise = promiseVisit("removed", uri);
   let scorePromise = promiseOneObserver("weave:engine:score:updated");
   await PlacesUtils.history.remove(uri);
   await Promise.all([scorePromise, visitRemovedPromise]);
 
@@ -158,17 +158,17 @@ add_task(async function test_track_delet
   do_check_eq(tracker.score, SCORE_INCREMENT_XLARGE);
 
   await cleanup();
 });
 
 add_task(async function test_dont_track_expiration() {
   _("Expirations are not tracked.");
   let uriToRemove = await addVisit("to_remove");
-  let guidToRemove = await engine._store.GUIDForUri(uriToRemove.spec);
+  let guidToRemove = await PlacesSyncUtils.history.fetchGuidForURL(uriToRemove.spec);
 
   await resetTracker();
   await verifyTrackerEmpty();
 
   await startTracking();
   let visitRemovedPromise = promiseVisit("removed", uriToRemove);
   let scorePromise = promiseOneObserver("weave:engine:score:updated");
 
@@ -212,29 +212,29 @@ add_task(async function test_stop_tracki
   await cleanup();
 });
 
 add_task(async function test_filter_hidden() {
   await startTracking();
 
   _("Add visit; should be hidden by the redirect");
   let hiddenURI = await addVisit("hidden");
-  let hiddenGUID = await engine._store.GUIDForUri(hiddenURI.spec);
+  let hiddenGUID = await PlacesSyncUtils.history.fetchGuidForURL(hiddenURI.spec);
   _(`Hidden visit GUID: ${hiddenGUID}`);
 
   _("Add redirect visit; should be tracked");
   let trackedURI = await addVisit("redirect", hiddenURI.spec,
     PlacesUtils.history.TRANSITION_REDIRECT_PERMANENT);
-  let trackedGUID = await engine._store.GUIDForUri(trackedURI.spec);
+  let trackedGUID = await PlacesSyncUtils.history.fetchGuidForURL(trackedURI.spec);
   _(`Tracked visit GUID: ${trackedGUID}`);
 
   _("Add visit for framed link; should be ignored");
   let embedURI = await addVisit("framed_link", null,
     PlacesUtils.history.TRANSITION_FRAMED_LINK);
-  let embedGUID = await engine._store.GUIDForUri(embedURI.spec);
+  let embedGUID = await PlacesSyncUtils.history.fetchGuidForURL(embedURI.spec);
   _(`Framed link visit GUID: ${embedGUID}`);
 
   _("Run Places maintenance to mark redirect visit as hidden");
   await PlacesDBUtils.maintenanceOnIdle();
 
   await verifyTrackedItems([trackedGUID]);
 
   await cleanup();
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -86,16 +86,19 @@ const HistorySyncUtils = PlacesSyncUtils
    * sensible date.
    *
    * @param {Date} visitDate
    *        The visit date.
    * @return {Date} The clamped visit date.
    */
   clampVisitDate(visitDate) {
     let currentDate = new Date();
+    if (!Number.isInteger(visitDate.getTime())) {
+      return currentDate;
+    }
     if (visitDate > currentDate) {
       return currentDate;
     }
     if (visitDate < BookmarkSyncUtils.EARLIEST_BOOKMARK_TIMESTAMP) {
       return new Date(BookmarkSyncUtils.EARLIEST_BOOKMARK_TIMESTAMP);
     }
     return visitDate;
   },
@@ -146,30 +149,30 @@ const HistorySyncUtils = PlacesSyncUtils
       nonSyncableGuids = nonSyncableGuids.concat(rows.map(row => row.getResultByName("guid")));
     }
     return nonSyncableGuids;
   },
 
   /**
    * Change the guid of the given uri
    *
-   * @param uri
-   * @param guid
+   * @param oldGuid
+   * @param newGuid
    */
-  changeGuid(uri, guid) {
-      let canonicalURL = PlacesUtils.SYNC_BOOKMARK_VALIDATORS.url(uri);
-      let validatedGuid = PlacesUtils.BOOKMARK_VALIDATORS.guid(guid);
-      return PlacesUtils.withConnectionWrapper("HistorySyncUtils: changeGuid",
-        async function(db) {
-          await db.executeCached(`
-            UPDATE moz_places
-            SET guid = :guid
-            WHERE url_hash = hash(:page_url) AND url = :page_url`,
-            {guid: validatedGuid, page_url: canonicalURL.href});
-        });
+  changeGuid(oldGuid, newGuid) {
+    PlacesUtils.BOOKMARK_VALIDATORS.guid(oldGuid);
+    PlacesUtils.BOOKMARK_VALIDATORS.guid(newGuid);
+    return PlacesUtils.withConnectionWrapper("HistorySyncUtils: changeGuid",
+      async function(db) {
+        await db.executeCached(`
+          UPDATE moz_places
+          SET guid = :newGuid
+          WHERE guid = :oldGuid`,
+          { newGuid, oldGuid });
+      });
   },
 
   /**
    * Fetch the last 20 visits (date and type of it) corresponding to a given url
    *
    * @param url
    * @returns {Array} Each element of the Array is an object with members: date and type
    */
@@ -237,40 +240,51 @@ const HistorySyncUtils = PlacesSyncUtils
 
   /**
    * Get all URLs filtered by the limit and since members of the options object.
    *
    * @param options
    *        Options object with two members, since and limit. Both of them must be provided
    * @returns {Array} - Up to limit number of URLs starting from the date provided by since
    */
-  async getAllURLs(options) {
+  async getAllGuids(options) {
     // Check that the limit property is finite number.
     if (!Number.isFinite(options.limit)) {
       throw new Error("The number provided in options.limit is not finite.");
     }
     // Check that the since property is of type Date.
     if (!options.since || Object.prototype.toString.call(options.since) != "[object Date]") {
       throw new Error("The property since of the options object must be of type Date.");
     }
     let db = await PlacesUtils.promiseDBConnection();
     let sinceInMicroseconds = PlacesUtils.toPRTime(options.since);
+    if (!Number.isFinite(sinceInMicroseconds)) {
+      throw new TypeError("Can't fetch GUIDs visited since an invalid date");
+    }
     let rows = await db.executeCached(`
-      SELECT DISTINCT p.url
+      SELECT p.guid, p.last_visit_date
       FROM moz_places p
       JOIN moz_historyvisits v ON p.id = v.place_id
       WHERE p.last_visit_date > :cutoff_date AND
             p.hidden = 0 AND
             v.visit_type NOT IN (0,
               ${PlacesUtils.history.TRANSITION_FRAMED_LINK})
-      ORDER BY frecency DESC
+      GROUP BY p.guid
+      ORDER BY p.frecency DESC
       LIMIT :max_results`,
       { cutoff_date: sinceInMicroseconds, max_results: options.limit }
     );
-    return rows.map(row => row.getResultByName("url"));
+    let changes = {};
+    for (let row of rows) {
+      let guid = row.getResultByName("guid");
+      let lastVisitAsPRTime = row.getResultByName("last_visit_date");
+      let modified = lastVisitAsPRTime / MICROSECONDS_PER_SECOND;
+      changes[guid] = modified;
+    }
+    return changes;
   },
 });
 
 const BookmarkSyncUtils = PlacesSyncUtils.bookmarks = Object.freeze({
   SMART_BOOKMARKS_ANNO: "Places/SmartBookmark",
   DESCRIPTION_ANNO: "bookmarkProperties/description",
   SIDEBAR_ANNO: "bookmarkProperties/loadInSidebar",
   SYNC_PARENT_ANNO: "sync/parent",