Bug 676563 - Add dateAdded field to synced bookmarks r?markh,rnewman draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Thu, 29 Dec 2016 13:31:27 -0500
changeset 454583 6b4d1058d4246ca60bd703ac24e50c7efddee8dc
parent 454575 12de9b2dacba8ee3125d09125a156d5ee985a6aa
child 540757 3624b142ffb3605c82cf101c8b89315cf9884fba
push id39983
push userbmo:tchiovoloni@mozilla.com
push dateThu, 29 Dec 2016 19:12:02 +0000
reviewersmarkh, rnewman
bugs676563
milestone53.0a1
Bug 676563 - Add dateAdded field to synced bookmarks r?markh,rnewman MozReview-Commit-ID: 5dxoTGrypqm
services/sync/modules/engines.js
services/sync/modules/engines/bookmarks.js
services/sync/tests/unit/test_bookmark_duping.js
services/sync/tests/unit/test_bookmark_engine.js
services/sync/tests/unit/test_bookmark_tracker.js
toolkit/components/places/Bookmarks.jsm
toolkit/components/places/PlacesSyncUtils.jsm
toolkit/components/places/PlacesUtils.jsm
toolkit/components/places/tests/bookmarks/test_bookmarks_insert.js
toolkit/components/places/tests/bookmarks/test_bookmarks_update.js
toolkit/components/places/tests/unit/test_sync_utils.js
--- a/services/sync/modules/engines.js
+++ b/services/sync/modules/engines.js
@@ -731,16 +731,32 @@ Engine.prototype = {
   }
 };
 
 this.SyncEngine = function SyncEngine(name, service) {
   Engine.call(this, name || "SyncEngine", service);
 
   this.loadToFetch();
   this.loadPreviousFailed();
+  // The set of records needing a weak reupload.
+  // The difference between this and a "normal" reupload is that these records
+  // are only tracked in memory, and if the reupload attempt fails (shutdown,
+  // 412, etc), we abort uploading the "weak" set.
+  //
+  // The rationale here is for the cases where we receive a record from the
+  // server that we know is wrong in some (small) way. For example, the
+  // dateAdded field on bookmarks -- maybe we have a better date, or the server
+  // record is entirely missing the date, etc.
+  //
+  // In these cases, we fix our local copy of the record, and mark it for
+  // weak reupload. A normal ("strong") reupload is problematic here because
+  // in the case of a conflict from the server, there's a window where our
+  // record would be marked as modified more recently than a change that occurs
+  // on another device change, and we lose data from the user.
+  this._needWeakReupload = new Set();
 }
 
 // Enumeration to define approaches to handling bad records.
 // Attached to the constructor to allow use as a kind of static enumeration.
 SyncEngine.kRecoveryStrategy = {
   ignore: "ignore",
   retry:  "retry",
   error:  "error"
@@ -901,17 +917,16 @@ SyncEngine.prototype = {
     let record = this._store.createRecord(id, this.name);
     record.id = id;
     record.collection = this.name;
     return record;
   },
 
   // Any setup that needs to happen at the beginning of each sync.
   _syncStartup: function () {
-
     // Determine if we need to wipe on outdated versions
     let metaGlobal = this.service.recordManager.get(this.metaURL);
     let engines = metaGlobal.payload.engines || {};
     let engineData = engines[this.name] || {};
 
     let needsWipe = false;
 
     // Assume missing versions are 0 and wipe the server
@@ -967,16 +982,17 @@ SyncEngine.prototype = {
     // to upload back.
     this._tracker.clearChangedIDs();
 
     this._log.info(this._modified.count() +
                    " outgoing items pre-reconciliation");
 
     // Keep track of what to delete at the end of sync
     this._delete = {};
+    this._needWeakReupload.clear();
   },
 
   /**
    * A tiny abstraction to make it easier to test incoming record
    * application.
    */
   itemSource: function () {
     return new Collection(this.engineURL, this._recordObj, this.service);
@@ -1510,25 +1526,25 @@ SyncEngine.prototype = {
                    item.id);
     return remoteIsNewer;
   },
 
   // Upload outgoing records.
   _uploadOutgoing: function () {
     this._log.trace("Uploading local changes to server.");
 
+    // collection we'll upload
+    let up = new Collection(this.engineURL, null, this.service);
     let modifiedIDs = this._modified.ids();
+    let counts = { failed: 0, sent: 0 };
     if (modifiedIDs.length) {
       this._log.trace("Preparing " + modifiedIDs.length +
                       " outgoing records");
 
-      let counts = { sent: modifiedIDs.length, failed: 0 };
-
-      // collection we'll upload
-      let up = new Collection(this.engineURL, null, this.service);
+      counts.sent = modifiedIDs.length;
 
       let failed = [];
       let successful = [];
       let handleResponse = (resp, batchOngoing = false) => {
         // Note: We don't want to update this.lastSync, or this._modified until
         // the batch is complete, however we want to remember success/failure
         // indicators for when that happens.
         if (!resp.success) {
@@ -1582,34 +1598,87 @@ SyncEngine.prototype = {
           out.encrypt(this.service.collectionKeys.keyForCollection(this.name));
           ok = true;
         } catch (ex) {
           if (Async.isShutdownException(ex)) {
             throw ex;
           }
           this._log.warn("Error creating record", ex);
         }
+        this._needWeakReupload.delete(id);
         if (ok) {
           let { enqueued, error } = postQueue.enqueue(out);
           if (!enqueued) {
             ++counts.failed;
             if (!this.allowSkippedRecord) {
               throw error;
             }
             this._modified.delete(id);
             this._log.warn(`Failed to enqueue record "${id}" (skipping)`, error);
           }
         }
         this._store._sleep(0);
       }
       postQueue.flush(true);
+    }
+
+    if (this._needWeakReupload.size) {
+      try {
+        this._weakReupload(up, counts);
+      } catch (e) {
+        if (Async.isShutdownException(e)) {
+          throw e;
+        }
+        this._log.warn("Weak reupload failed", e);
+      }
+    }
+    if (counts.sent || counts.failed) {
       Observers.notify("weave:engine:sync:uploaded", counts, this.name);
     }
   },
 
+  _weakReupload(collection, counts) {
+    let pendingSent = 0;
+    let postQueue = collection.newPostQueue(this._log, this.lastSync, (resp, batchOngoing = false) => {
+      if (!resp.success) {
+        this._needWeakReupload.clear();
+        this._log.warn("Uploading records (weak) failed: " + resp);
+        resp.failureCode = resp.status == 412 ? ENGINE_BATCH_INTERRUPTED : ENGINE_UPLOAD_FAIL;
+        throw resp;
+      }
+      if (!batchOngoing) {
+        counts.sent += pendingSent;
+        pendingSent = 0;
+      }
+    });
+
+    for (let id of this._needWeakReupload) {
+      let out;
+      try {
+        out = this._createRecord(id);
+        this._log.trace("Outgoing (weak)", out);
+        out.encrypt(this.service.collectionKeys.keyForCollection(this.name));
+      } catch (ex) {
+        if (Async.isShutdownException(ex)) {
+          throw ex;
+        }
+        continue;
+      }
+      let { enqueued } = postQueue.enqueue(out);
+      if (!enqueued) {
+        ++counts.failed;
+      } else {
+        ++pendingSent;
+      }
+      this._store._sleep(0);
+    }
+    postQueue.flush(true);
+    return counts;
+  },
+
   _onRecordsWritten(succeeded, failed) {
     // Implement this method to take specific actions against successfully
     // uploaded records and failed records.
   },
 
   // Any cleanup necessary.
   // Save the current snapshot so as to calculate changes at next sync
   _syncFinish: function () {
@@ -1635,16 +1704,17 @@ SyncEngine.prototype = {
           doDelete(key, val.slice(0, 100));
           val = val.slice(100);
         }
       }
     }
   },
 
   _syncCleanup: function () {
+    this._needWeakReupload.clear();
     if (!this._modified) {
       return;
     }
 
     try {
       // Mark failed WBOs as changed again so they are reuploaded next time.
       this.trackRemainingChanges();
     } finally {
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -115,34 +115,50 @@ PlacesItem.prototype = {
   },
 
   __proto__: CryptoWrapper.prototype,
   _logName: "Sync.Record.PlacesItem",
 
   // Converts the record to a Sync bookmark object that can be passed to
   // `PlacesSyncUtils.bookmarks.{insert, update}`.
   toSyncBookmark() {
-    return {
+    let result = {
       kind: this.type,
       syncId: this.id,
       parentSyncId: this.parentid,
     };
+    let date = this.dateAdded;
+    let serverModifiedDate = +this.modified * 1000;
+    if (!date || isNaN(date) || date >= serverModifiedDate) {
+      // Server modified date is an upper bound on the creation date, so
+      // `date >= serverModifiedDate` means that a client's clock was wrong at
+      // some point.
+      date = serverModifiedDate;
+    }
+    if (date && !isNaN(date) &&
+        date > PlacesSyncUtils.bookmarks.EARLIEST_BOOKMARK_TIMESTAMP) {
+      result.dateAdded = date;
+    }
+    return result;
   },
 
   // Populates the record from a Sync bookmark object returned from
   // `PlacesSyncUtils.bookmarks.fetch`.
   fromSyncBookmark(item) {
     this.parentid = item.parentSyncId;
     this.parentName = item.parentTitle;
+    if (item.dateAdded) {
+      this.dateAdded = item.dateAdded
+    }
   },
 };
 
 Utils.deferGetSet(PlacesItem,
                   "cleartext",
-                  ["hasDupe", "parentid", "parentName", "type"]);
+                  ["hasDupe", "parentid", "parentName", "type", "dateAdded"]);
 
 this.Bookmark = function Bookmark(collection, id, type) {
   PlacesItem.call(this, collection, id, type || "bookmark");
 }
 Bookmark.prototype = {
   __proto__: PlacesItem.prototype,
   _logName: "Sync.Record.Bookmark",
 
@@ -688,30 +704,36 @@ BookmarksStore.prototype = {
     let info = record.toSyncBookmark();
     // This can throw if we're inserting an invalid or incomplete bookmark.
     // That's fine; the exception will be caught by `applyIncomingBatch`
     // without aborting further processing.
     let item = Async.promiseSpinningly(PlacesSyncUtils.bookmarks.insert(info));
     if (item) {
       this._log.debug(`Created ${item.kind} ${item.syncId} under ${
         item.parentSyncId}`, item);
+      if (item.dateAdded != record.dateAdded) {
+        this.engine._needWeakReupload.add(item.syncId);
+      }
     }
   },
 
   remove: function BStore_remove(record) {
     this._log.trace(`Buffering removal of item "${record.id}".`);
     this._itemsToDelete.add(record.id);
   },
 
   update: function BStore_update(record) {
     let info = record.toSyncBookmark();
     let item = Async.promiseSpinningly(PlacesSyncUtils.bookmarks.update(info));
     if (item) {
       this._log.debug(`Updated ${item.kind} ${item.syncId} under ${
         item.parentSyncId}`, item);
+      if (item.dateAdded != record.dateAdded) {
+        this.engine._needWeakReupload.add(item.syncId);
+      }
     }
   },
 
   _orderChildren: function _orderChildren() {
     let promises = Object.keys(this._childrenToOrder).map(syncID => {
       let children = this._childrenToOrder[syncID];
       return PlacesSyncUtils.bookmarks.order(syncID, children).catch(ex => {
         this._log.debug(`Could not order children for ${syncID}`, ex);
--- a/services/sync/tests/unit/test_bookmark_duping.js
+++ b/services/sync/tests/unit/test_bookmark_duping.js
@@ -120,16 +120,17 @@ async function validate(collection, expe
   for (let elt of summary) {
     (isInExpectedFailures(elt) ? expected : unexpected).push(elt);
   }
   if (unexpected.length || expected.length != expectedFailures.length) {
     do_print("Validation failed:");
     do_print(JSON.stringify(summary));
     // print the entire validator output as it has IDs etc.
     do_print(JSON.stringify(problems, undefined, 2));
+    do_print("Expected: " + JSON.stringify(expectedFailures, undefined, 2));
     // All server records and the entire bookmark tree.
     do_print("Server records:\n" + JSON.stringify(collection.payloads(), undefined, 2));
     let tree = await PlacesUtils.promiseBookmarksTree("", { includeItemIds: true });
     do_print("Local bookmark tree:\n" + JSON.stringify(tree, undefined, 2));
     ok(false);
   }
 }
 
@@ -510,16 +511,17 @@ add_task(async function test_dupe_repare
     collection.insert(newGUID, encryptPayload({
       id: newGUID,
       bmkUri: "http://getfirefox.com/",
       type: "bookmark",
       title: "Get Firefox!",
       parentName: "Folder 1",
       parentid: newParentGUID,
       tags: [],
+      dateAdded: Date.now() - 1000
     }), Date.now() / 1000 + 10);
 
     _("Syncing so new dupe record is processed");
     engine.lastSync = engine.lastSync - 0.01;
     engine.sync();
 
     // We should have logically deleted the dupe record.
     equal(collection.count(), 8);
--- a/services/sync/tests/unit/test_bookmark_engine.js
+++ b/services/sync/tests/unit/test_bookmark_engine.js
@@ -742,13 +742,146 @@ add_task(async function test_misreconcil
   do_check_eq(store.GUIDForId(toolbarIDBefore), "toolbar");
   do_check_eq(parentGUIDBefore, parentGUIDAfter);
   do_check_eq(parentIDBefore, parentIDAfter);
 
   await PlacesSyncUtils.bookmarks.reset();
   await promiseStopServer(server);
 });
 
+add_task(async function test_sync_dateAdded() {
+  await Service.recordManager.clearCache();
+  await PlacesSyncUtils.bookmarks.reset();
+  let engine = new BookmarksEngine(Service);
+  let store  = engine._store;
+  let server = serverForFoo(engine);
+  await SyncTestingInfrastructure(server);
+
+  let collection = server.user("foo").collection("bookmarks");
+
+  Svc.Obs.notify("weave:engine:start-tracking");   // We skip usual startup...
+
+  // Just matters that it's in the past, not how far.
+  let now = Date.now();
+  let oneYearMS = 365 * 24 * 60 * 60 * 1000;
+
+  try {
+    let item1GUID = "abcdefabcdef";
+    let item1 = new Bookmark("bookmarks", item1GUID);
+    item1.bmkUri = "https://example.com";
+    item1.title = "asdf";
+    item1.parentName = "Bookmarks Toolbar";
+    item1.parentid = "toolbar";
+    item1.dateAdded = now - oneYearMS;
+    collection.insert(item1GUID, encryptPayload(item1.cleartext));
+
+    let item2GUID = "aaaaaaaaaaaa";
+    let item2 = new Bookmark("bookmarks", item2GUID);
+    item2.bmkUri = "https://example.com/2";
+    item2.title = "asdf2";
+    item2.parentName = "Bookmarks Toolbar";
+    item2.parentid = "toolbar";
+    item2.dateAdded = now + oneYearMS;
+    const item2LastModified = now / 1000 - 100;
+    collection.insert(item2GUID, encryptPayload(item2.cleartext), item2LastModified);
+
+    let item3GUID = "bbbbbbbbbbbb";
+    let item3 = new Bookmark("bookmarks", item3GUID);
+    item3.bmkUri = "https://example.com/3";
+    item3.title = "asdf3";
+    item3.parentName = "Bookmarks Toolbar";
+    item3.parentid = "toolbar";
+    // no dateAdded
+    collection.insert(item3GUID, encryptPayload(item3.cleartext));
+
+    let item4GUID = "cccccccccccc";
+    let item4 = new Bookmark("bookmarks", item4GUID);
+    item4.bmkUri = "https://example.com/4";
+    item4.title = "asdf4";
+    item4.parentName = "Bookmarks Toolbar";
+    item4.parentid = "toolbar";
+    // no dateAdded, but lastModified in past
+    const item4LastModified = (now - oneYearMS) / 1000;
+    collection.insert(item4GUID, encryptPayload(item4.cleartext), item4LastModified);
+
+    let item5GUID = "dddddddddddd";
+    let item5 = new Bookmark("bookmarks", item5GUID);
+    item5.bmkUri = "https://example.com/5";
+    item5.title = "asdf5";
+    item5.parentName = "Bookmarks Toolbar";
+    item5.parentid = "toolbar";
+    // no dateAdded, lastModified in (near) future.
+    const item5LastModified = (now + 60000) / 1000;
+    collection.insert(item5GUID, encryptPayload(item5.cleartext), item5LastModified);
+
+    await sync_engine_and_validate_telem(engine, false);
+
+    let record1 = await store.createRecord(item1GUID);
+    let record2 = await store.createRecord(item2GUID);
+
+    equal(item1.dateAdded, record1.dateAdded, "dateAdded in past should be synced");
+    equal(record2.dateAdded, item2LastModified * 1000, "dateAdded in future should be ignored in favor of last modified");
+
+    let record3 = await store.createRecord(item3GUID);
+
+    ok(record3.dateAdded);
+    // Make sure it's within 24 hours of the right timestamp... This is a little
+    // dodgey but we only really care that it's basically accurate and has the
+    // right day.
+    ok(Math.abs(Date.now() - record3.dateAdded) < 24 * 60 * 60 * 1000);
+
+    let record4 = await store.createRecord(item4GUID);
+    equal(record4.dateAdded, item4LastModified * 1000,
+          "If no dateAdded is provided, lastModified should be used");
+
+    let record5 = await store.createRecord(item5GUID);
+    equal(record5.dateAdded, item5LastModified * 1000,
+          "If no dateAdded is provided, lastModified should be used (even if it's in the future)");
+
+
+    // Update item2 and try resyncing it.
+    item2.dateAdded = now - 100000;
+    collection.insert(item2GUID, encryptPayload(item2.cleartext), now / 1000 - 50);
+
+
+    // Also, add a local bookmark and make sure it's date added makes it up to the server
+    let bzid = PlacesUtils.bookmarks.insertBookmark(
+      PlacesUtils.bookmarksMenuFolderId, Utils.makeURI("https://bugzilla.mozilla.org/"),
+      PlacesUtils.bookmarks.DEFAULT_INDEX, "Bugzilla");
+
+    let bzguid = await PlacesUtils.promiseItemGuid(bzid);
+
+
+    await sync_engine_and_validate_telem(engine, false);
+
+    let newRecord2 = await store.createRecord(item2GUID);
+    equal(newRecord2.dateAdded, item2.dateAdded, "dateAdded update should work for earlier date");
+
+    let bzWBO = JSON.parse(JSON.parse(collection._wbos[bzguid].payload).ciphertext);
+    ok(bzWBO.dateAdded, "Locally added dateAdded lost");
+
+    let localRecord = await store.createRecord(bzguid);
+    equal(bzWBO.dateAdded, localRecord.dateAdded, "dateAdded should not change during upload");
+
+    item2.dateAdded += 10000;
+    collection.insert(item2GUID, encryptPayload(item2.cleartext), now / 1000 - 10);
+    engine.lastSync = now / 1000 - 20;
+
+    await sync_engine_and_validate_telem(engine, false);
+
+    let newerRecord2 = await store.createRecord(item2GUID);
+    equal(newerRecord2.dateAdded, item2.dateAdded,
+      "dateAdded update should be respected for later date");
+
+  } finally {
+    store.wipe();
+    Svc.Prefs.resetBranch("");
+    Service.recordManager.clearCache();
+    await PlacesSyncUtils.bookmarks.reset();
+    await promiseStopServer(server);
+  }
+});
+
 function run_test() {
   initTestLogging("Trace");
   generateNewKeys(Service.collectionKeys);
   run_next_test();
 }
--- a/services/sync/tests/unit/test_bookmark_tracker.js
+++ b/services/sync/tests/unit/test_bookmark_tracker.js
@@ -37,16 +37,17 @@ async function verifyTrackerEmpty() {
 
 async function resetTracker() {
   await PlacesTestUtils.markBookmarksAsSynced();
   tracker.resetScore();
 }
 
 async function cleanup() {
   engine.lastSync = 0;
+  engine._needWeakReupload.clear()
   store.wipe();
   await resetTracker();
   await stopTracking();
 }
 
 // startTracking is a signal that the test wants to notice things that happen
 // after this is called (ie, things already tracked should be discarded.)
 async function startTracking() {
--- a/toolkit/components/places/Bookmarks.jsm
+++ b/toolkit/components/places/Bookmarks.jsm
@@ -154,33 +154,33 @@ var Bookmarks = Object.freeze({
    *        object representing a bookmark-item.
    *
    * @return {Promise} resolved when the creation is complete.
    * @resolves to an object representing the created bookmark.
    * @rejects if it's not possible to create the requested bookmark.
    * @throws if the arguments are invalid.
    */
   insert(info) {
-    // Ensure to use the same date for dateAdded and lastModified, even if
-    // dateAdded may be imposed by the caller.
-    let time = (info && info.dateAdded) || new Date();
+    let now = new Date();
+    let addedTime = (info && info.dateAdded) || now;
+    let modTime = addedTime;
+    if (addedTime > now) {
+      modTime = now;
+    }
     let insertInfo = validateBookmarkObject(info,
       { type: { defaultValue: this.TYPE_BOOKMARK }
       , index: { defaultValue: this.DEFAULT_INDEX }
       , url: { requiredIf: b => b.type == this.TYPE_BOOKMARK
              , validIf: b => b.type == this.TYPE_BOOKMARK }
       , parentGuid: { required: true }
       , title: { validIf: b => [ this.TYPE_BOOKMARK
                                , this.TYPE_FOLDER ].includes(b.type) }
-      , dateAdded: { defaultValue: time
-                   , validIf: b => !b.lastModified ||
-                                    b.dateAdded <= b.lastModified }
-      , lastModified: { defaultValue: time,
-                        validIf: b => (!b.dateAdded && b.lastModified >= time) ||
-                                      (b.dateAdded && b.lastModified >= b.dateAdded) }
+      , dateAdded: { defaultValue: addedTime }
+      , lastModified: { defaultValue: modTime,
+                        validIf: b => b.lastModified >= now || (b.dateAdded && b.lastModified >= b.dateAdded) }
       , source: { defaultValue: this.SOURCES.DEFAULT }
       });
 
     return Task.spawn(function* () {
       // Ensure the parent exists.
       let parent = yield fetchBookmark({ guid: insertInfo.parentGuid });
       if (!parent)
         throw new Error("parentGuid must be valid");
@@ -262,34 +262,33 @@ var Bookmarks = Object.freeze({
 
     return Task.spawn(function* () {
       // Ensure the item exists.
       let item = yield fetchBookmark(updateInfo);
       if (!item)
         throw new Error("No bookmarks found for the provided GUID");
       if (updateInfo.hasOwnProperty("type") && updateInfo.type != item.type)
         throw new Error("The bookmark type cannot be changed");
-      if (updateInfo.hasOwnProperty("dateAdded") &&
-          updateInfo.dateAdded.getTime() != item.dateAdded.getTime())
-        throw new Error("The bookmark dateAdded cannot be changed");
 
       // Remove any property that will stay the same.
       removeSameValueProperties(updateInfo, item);
       // Check if anything should still be updated.
       if (Object.keys(updateInfo).length < 3) {
         // Remove non-enumerable properties.
         return Object.assign({}, item);
       }
-
+      const now = new Date();
       updateInfo = validateBookmarkObject(updateInfo,
         { url: { validIf: () => item.type == this.TYPE_BOOKMARK }
         , title: { validIf: () => [ this.TYPE_BOOKMARK
                                   , this.TYPE_FOLDER ].includes(item.type) }
-        , lastModified: { defaultValue: new Date()
-                        , validIf: b => b.lastModified >= item.dateAdded }
+        , lastModified: { defaultValue: now
+                        , validIf: b => b.lastModified >= now ||
+                                        b.lastModified >= (b.dateAdded || item.dateAdded) }
+        , dateAdded: { defaultValue: item.dateAdded }
         });
 
       return PlacesUtils.withConnectionWrapper("Bookmarks.jsm: update",
         Task.async(function*(db) {
         let parent;
         if (updateInfo.hasOwnProperty("parentGuid")) {
           if (item.type == this.TYPE_FOLDER) {
             // Make sure we are not moving a folder into itself or one of its
@@ -833,16 +832,18 @@ function notify(observers, notification,
 function updateBookmark(info, item, newParent) {
   return PlacesUtils.withConnectionWrapper("Bookmarks.jsm: updateBookmark",
     Task.async(function*(db) {
 
     let tuples = new Map();
     tuples.set("lastModified", { value: PlacesUtils.toPRTime(info.lastModified) });
     if (info.hasOwnProperty("title"))
       tuples.set("title", { value: info.title });
+    if (info.hasOwnProperty("dateAdded"))
+      tuples.set("dateAdded", { value: PlacesUtils.toPRTime(info.dateAdded) });
 
     yield db.executeTransaction(function* () {
       let isTagging = item._grandParentId == PlacesUtils.tagsFolderId;
       let syncChangeDelta =
         PlacesSyncUtils.bookmarks.determineSyncChangeDelta(info.source);
 
       if (info.hasOwnProperty("url")) {
         // Ensure a page exists in moz_places for this URL.
@@ -890,22 +891,27 @@ function updateBookmark(info, item, newP
           yield setAncestorsLastModified(db, item.parentGuid, info.lastModified,
                                          syncChangeDelta);
         }
         yield setAncestorsLastModified(db, newParent.guid, info.lastModified,
                                        syncChangeDelta);
       }
 
       if (syncChangeDelta) {
-        let isChangingIndex = info.hasOwnProperty("index") &&
-                              info.index != item.index;
         // Sync stores child indices in the parent's record, so we only bump the
         // item's counter if we're updating at least one more property in
-        // addition to the index and last modified time.
-        let needsSyncChange = isChangingIndex ? tuples.size > 2 : tuples.size > 1;
+        // addition to the index, last modified time, and dateAdded
+        let sizeThreshold = 1;
+        if (info.hasOwnProperty("index") && info.index != item.index) {
+          ++sizeThreshold;
+        }
+        if (tuples.has("dateAdded")) {
+          ++sizeThreshold;
+        }
+        let needsSyncChange = tuples.size > sizeThreshold;
         if (needsSyncChange) {
           tuples.set("syncChangeDelta", { value: syncChangeDelta
                                         , fragment: "syncChangeCounter = syncChangeCounter + :syncChangeDelta" });
         }
       }
 
       if (isTagging) {
         // If we're updating a tag entry, bump the sync change counter for
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -59,16 +59,21 @@ XPCOMUtils.defineLazyGetter(this, "ROOTS
 
 const BookmarkSyncUtils = PlacesSyncUtils.bookmarks = Object.freeze({
   SMART_BOOKMARKS_ANNO: "Places/SmartBookmark",
   DESCRIPTION_ANNO: "bookmarkProperties/description",
   SIDEBAR_ANNO: "bookmarkProperties/loadInSidebar",
   SYNC_PARENT_ANNO: "sync/parent",
   SYNC_MOBILE_ROOT_ANNO: "mobile/bookmarksRoot",
 
+  // Jan 23, 1993 in milliseconds since 1970. Corresponds roughly to the release
+  // of the original NCSA Mosiac. We can safely assume that any dates before
+  // this time are invalid.
+  EARLIEST_BOOKMARK_TIMESTAMP: Date.UTC(1993, 0, 23),
+
   KINDS: {
     BOOKMARK: "bookmark",
     // Microsummaries were removed from Places in bug 524091. For now, Sync
     // treats them identically to bookmarks. Bug 745410 tracks removing them
     // entirely.
     MICROSUMMARY: "microsummary",
     QUERY: "query",
     FOLDER: "folder",
@@ -569,16 +574,18 @@ const BookmarkSyncUtils = PlacesSyncUtil
    * Fetches a Sync bookmark object for an item in the tree. The object contains
    * the following properties, depending on the item's kind:
    *
    *  - kind (all): A string representing the item's kind.
    *  - syncId (all): The item's sync ID.
    *  - parentSyncId (all): The sync ID of the item's parent.
    *  - parentTitle (all): The title of the item's parent, used for de-duping.
    *    Omitted for the Places root and parents with empty titles.
+   *  - dateAdded (all): Date the bookmark was added, or date created in sync
+   *    on a remote device if known.
    *  - title ("bookmark", "folder", "livemark", "query"): The item's title.
    *    Omitted if empty.
    *  - url ("bookmark", "query"): The item's URL.
    *  - tags ("bookmark", "query"): An array containing the item's tags.
    *  - keyword ("bookmark"): The bookmark's keyword, if one exists.
    *  - description ("bookmark", "folder", "livemark"): The item's description.
    *    Omitted if one isn't set.
    *  - loadInSidebar ("bookmark", "query"): Whether to load the bookmark in
@@ -1022,16 +1029,23 @@ var shouldReinsertLivemark = Task.async(
 var updateSyncBookmark = Task.async(function* (updateInfo) {
   let guid = BookmarkSyncUtils.syncIdToGuid(updateInfo.syncId);
   let oldBookmarkItem = yield PlacesUtils.bookmarks.fetch(guid);
   if (!oldBookmarkItem) {
     throw new Error(`Bookmark with sync ID ${
       updateInfo.syncId} does not exist`);
   }
 
+  if (updateInfo.hasOwnProperty("dateAdded")) {
+    let keepDateAdded = updateInfo.dateAdded > BookmarkSyncUtils.EARLIEST_BOOKMARK_TIMESTAMP;
+    if (!keepDateAdded) {
+      delete updateInfo.dateAdded;
+    }
+  }
+
   let shouldReinsert = false;
   let oldKind = yield getKindForItem(oldBookmarkItem);
   if (updateInfo.hasOwnProperty("kind") && updateInfo.kind != oldKind) {
     // If the item's aren't the same kind, we can't update the record;
     // we must remove and reinsert.
     shouldReinsert = true;
     if (BookmarkSyncLog.level <= Log.Level.Warn) {
       let oldSyncId = BookmarkSyncUtils.guidToSyncId(oldBookmarkItem.guid);
@@ -1048,16 +1062,19 @@ var updateSyncBookmark = Task.async(func
       let oldSyncId = BookmarkSyncUtils.guidToSyncId(oldBookmarkItem.guid);
       BookmarkSyncLog.debug(`updateSyncBookmark: Local ${
         oldSyncId} and remote ${
         updateInfo.syncId} livemarks have different URLs`);
     }
   }
 
   if (shouldReinsert) {
+    if (!updateInfo.hasOwnProperty("dateAdded")) {
+      updateInfo.dateAdded = oldBookmarkItem.dateAdded.getTime();
+    }
     let newInfo = validateNewBookmark(updateInfo);
     yield PlacesUtils.bookmarks.remove({
       guid,
       source: SOURCE_SYNC,
     });
     // A reinsertion likely indicates a confused client, since there aren't
     // public APIs for changing livemark URLs or an item's kind (e.g., turning
     // a folder into a separator while preserving its annos and position).
@@ -1221,16 +1238,17 @@ function validateNewBookmark(info) {
                                    , BookmarkSyncUtils.KINDS.QUERY
                                    , BookmarkSyncUtils.KINDS.FOLDER
                                    , BookmarkSyncUtils.KINDS.LIVEMARK ].includes(b.kind) }
     , loadInSidebar: { validIf: b => [ BookmarkSyncUtils.KINDS.BOOKMARK
                                      , BookmarkSyncUtils.KINDS.MICROSUMMARY
                                      , BookmarkSyncUtils.KINDS.QUERY ].includes(b.kind) }
     , feed: { validIf: b => b.kind == BookmarkSyncUtils.KINDS.LIVEMARK }
     , site: { validIf: b => b.kind == BookmarkSyncUtils.KINDS.LIVEMARK }
+    , dateAdded: { required: false }
     });
 
   return insertInfo;
 }
 
 // Returns an array of GUIDs for items that have an `anno` with the given `val`.
 var fetchGuidsWithAnno = Task.async(function* (anno, val) {
   let db = yield PlacesUtils.promiseDBConnection();
@@ -1340,16 +1358,20 @@ var placesBookmarkToSyncBookmark = Task.
         item.kind = yield getKindForItem(bookmarkItem);
         break;
 
       case "title":
       case "url":
         item[prop] = bookmarkItem[prop];
         break;
 
+      case "dateAdded":
+        item[prop] = new Date(bookmarkItem[prop]).getTime();
+        break;
+
       // Livemark objects contain additional properties. The feed URL is
       // required; the site URL is optional.
       case "feedURI":
         item.feed = new URL(bookmarkItem.feedURI.spec);
         break;
 
       case "siteURI":
         if (bookmarkItem.siteURI) {
@@ -1378,16 +1400,20 @@ function syncBookmarkToPlacesBookmark(in
         bookmarkInfo.type = getTypeForKind(info.kind);
         break;
 
       // Convert sync IDs to Places GUIDs for roots.
       case "syncId":
         bookmarkInfo.guid = BookmarkSyncUtils.syncIdToGuid(info.syncId);
         break;
 
+      case "dateAdded":
+        bookmarkInfo.dateAdded = new Date(info.dateAdded);
+        break;
+
       case "parentSyncId":
         bookmarkInfo.parentGuid =
           BookmarkSyncUtils.syncIdToGuid(info.parentSyncId);
         // Instead of providing an index, Sync reorders children at the end of
         // the sync using `BookmarkSyncUtils.order`. We explicitly specify the
         // default index here to prevent `PlacesUtils.bookmarks.update` and
         // `PlacesUtils.livemarks.addLivemark` from throwing.
         bookmarkInfo.index = PlacesUtils.bookmarks.DEFAULT_INDEX;
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -282,16 +282,18 @@ const SYNC_BOOKMARK_VALIDATORS = Object.
         throw new Error(`Invalid tag: ${tag}`);
       }
     }
     return v;
   },
   keyword: simpleValidateFunc(v => v === null || typeof v == "string"),
   description: simpleValidateFunc(v => v === null || typeof v == "string"),
   loadInSidebar: simpleValidateFunc(v => v === true || v === false),
+  dateAdded: simpleValidateFunc(v => typeof v === "number"
+    && v > PlacesSyncUtils.bookmarks.EARLIEST_BOOKMARK_TIMESTAMP),
   feed: v => v === null ? v : BOOKMARK_VALIDATORS.url(v),
   site: v => v === null ? v : BOOKMARK_VALIDATORS.url(v),
   title: BOOKMARK_VALIDATORS.title,
   url: BOOKMARK_VALIDATORS.url,
 });
 
 // Sync change records are passed between `PlacesSyncUtils` and the Sync
 // bookmarks engine, and are used to update an item's sync status and change
--- a/toolkit/components/places/tests/bookmarks/test_bookmarks_insert.js
+++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_insert.js
@@ -37,20 +37,17 @@ add_task(function* invalid_input_throws(
 
   Assert.throws(() => PlacesUtils.bookmarks.insert({ lastModified: -10 }),
                 /Invalid value for property 'lastModified'/);
   Assert.throws(() => PlacesUtils.bookmarks.insert({ lastModified: "today" }),
                 /Invalid value for property 'lastModified'/);
   Assert.throws(() => PlacesUtils.bookmarks.insert({ lastModified: Date.now() }),
                 /Invalid value for property 'lastModified'/);
   let time = new Date();
-  let future = new Date(time + 86400000);
-  Assert.throws(() => PlacesUtils.bookmarks.insert({ dateAdded: future,
-                                                     lastModified: time }),
-                /Invalid value for property 'dateAdded'/);
+
   let past = new Date(time - 86400000);
   Assert.throws(() => PlacesUtils.bookmarks.insert({ lastModified: past }),
                 /Invalid value for property 'lastModified'/);
 
   Assert.throws(() => PlacesUtils.bookmarks.insert({ type: -1 }),
                 /Invalid value for property 'type'/);
   Assert.throws(() => PlacesUtils.bookmarks.insert({ type: 100 }),
                 /Invalid value for property 'type'/);
--- a/toolkit/components/places/tests/bookmarks/test_bookmarks_update.js
+++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_update.js
@@ -98,32 +98,16 @@ add_task(function* invalid_properties_fo
                                          type: PlacesUtils.bookmarks.TYPE_FOLDER });
     Assert.ok(false, "Should have thrown");
   } catch (ex) {
     Assert.ok(/The bookmark type cannot be changed/.test(ex));
   }
 
   try {
     yield PlacesUtils.bookmarks.update({ guid: bm.guid,
-                                         dateAdded: new Date() });
-    Assert.ok(false, "Should have thrown");
-  } catch (ex) {
-    Assert.ok(/The bookmark dateAdded cannot be changed/.test(ex));
-  }
-
-  try {
-    yield PlacesUtils.bookmarks.update({ guid: bm.guid,
-                                         dateAdded: new Date() });
-    Assert.ok(false, "Should have thrown");
-  } catch (ex) {
-    Assert.ok(/The bookmark dateAdded cannot be changed/.test(ex));
-  }
-
-  try {
-    yield PlacesUtils.bookmarks.update({ guid: bm.guid,
                                          parentGuid: "123456789012",
                                          index: 1 });
     Assert.ok(false, "Should have thrown");
   } catch (ex) {
     Assert.ok(/No bookmarks found for the provided parentGuid/.test(ex));
   }
 
   let past = new Date(Date.now() - 86400000);
--- a/toolkit/components/places/tests/unit/test_sync_utils.js
+++ b/toolkit/components/places/tests/unit/test_sync_utils.js
@@ -1538,41 +1538,42 @@ add_task(function* test_fetch() {
     let item = yield PlacesSyncUtils.bookmarks.fetch(folder.syncId);
     deepEqual(item, {
       syncId: folder.syncId,
       kind: "folder",
       parentSyncId: "menu",
       description: "Folder description",
       childSyncIds: [folderBmk.syncId, folderSep.syncId],
       parentTitle: "Bookmarks Menu",
+      dateAdded: item.dateAdded,
       title: "",
     }, "Should include description, children, title, and parent title in folder");
   }
 
   do_print("Fetch bookmark with description, sidebar anno, and tags");
   {
     let item = yield PlacesSyncUtils.bookmarks.fetch(bmk.syncId);
     deepEqual(Object.keys(item).sort(), ["syncId", "kind", "parentSyncId",
-      "url", "tags", "description", "loadInSidebar", "parentTitle", "title"].sort(),
+      "url", "tags", "description", "loadInSidebar", "parentTitle", "title", "dateAdded"].sort(),
       "Should include bookmark-specific properties");
     equal(item.syncId, bmk.syncId, "Sync ID should match");
     equal(item.url.href, "https://example.com/", "Should return URL");
     equal(item.parentSyncId, "menu", "Should return parent sync ID");
     deepEqual(item.tags, ["taggy"], "Should return tags");
     equal(item.description, "Bookmark description", "Should return bookmark description");
     strictEqual(item.loadInSidebar, true, "Should return sidebar anno");
     equal(item.parentTitle, "Bookmarks Menu", "Should return parent title");
     strictEqual(item.title, "", "Should return empty title");
   }
 
   do_print("Fetch bookmark with keyword; without parent title or annos");
   {
     let item = yield PlacesSyncUtils.bookmarks.fetch(folderBmk.syncId);
     deepEqual(Object.keys(item).sort(), ["syncId", "kind", "parentSyncId",
-      "url", "keyword", "tags", "loadInSidebar", "parentTitle", "title"].sort(),
+      "url", "keyword", "tags", "loadInSidebar", "parentTitle", "title", "dateAdded"].sort(),
       "Should omit blank bookmark-specific properties");
     strictEqual(item.loadInSidebar, false, "Should not load bookmark in sidebar");
     deepEqual(item.tags, [], "Tags should be empty");
     equal(item.keyword, "kw", "Should return keyword");
     strictEqual(item.parentTitle, "", "Should include parent title even if empty");
     strictEqual(item.title, "", "Should include bookmark title even if empty");
   }
 
@@ -1581,27 +1582,27 @@ add_task(function* test_fetch() {
     let item = yield PlacesSyncUtils.bookmarks.fetch(folderSep.syncId);
     strictEqual(item.index, 1, "Should return separator position");
   }
 
   do_print("Fetch tag query");
   {
     let item = yield PlacesSyncUtils.bookmarks.fetch(tagQuery.syncId);
     deepEqual(Object.keys(item).sort(), ["syncId", "kind", "parentSyncId",
-      "url", "title", "folder", "parentTitle"].sort(),
+      "url", "title", "folder", "parentTitle", "dateAdded"].sort(),
       "Should include query-specific properties");
     equal(item.url.href, `place:type=7&folder=${tagFolderId}`, "Should not rewrite outgoing tag queries");
     equal(item.folder, "taggy", "Should return tag name for tag queries");
   }
 
   do_print("Fetch smart bookmark");
   {
     let item = yield PlacesSyncUtils.bookmarks.fetch(smartBmk.syncId);
     deepEqual(Object.keys(item).sort(), ["syncId", "kind", "parentSyncId",
-      "url", "title", "query", "parentTitle"].sort(),
+      "url", "title", "query", "parentTitle", "dateAdded"].sort(),
       "Should include smart bookmark-specific properties");
     equal(item.query, "BookmarksToolbar", "Should return query name for smart bookmarks");
   }
 
   yield PlacesUtils.bookmarks.eraseEverything();
   yield PlacesSyncUtils.bookmarks.reset();
 });
 
@@ -1617,17 +1618,17 @@ add_task(function* test_fetch_livemark()
       index: PlacesUtils.bookmarks.DEFAULT_INDEX,
     });
     PlacesUtils.annotations.setItemAnnotation(livemark.id, DESCRIPTION_ANNO,
       "Livemark description", 0, PlacesUtils.annotations.EXPIRE_NEVER);
 
     do_print("Fetch livemark");
     let item = yield PlacesSyncUtils.bookmarks.fetch(livemark.guid);
     deepEqual(Object.keys(item).sort(), ["syncId", "kind", "parentSyncId",
-      "description", "feed", "site", "parentTitle", "title"].sort(),
+      "description", "feed", "site", "parentTitle", "title", "dateAdded"].sort(),
       "Should include livemark-specific properties");
     equal(item.description, "Livemark description", "Should return description");
     equal(item.feed.href, site + "/feed/1", "Should return feed URL");
     equal(item.site.href, site + "/", "Should return site URL");
     strictEqual(item.title, "", "Should include livemark title even if empty");
   } finally {
     yield stopServer();
   }