diff --git a/services/sync/modules/engines.js b/services/sync/modules/engines.js index ad3549445bae..825d92fa3526 100644 --- a/services/sync/modules/engines.js +++ b/services/sync/modules/engines.js @@ -763,6 +763,22 @@ this.SyncEngine = function SyncEngine(name, 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. @@ -1008,6 +1024,7 @@ SyncEngine.prototype = { // Keep track of what to delete at the end of sync this._delete = {}; + this._needWeakReupload.clear(); }, /** @@ -1551,15 +1568,15 @@ SyncEngine.prototype = { _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 = []; @@ -1625,6 +1642,7 @@ SyncEngine.prototype = { throw ex; } } + this._needWeakReupload.delete(id); if (ok) { let { enqueued, error } = postQueue.enqueue(out); if (!enqueued) { @@ -1639,10 +1657,70 @@ SyncEngine.prototype = { 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. @@ -1678,6 +1756,7 @@ SyncEngine.prototype = { }, _syncCleanup() { + this._needWeakReupload.clear(); if (!this._modified) { return; } @@ -1814,6 +1893,27 @@ SyncEngine.prototype = { 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; + } }; /** diff --git a/services/sync/modules/engines/bookmarks.js b/services/sync/modules/engines/bookmarks.js index 51d6cf41181b..9c0adc8a0fe7 100644 --- a/services/sync/modules/engines/bookmarks.js +++ b/services/sync/modules/engines/bookmarks.js @@ -120,11 +120,17 @@ PlacesItem.prototype = { // 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 @@ -132,12 +138,15 @@ PlacesItem.prototype = { 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"); @@ -543,6 +552,33 @@ BookmarksEngine.prototype = { 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 + ")."); @@ -666,6 +702,9 @@ BookmarksStore.prototype = { 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); + } } }, @@ -680,6 +719,9 @@ BookmarksStore.prototype = { 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); + } } }, diff --git a/services/sync/tests/unit/test_bookmark_duping.js b/services/sync/tests/unit/test_bookmark_duping.js index b01af88921ef..6345eafa0e53 100644 --- a/services/sync/tests/unit/test_bookmark_duping.js +++ b/services/sync/tests/unit/test_bookmark_duping.js @@ -123,6 +123,7 @@ async function validate(collection, expectedFailures = []) { 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 }); @@ -160,7 +161,7 @@ add_task(async function test_dupe_bookmark() { 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. @@ -218,7 +219,7 @@ add_task(async function test_dupe_reparented_bookmark() { 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. @@ -295,7 +296,7 @@ add_task(async function test_dupe_reparented_locally_changed_bookmark() { }); _("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. @@ -386,7 +387,7 @@ add_task(async function test_dupe_reparented_to_earlier_appearing_parent_bookmar _("Syncing so new records are processed."); - engine.lastSync = engine.lastSync - 0.01; + engine.lastSync = engine.lastSync - 5; engine.sync(); // Everything should be parented correctly. @@ -462,7 +463,7 @@ add_task(async function test_dupe_reparented_to_later_appearing_parent_bookmark( }), 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 @@ -514,10 +515,11 @@ add_task(async function test_dupe_reparented_to_future_arriving_parent_bookmark( 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. @@ -557,6 +559,7 @@ add_task(async function test_dupe_reparented_to_future_arriving_parent_bookmark( 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({ @@ -567,10 +570,12 @@ add_task(async function test_dupe_reparented_to_future_arriving_parent_bookmark( 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. @@ -626,7 +631,7 @@ add_task(async function test_dupe_empty_folder() { }), 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); diff --git a/services/sync/tests/unit/test_bookmark_engine.js b/services/sync/tests/unit/test_bookmark_engine.js index 766ac1f0e661..1094546121dc 100644 --- a/services/sync/tests/unit/test_bookmark_engine.js +++ b/services/sync/tests/unit/test_bookmark_engine.js @@ -743,6 +743,165 @@ add_task(async function test_misreconciled_root() { 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); diff --git a/services/sync/tests/unit/test_bookmark_tracker.js b/services/sync/tests/unit/test_bookmark_tracker.js index ad3b25a13bf4..fa0cdd6c80e4 100644 --- a/services/sync/tests/unit/test_bookmark_tracker.js +++ b/services/sync/tests/unit/test_bookmark_tracker.js @@ -42,6 +42,7 @@ async function resetTracker() { async function cleanup() { engine.lastSync = 0; + engine._needWeakReupload.clear() store.wipe(); await resetTracker(); await stopTracking(); diff --git a/toolkit/components/places/Bookmarks.jsm b/toolkit/components/places/Bookmarks.jsm index 87fcb0df2890..e2d855e9a141 100644 --- a/toolkit/components/places/Bookmarks.jsm +++ b/toolkit/components/places/Bookmarks.jsm @@ -159,9 +159,12 @@ var Bookmarks = Object.freeze({ * @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 } @@ -170,12 +173,9 @@ var Bookmarks = Object.freeze({ , 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 } }); @@ -267,9 +267,6 @@ var Bookmarks = Object.freeze({ 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); @@ -278,13 +275,15 @@ var Bookmarks = Object.freeze({ // 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", @@ -842,6 +841,8 @@ function updateBookmark(info, item, newParent) { 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; @@ -899,12 +900,17 @@ function updateBookmark(info, item, newParent) { } 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" }); diff --git a/toolkit/components/places/PlacesSyncUtils.jsm b/toolkit/components/places/PlacesSyncUtils.jsm index 7f13ba0e7843..ffed4886e259 100644 --- a/toolkit/components/places/PlacesSyncUtils.jsm +++ b/toolkit/components/places/PlacesSyncUtils.jsm @@ -80,6 +80,11 @@ const BookmarkSyncUtils = PlacesSyncUtils.bookmarks = Object.freeze({ 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 @@ -111,6 +116,19 @@ const BookmarkSyncUtils = PlacesSyncUtils.bookmarks = Object.freeze({ 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. @@ -632,6 +650,8 @@ const BookmarkSyncUtils = PlacesSyncUtils.bookmarks = Object.freeze({ * - 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. @@ -779,6 +799,19 @@ const BookmarkSyncUtils = PlacesSyncUtils.bookmarks = Object.freeze({ { 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", () => { @@ -1129,6 +1162,16 @@ var updateSyncBookmark = Task.async(function* (updateInfo) { 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) { @@ -1155,6 +1198,9 @@ var updateSyncBookmark = Task.async(function* (updateInfo) { } if (shouldReinsert) { + if (!updateInfo.hasOwnProperty("dateAdded")) { + updateInfo.dateAdded = oldBookmarkItem.dateAdded.getTime(); + } let newInfo = validateNewBookmark(updateInfo); yield PlacesUtils.bookmarks.remove({ guid, @@ -1320,6 +1366,7 @@ function validateNewBookmark(info) { , 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; @@ -1439,6 +1486,10 @@ var placesBookmarkToSyncBookmark = Task.async(function* (bookmarkItem) { 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": @@ -1477,6 +1528,10 @@ function syncBookmarkToPlacesBookmark(info) { bookmarkInfo.guid = BookmarkSyncUtils.syncIdToGuid(info.syncId); break; + case "dateAdded": + bookmarkInfo.dateAdded = new Date(info.dateAdded); + break; + case "parentSyncId": bookmarkInfo.parentGuid = BookmarkSyncUtils.syncIdToGuid(info.parentSyncId); diff --git a/toolkit/components/places/PlacesUtils.jsm b/toolkit/components/places/PlacesUtils.jsm index d3a3a650e34e..7ba3830807f8 100644 --- a/toolkit/components/places/PlacesUtils.jsm +++ b/toolkit/components/places/PlacesUtils.jsm @@ -282,6 +282,8 @@ const SYNC_BOOKMARK_VALIDATORS = Object.freeze({ 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, diff --git a/toolkit/components/places/tests/bookmarks/test_bookmarks_insert.js b/toolkit/components/places/tests/bookmarks/test_bookmarks_insert.js index 9b1385b99c55..928100ab8167 100644 --- a/toolkit/components/places/tests/bookmarks/test_bookmarks_insert.js +++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_insert.js @@ -42,10 +42,7 @@ add_task(function* invalid_input_throws() { 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'/); diff --git a/toolkit/components/places/tests/bookmarks/test_bookmarks_update.js b/toolkit/components/places/tests/bookmarks/test_bookmarks_update.js index d077fd6f3d5f..15bae3cb1e71 100644 --- a/toolkit/components/places/tests/bookmarks/test_bookmarks_update.js +++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_update.js @@ -101,22 +101,6 @@ add_task(function* invalid_properties_for_existing_bookmark() { 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", diff --git a/toolkit/components/places/tests/unit/test_sync_utils.js b/toolkit/components/places/tests/unit/test_sync_utils.js index f182917483d0..49d56778f0d5 100644 --- a/toolkit/components/places/tests/unit/test_sync_utils.js +++ b/toolkit/components/places/tests/unit/test_sync_utils.js @@ -1742,6 +1742,7 @@ add_task(function* test_fetch() { 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"); } @@ -1750,7 +1751,7 @@ add_task(function* test_fetch() { { 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"); @@ -1766,7 +1767,7 @@ add_task(function* test_fetch() { { 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"); @@ -1785,7 +1786,7 @@ add_task(function* test_fetch() { { 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"); @@ -1795,7 +1796,7 @@ add_task(function* test_fetch() { { 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"); } @@ -1821,7 +1822,7 @@ add_task(function* test_fetch_livemark() { 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");