Bug 676563 - Add dateAdded field to synced bookmarks r?markh,rnewman draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Tue, 17 Jan 2017 14:45:08 -0500
changeset 496081 641509b2eb089a8839af03f7359425fd35d2817e
parent 496010 56fbe9964a0bccccb6b75a2be4190f52f7a0a502
child 548539 67701a11bd3bf702897cb9759fba231336bd0182
push id48519
push userbmo:tchiovoloni@mozilla.com
push dateThu, 09 Mar 2017 19:34:25 +0000
reviewersmarkh, rnewman
bugs676563
milestone55.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
@@ -758,16 +758,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"
@@ -1003,16 +1019,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() {
     return new Collection(this.engineURL, this._recordObj, this.service);
@@ -1546,25 +1563,25 @@ SyncEngine.prototype = {
                    item.id);
     return remoteIsNewer;
   },
 
   // Upload outgoing records.
   _uploadOutgoing() {
     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) {
@@ -1620,34 +1637,95 @@ SyncEngine.prototype = {
         } catch (ex) {
           this._log.warn("Error creating record", ex);
           ++counts.failed;
           if (Async.isShutdownException(ex) || !this.allowSkippedRecord) {
             Observers.notify("weave:engine:sync:uploaded", counts, this.name);
             throw 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 {
+        const { sent, failed } = this._weakReupload(up);
+        counts.sent += sent;
+        counts.failed += failed;
+      } 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) {
+    const counts = { sent: 0, failed: 0 };
+    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;
+      }
+    });
+
+    let pendingWeakReupload = this.buildWeakReuploadMap(this._needWeakReupload);
+    for (let [id, encodedRecord] of pendingWeakReupload) {
+      try {
+        this._log.trace("Outgoing (weak)", encodedRecord);
+        encodedRecord.encrypt(this.service.collectionKeys.keyForCollection(this.name));
+      } catch (ex) {
+        if (Async.isShutdownException(ex)) {
+          throw ex;
+        }
+        this._log.warn(`Failed to encrypt record "${id}" during weak reupload`, ex);
+        ++counts.failed;
+        continue;
+      }
+      // Note that general errors (network error, 412, etc.) will throw here.
+      // `enqueued` is only false if the specific item failed to enqueue, but
+      // other items should be/are fine. For example, enqueued will be false if
+      // it is larger than the max post or WBO size.
+      let { enqueued } = postQueue.enqueue(encodedRecord);
+      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() {
@@ -1673,16 +1751,17 @@ SyncEngine.prototype = {
           doDelete(key, val.slice(0, 100));
           val = val.slice(100);
         }
       }
     }
   },
 
   _syncCleanup() {
+    this._needWeakReupload.clear();
     if (!this._modified) {
       return;
     }
 
     try {
       // Mark failed WBOs as changed again so they are reuploaded next time.
       this.trackRemainingChanges();
     } finally {
@@ -1809,16 +1888,37 @@ SyncEngine.prototype = {
    * items that failed to upload. This method is called at the end of each sync.
    *
    */
   trackRemainingChanges() {
     for (let [id, change] of this._modified.entries()) {
       this._tracker.addChangedID(id, change);
     }
   },
+
+  /**
+   * Returns a map of (id, unencrypted record) that will be used to perform
+   * the weak reupload. Subclasses may override this to filter out items we
+   * shouldn't upload as part of a weak reupload (items that have changed,
+   * for example).
+   */
+  buildWeakReuploadMap(idSet) {
+    let result = new Map();
+    for (let id of idSet) {
+      try {
+        result.set(id, this._createRecord(id));
+      } catch (ex) {
+        if (Async.isShutdownException(ex)) {
+          throw ex;
+        }
+        this._log.warn("createRecord failed during weak reupload", ex);
+      }
+    }
+    return result;
+  }
 };
 
 /**
  * A changeset is created for each sync in `Engine::get{Changed, All}IDs`,
  * and stores opaque change data for tracked IDs. The default implementation
  * only records timestamps, though engines can extend this to store additional
  * data for each entry.
  */
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -115,34 +115,43 @@ 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 dateAdded = PlacesSyncUtils.bookmarks.ratchetTimestampBackwards(
+      this.dateAdded, +this.modified * 1000);
+    if (dateAdded !== undefined) {
+      result.dateAdded = dateAdded;
+    }
+    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",
 
@@ -538,16 +547,43 @@ BookmarksEngine.prototype = {
       // Make sure deleted items are marked as tombstones. We do this here
       // in addition to the `isTombstone` call above because it's possible
       // a changed bookmark might be deleted during a sync (bug 1313967).
       this._modified.setTombstone(record.id);
     }
     return record;
   },
 
+  buildWeakReuploadMap(idSet) {
+    // We want to avoid uploading records which have changed, since that could
+    // cause an inconsistent state on the server.
+    //
+    // Strictly speaking, it would be correct to just call getChangedIds() after
+    // building the initial weak reupload map, however this is quite slow, since
+    // we might end up doing createRecord() (which runs at least one, and
+    // sometimes multiple database queries) for a potentially large number of
+    // items.
+    //
+    // Since the call to getChangedIds is relatively cheap, we do it once before
+    // building the weakReuploadMap (which is where the calls to createRecord()
+    // occur) as an optimization, and once after for correctness, to handle the
+    // unlikely case that a record was modified while we were building the map.
+    let initialChanges = Async.promiseSpinningly(PlacesSyncUtils.bookmarks.getChangedIds());
+    for (let changed of initialChanges) {
+      idSet.delete(changed);
+    }
+
+    let map = SyncEngine.prototype.buildWeakReuploadMap.call(this, idSet);
+    let changes = Async.promiseSpinningly(PlacesSyncUtils.bookmarks.getChangedIds());
+    for (let id of changes) {
+      map.delete(id);
+    }
+    return map;
+  },
+
   _findDupe: function _findDupe(item) {
     this._log.trace("Finding dupe for " + item.id +
                     " (already duped: " + item.hasDupe + ").");
 
     // Don't bother finding a dupe if the incoming item has duplicates.
     if (item.hasDupe) {
       this._log.trace(item.id + " already a dupe: not finding one.");
       return null;
@@ -661,30 +697,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
@@ -118,16 +118,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);
   }
 }
 
@@ -155,17 +156,17 @@ add_task(async function test_dupe_bookma
       type: "bookmark",
       title: "Get Firefox!",
       parentName: "Folder 1",
       parentid: folder1_guid,
     };
 
     collection.insert(newGUID, encryptPayload(to_apply), Date.now() / 1000 + 500);
     _("Syncing so new dupe record is processed");
-    engine.lastSync = engine.lastSync - 0.01;
+    engine.lastSync = engine.lastSync - 5;
     engine.sync();
 
     // We should have logically deleted the dupe record.
     equal(collection.count(), 7);
     ok(getServerRecord(collection, bmk1_guid).deleted);
     // and physically removed from the local store.
     await promiseNoLocalItem(bmk1_guid);
     // Parent should still only have 1 item.
@@ -213,17 +214,17 @@ add_task(async function test_dupe_repare
       title: "Get Firefox!",
       parentName: "Folder 1",
       parentid: folder2_guid,
     };
 
     collection.insert(newGUID, encryptPayload(to_apply), Date.now() / 1000 + 500);
 
     _("Syncing so new dupe record is processed");
-    engine.lastSync = engine.lastSync - 0.01;
+    engine.lastSync = engine.lastSync - 5;
     engine.sync();
 
     // We should have logically deleted the dupe record.
     equal(collection.count(), 8);
     ok(getServerRecord(collection, bmk1_guid).deleted);
     // and physically removed from the local store.
     await promiseNoLocalItem(bmk1_guid);
     // The original folder no longer has the item
@@ -290,17 +291,17 @@ add_task(async function test_dupe_repare
     // to *not* apply the incoming record.
     await PlacesTestUtils.setBookmarkSyncFields({
       guid: bmk1_guid,
       syncChangeCounter: 1,
       lastModified: Date.now() + (deltaSeconds + 10) * 1000,
     });
 
     _("Syncing so new dupe record is processed");
-    engine.lastSync = engine.lastSync - 0.01;
+    engine.lastSync = engine.lastSync - 5;
     engine.sync();
 
     // We should have logically deleted the dupe record.
     equal(collection.count(), 8);
     ok(getServerRecord(collection, bmk1_guid).deleted);
     // and physically removed from the local store.
     await promiseNoLocalItem(bmk1_guid);
     // The original folder still longer has the item
@@ -381,17 +382,17 @@ add_task(async function test_dupe_repare
       title: "Get Firefox!",
       parentName: "Folder 1",
       parentid: newParentGUID,
       tags: [],
     }), Date.now() / 1000 + 500);
 
 
     _("Syncing so new records are processed.");
-    engine.lastSync = engine.lastSync - 0.01;
+    engine.lastSync = engine.lastSync - 5;
     engine.sync();
 
     // Everything should be parented correctly.
     equal((await getFolderChildrenIDs(folder1_id)).length, 0);
     let newParentID = store.idForGUID(newParentGUID);
     let newID = store.idForGUID(newGUID);
     deepEqual(await getFolderChildrenIDs(newParentID), [newID]);
 
@@ -457,17 +458,17 @@ add_task(async function test_dupe_repare
       title: "A second folder",
       parentName: "Bookmarks Toolbar",
       parentid: "toolbar",
       children: [newParentGUID],
       tags: [],
     }), Date.now() / 1000 + 500);
 
     _("Syncing so out-of-order records are processed.");
-    engine.lastSync = engine.lastSync - 0.01;
+    engine.lastSync = engine.lastSync - 5;
     engine.sync();
 
     // The intended parent did end up existing, so it should be parented
     // correctly after de-duplication.
     equal((await getFolderChildrenIDs(folder1_id)).length, 0);
     let newParentID = store.idForGUID(newParentGUID);
     let newID = store.idForGUID(newGUID);
     deepEqual(await getFolderChildrenIDs(newParentID), [newID]);
@@ -509,20 +510,21 @@ 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() - 10000
     }), Date.now() / 1000 + 500);
 
     _("Syncing so new dupe record is processed");
-    engine.lastSync = engine.lastSync - 0.01;
+    engine.lastSync = engine.lastSync - 5;
     engine.sync();
 
     // We should have logically deleted the dupe record.
     equal(collection.count(), 8);
     ok(getServerRecord(collection, bmk1_guid).deleted);
     // and physically removed from the local store.
     await promiseNoLocalItem(bmk1_guid);
     // The intended parent doesn't exist, so it remains in the original folder
@@ -552,30 +554,33 @@ add_task(async function test_dupe_repare
     collection.insert(newParentGUID, encryptPayload({
       id: newParentGUID,
       type: "folder",
       title: "Folder 1",
       parentName: "A second folder",
       parentid: folder2_guid,
       children: [newGUID],
       tags: [],
+      dateAdded: Date.now() - 10000,
     }), Date.now() / 1000 + 500);
     // We also queue an update to "folder 2" that references the new parent.
     collection.insert(folder2_guid, encryptPayload({
       id: folder2_guid,
       type: "folder",
       title: "A second folder",
       parentName: "Bookmarks Toolbar",
       parentid: "toolbar",
       children: [newParentGUID],
       tags: [],
+      dateAdded: Date.now() - 11000,
     }), Date.now() / 1000 + 500);
 
+
     _("Syncing so missing parent appears");
-    engine.lastSync = engine.lastSync - 0.01;
+    engine.lastSync = engine.lastSync - 5;
     engine.sync();
 
     // The intended parent now does exist, so it should have been reparented.
     equal((await getFolderChildrenIDs(folder1_id)).length, 0);
     let newParentID = store.idForGUID(newParentGUID);
     let newID = store.idForGUID(newGUID);
     deepEqual(await getFolderChildrenIDs(newParentID), [newID]);
 
@@ -621,17 +626,17 @@ add_task(async function test_dupe_empty_
       type: "folder",
       title: "Folder 1",
       parentName: "Bookmarks Toolbar",
       parentid: "toolbar",
       children: [],
     }), Date.now() / 1000 + 500);
 
     _("Syncing so new dupe records are processed");
-    engine.lastSync = engine.lastSync - 0.01;
+    engine.lastSync = engine.lastSync - 5;
     engine.sync();
 
     await validate(collection);
 
     // Collection now has one additional record - the logically deleted dupe.
     equal(collection.count(), 6);
     // original folder should be logically deleted.
     ok(getServerRecord(collection, folder1_guid).deleted);
--- a/services/sync/tests/unit/test_bookmark_engine.js
+++ b/services/sync/tests/unit/test_bookmark_engine.js
@@ -738,13 +738,172 @@ 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);
+
+    let item6GUID = "eeeeeeeeeeee";
+    let item6 = new Bookmark("bookmarks", item6GUID);
+    item6.bmkUri = "https://example.com/6";
+    item6.title = "asdf6";
+    item6.parentName = "Bookmarks Toolbar";
+    item6.parentid = "toolbar";
+    const item6LastModified = (now - oneYearMS) / 1000;
+    collection.insert(item6GUID, encryptPayload(item6.cleartext), item6LastModified);
+
+    let origBuildWeakReuploadMap = engine.buildWeakReuploadMap;
+    engine.buildWeakReuploadMap = set => {
+      let fullMap = origBuildWeakReuploadMap.call(engine, set);
+      fullMap.delete(item6GUID);
+      return fullMap;
+    };
+
+    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)");
+
+    let item6WBO = JSON.parse(JSON.parse(collection._wbos[item6GUID].payload).ciphertext);
+    ok(!item6WBO.dateAdded,
+       "If we think an item has been modified locally, we don't upload it to the server");
+
+    let record6 = await store.createRecord(item6GUID);
+    equal(record6.dateAdded, item6LastModified * 1000,
+       "We still remember the more accurate dateAdded if we don't upload a record due to local changes");
+    engine.buildWeakReuploadMap = origBuildWeakReuploadMap;
+
+    // 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, newRecord2.dateAdded,
+      "dateAdded update should be ignored for later date if we know an earlier one ");
+
+
+
+  } 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
@@ -837,16 +836,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.
@@ -894,22 +895,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
@@ -75,16 +75,21 @@ const HistorySyncUtils = PlacesSyncUtils
 
 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",
@@ -107,16 +112,29 @@ const BookmarkSyncUtils = PlacesSyncUtil
   /**
    * Converts a Sync record ID to a Places GUID.
    */
   syncIdToGuid(syncId) {
     return ROOT_SYNC_ID_TO_GUID[syncId] || syncId;
   },
 
   /**
+   * Resolves to an array of the syncIDs of bookmarks that have a nonzero change
+   * counter
+   */
+  getChangedIds: Task.async(function* () {
+    let db = yield PlacesUtils.promiseDBConnection();
+    let result = yield db.executeCached(`
+      SELECT guid FROM moz_bookmarks
+      WHERE syncChangeCounter >= 1`);
+    return result.map(row =>
+      BookmarkSyncUtils.guidToSyncId(row.getResultByName("guid")));
+  }),
+
+  /**
    * Fetches the sync IDs for a folder's children, ordered by their position
    * within the folder.
    */
   fetchChildSyncIds: Task.async(function* (parentSyncId) {
     PlacesUtils.SYNC_BOOKMARK_VALIDATORS.syncId(parentSyncId);
     let parentGuid = BookmarkSyncUtils.syncIdToGuid(parentSyncId);
 
     let db = yield PlacesUtils.promiseDBConnection();
@@ -627,16 +645,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): Timestamp in milliseconds, when the bookmark was added
+   *    or created 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
@@ -774,16 +794,29 @@ const BookmarkSyncUtils = PlacesSyncUtil
       UPDATE moz_bookmarks
         SET syncChangeCounter = syncChangeCounter + :syncChangeDelta
       WHERE type = :type AND
             fk = (SELECT id FROM moz_places WHERE url_hash = hash(:url) AND
                   url = :url)`,
       { syncChangeDelta, type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
         url: url.href });
   },
+
+  /**
+   * Returns `undefined` if no sensible timestamp could be found.
+   * Otherwise, returns the earliest sensible timestamp between `existingMillis`
+   * and `serverMillis`.
+   */
+  ratchetTimestampBackwards(existingMillis, serverMillis, lowerBound = BookmarkSyncUtils.EARLIEST_BOOKMARK_TIMESTAMP) {
+    const possible = [+existingMillis, +serverMillis].filter(n => !isNaN(n) && n > lowerBound);
+    if (!possible.length) {
+      return undefined;
+    }
+    return Math.min(...possible);
+  }
 });
 
 XPCOMUtils.defineLazyGetter(this, "BookmarkSyncLog", () => {
   return Log.repository.getLogger("BookmarkSyncUtils");
 });
 
 function validateSyncBookmarkObject(input, behavior) {
   return PlacesUtils.validateItemProperties(
@@ -1124,16 +1157,26 @@ 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 newDateAdded = BookmarkSyncUtils.ratchetTimestampBackwards(
+      oldBookmarkItem.dateAdded, updateInfo.dateAdded);
+    if (!newDateAdded || newDateAdded === oldBookmarkItem.dateAdded) {
+      delete updateInfo.dateAdded;
+    } else {
+      updateInfo.dateAdded = newDateAdded;
+    }
+  }
+
   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);
@@ -1150,16 +1193,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).
@@ -1315,16 +1361,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();
@@ -1434,16 +1481,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) {
@@ -1472,16 +1523,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
@@ -277,16 +277,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
@@ -1737,41 +1737,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");
   }
 
@@ -1780,27 +1781,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();
 });
 
@@ -1816,17 +1817,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();
   }