From 2449a744b8644a7ab1b348a3e51610831c40d0be Mon Sep 17 00:00:00 2001 From: Kit Cambridge Date: Tue, 19 Dec 2017 17:10:54 -0800 Subject: [PATCH] Bug 1305563 - Add a `BufferedBookmarksEngine` that can be toggled with a pref. r=markh,tcsc This patch adds a new bookmarks engine, pref'd off, that writes to the buffer instead of Places. MozReview-Commit-ID: 7idBa03kOzm --HG-- extra : rebase_source : 671ebeb3de824dc43c0ef14b2dfe065a33e2911a --- services/sync/modules/engines.js | 60 +- services/sync/modules/engines/bookmarks.js | 637 ++++++++++++------ services/sync/modules/service.js | 12 +- services/sync/services-sync.js | 1 + .../sync/tests/unit/test_bookmark_engine.js | 134 ++-- .../sync/tests/unit/test_bookmark_repair.js | 68 +- .../unit/test_bookmark_repair_responder.js | 4 +- .../sync/tests/unit/test_bookmark_tracker.js | 261 +------ services/sync/tests/unit/test_telemetry.js | 1 + tools/lint/eslint/modules.json | 2 +- 10 files changed, 665 insertions(+), 515 deletions(-) diff --git a/services/sync/modules/engines.js b/services/sync/modules/engines.js index 390f04955ab1..3202034119b2 100644 --- a/services/sync/modules/engines.js +++ b/services/sync/modules/engines.js @@ -895,6 +895,12 @@ SyncEngine.prototype = { /* * lastSync is a timestamp in server time. */ + async getLastSync() { + return this.lastSync; + }, + async setLastSync(lastSync) { + this.lastSync = lastSync; + }, get lastSync() { return this._lastSync; }, @@ -1029,13 +1035,7 @@ SyncEngine.prototype = { // the end of a sync, or after an error, we add all objects remaining in // this._modified to the tracker. this.lastSyncLocal = Date.now(); - let initialChanges; - if (this.lastSync) { - initialChanges = await this.pullNewChanges(); - } else { - this._log.debug("First sync, uploading all items"); - initialChanges = await this.pullAllChanges(); - } + let initialChanges = await this.pullChanges(); this._modified.replace(initialChanges); // Clear the tracker now. If the sync fails we'll add the ones we failed // to upload back. @@ -1049,6 +1049,15 @@ SyncEngine.prototype = { this._delete = {}; }, + async pullChanges() { + let lastSync = await this.getLastSync(); + if (lastSync) { + return this.pullNewChanges(); + } + this._log.debug("First sync, uploading all items"); + return this.pullAllChanges(); + }, + /** * A tiny abstraction to make it easier to test incoming record * application. @@ -1080,8 +1089,9 @@ SyncEngine.prototype = { this._log.trace("Downloading & applying server changes"); let newitems = this.itemSource(); + let lastSync = await this.getLastSync(); - newitems.newer = this.lastSync; + newitems.newer = lastSync; newitems.full = true; let downloadLimit = Infinity; @@ -1117,7 +1127,7 @@ SyncEngine.prototype = { let downloadedIDs = new Set(); // Stage 1: Fetch new records from the server, up to the download limit. - if (this.lastModified == null || this.lastModified > this.lastSync) { + if (this.lastModified == null || this.lastModified > lastSync) { let { response, records } = await newitems.getBatched(this.downloadBatchSize); if (!response.success) { response.failureCode = ENGINE_DOWNLOAD_FAIL; @@ -1164,7 +1174,7 @@ SyncEngine.prototype = { if (downloadedIDs.size == downloadLimit) { let guidColl = this.itemSource(); - guidColl.newer = this.lastSync; + guidColl.newer = lastSync; guidColl.older = oldestModified; guidColl.sort = "oldest"; @@ -1182,8 +1192,9 @@ SyncEngine.prototype = { // Fast-foward the lastSync timestamp since we have backlogged the // remaining items. - if (this.lastSync < this.lastModified) { - this.lastSync = this.lastModified; + if (lastSync < this.lastModified) { + lastSync = this.lastModified; + await this.setLastSync(lastSync); } // Stage 3: Backfill records from the backlog, and those that failed to @@ -1245,8 +1256,9 @@ SyncEngine.prototype = { this.toFetch = Utils.setDeleteAll(this.toFetch, ids); this.previousFailed = Utils.setAddAll(this.previousFailed, failedInBackfill); - if (this.lastSync < this.lastModified) { - this.lastSync = this.lastModified; + if (lastSync < this.lastModified) { + lastSync = this.lastModified; + await this.setLastSync(lastSync); } } @@ -1624,6 +1636,7 @@ SyncEngine.prototype = { let failed = []; let successful = []; + let lastSync = await this.getLastSync(); let handleResponse = async (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 @@ -1642,11 +1655,8 @@ SyncEngine.prototype = { // Nothing to do yet return; } - // Advance lastSync since we've finished the batch. - let modified = resp.headers["x-weave-timestamp"]; - if (modified > this.lastSync) { - this.lastSync = modified; - } + let serverModifiedTime = parseFloat(resp.headers["x-weave-timestamp"]); + if (failed.length && this._log.level <= Log.Level.Debug) { this._log.debug("Records that will be uploaded again because " + "the server couldn't store them: " @@ -1659,14 +1669,20 @@ SyncEngine.prototype = { this._modified.delete(id); } - await this._onRecordsWritten(successful, failed); + await this._onRecordsWritten(successful, failed, serverModifiedTime); + + // Advance lastSync since we've finished the batch. + if (serverModifiedTime > lastSync) { + lastSync = serverModifiedTime; + await this.setLastSync(lastSync); + } // clear for next batch failed.length = 0; successful.length = 0; }; - let postQueue = up.newPostQueue(this._log, this.lastSync, handleResponse); + let postQueue = up.newPostQueue(this._log, lastSync, handleResponse); for (let id of modifiedIDs) { let out; @@ -1717,7 +1733,7 @@ SyncEngine.prototype = { } }, - async _onRecordsWritten(succeeded, failed) { + async _onRecordsWritten(succeeded, failed, serverModifiedTime) { // Implement this method to take specific actions against successfully // uploaded records and failed records. }, diff --git a/services/sync/modules/engines/bookmarks.js b/services/sync/modules/engines/bookmarks.js index c023859c941a..75413ea51243 100644 --- a/services/sync/modules/engines/bookmarks.js +++ b/services/sync/modules/engines/bookmarks.js @@ -4,7 +4,8 @@ this.EXPORTED_SYMBOLS = ["BookmarksEngine", "PlacesItem", "Bookmark", "BookmarkFolder", "BookmarkQuery", - "Livemark", "BookmarkSeparator"]; + "Livemark", "BookmarkSeparator", + "BufferedBookmarksEngine"]; var Cc = Components.classes; var Ci = Components.interfaces; @@ -18,23 +19,26 @@ Cu.import("resource://services-sync/engines.js"); Cu.import("resource://services-sync/record.js"); Cu.import("resource://services-sync/util.js"); -XPCOMUtils.defineLazyModuleGetter(this, "BookmarkValidator", - "resource://services-sync/bookmark_validator.js"); +XPCOMUtils.defineLazyModuleGetters(this, { + SyncedBookmarksMirror: "resource://gre/modules/SyncedBookmarksMirror.jsm", + BookmarkValidator: "resource://services-sync/bookmark_validator.js", + OS: "resource://gre/modules/osfile.jsm", + PlacesBackups: "resource://gre/modules/PlacesBackups.jsm", + PlacesSyncUtils: "resource://gre/modules/PlacesSyncUtils.jsm", + PlacesUtils: "resource://gre/modules/PlacesUtils.jsm", + Resource: "resource://services-sync/resource.js", +}); + XPCOMUtils.defineLazyGetter(this, "PlacesBundle", () => { return Services.strings.createBundle("chrome://places/locale/places.properties"); }); -XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils", - "resource://gre/modules/PlacesUtils.jsm"); -XPCOMUtils.defineLazyModuleGetter(this, "PlacesSyncUtils", - "resource://gre/modules/PlacesSyncUtils.jsm"); -XPCOMUtils.defineLazyModuleGetter(this, "PlacesBackups", - "resource://gre/modules/PlacesBackups.jsm"); -const ANNOS_TO_TRACK = [PlacesSyncUtils.bookmarks.DESCRIPTION_ANNO, - PlacesSyncUtils.bookmarks.SIDEBAR_ANNO, - PlacesUtils.LMANNO_FEEDURI, PlacesUtils.LMANNO_SITEURI]; +XPCOMUtils.defineLazyGetter(this, "ANNOS_TO_TRACK", () => [ + PlacesSyncUtils.bookmarks.DESCRIPTION_ANNO, + PlacesSyncUtils.bookmarks.SIDEBAR_ANNO, PlacesUtils.LMANNO_FEEDURI, + PlacesUtils.LMANNO_SITEURI, +]); -const SERVICE_NOT_SUPPORTED = "Service not supported on this platform"; const FOLDER_SORTINDEX = 1000000; const { SOURCE_SYNC, @@ -43,10 +47,6 @@ const { SOURCE_SYNC_REPARENT_REMOVED_FOLDER_CHILDREN, } = Ci.nsINavBookmarksService; -const ORGANIZERQUERY_ANNO = "PlacesOrganizer/OrganizerQuery"; -const ALLBOOKMARKS_ANNO = "AllBookmarks"; -const MOBILE_ANNO = "MobileBookmarks"; - // Roots that should be deleted from the server, instead of applied locally. // This matches `AndroidBrowserBookmarksRepositorySession::forbiddenGUID`, // but allows tags because we don't want to reparent tag folders or tag items @@ -273,13 +273,53 @@ BookmarkSeparator.prototype = { Utils.deferGetSet(BookmarkSeparator, "cleartext", "pos"); -this.BookmarksEngine = function BookmarksEngine(service) { +/** + * The rest of this file implements two different bookmarks engines and stores. + * The `services.sync.engine.bookmarks.buffer` pref controls which one we use. + * `BaseBookmarksEngine` and `BaseBookmarksStore` define a handful of methods + * shared between the two implementations. + * + * `BookmarksEngine` and `BookmarksStore` pull locally changed IDs before + * syncing, examine every incoming record, use the default record-level + * reconciliation to resolve merge conflicts, and update records in Places + * using public APIs. This is similar to how the other sync engines work. + * + * Unfortunately, this general approach doesn't serve bookmark sync well. + * Bookmarks form a tree locally, but they're stored as opaque, encrypted, and + * unordered records on the server. The records are interdependent, with a + * set of constraints: each parent must know the IDs and order of its children, + * and a child can't appear in multiple parents. + * + * This has two important implications. + * + * First, some changes require us to upload multiple records. For example, + * moving a bookmark into a different folder uploads the bookmark, old folder, + * and new folder. + * + * Second, conflict resolution, like adding a bookmark to a folder on one + * device, and moving a different bookmark out of the same folder on a different + * device, must account for the tree structure. Otherwise, we risk uploading an + * incomplete tree, and confuse other devices that try to sync. + * + * Historically, the lack of durable change tracking and atomic uploads meant + * that we'd miss these changes entirely, or leave the server in an inconsistent + * state after a partial sync. Another device would then sync, download and + * apply the partial state directly to Places, and upload its changes. This + * could easily result in Sync scrambling bookmarks on both devices, and user + * intervention to manually undo the damage would make things worse. + * + * `BufferedBookmarksEngine` and `BufferedBookmarksStore` mitigate this by + * mirroring incoming bookmarks in a separate database, constructing trees from + * the local and remote bookmarks, and merging the two trees into a single + * consistent tree that accounts for every bookmark. For more information about + * merging, please see the explanation above `SyncedBookmarksMirror`. + */ +function BaseBookmarksEngine(service) { SyncEngine.call(this, "Bookmarks", service); -}; -BookmarksEngine.prototype = { +} +BaseBookmarksEngine.prototype = { __proto__: SyncEngine.prototype, _recordObj: PlacesItem, - _storeObj: BookmarksStore, _trackerObj: BookmarksTracker, version: 2, _defaultSort: "index", @@ -287,6 +327,70 @@ BookmarksEngine.prototype = { syncPriority: 4, allowSkippedRecord: false, + async _syncFinish() { + await SyncEngine.prototype._syncFinish.call(this); + await PlacesSyncUtils.bookmarks.ensureMobileQuery(); + }, + + async _createRecord(id) { + if (this._modified.isTombstone(id)) { + // If we already know a changed item is a tombstone, just create the + // record without dipping into Places. + return this._createTombstone(id); + } + let record = await SyncEngine.prototype._createRecord.call(this, id); + if (record.deleted) { + // 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; + }, + + async pullAllChanges() { + return this.pullNewChanges(); + }, + + async trackRemainingChanges() { + let changes = this._modified.changes; + await PlacesSyncUtils.bookmarks.pushChanges(changes); + }, + + _deleteId(id) { + this._noteDeletedId(id); + }, + + async _resetClient() { + await super._resetClient(); + await PlacesSyncUtils.bookmarks.reset(); + }, + + // Cleans up the Places root, reading list items (ignored in bug 762118, + // removed in bug 1155684), and pinned sites. + _shouldDeleteRemotely(incomingItem) { + return FORBIDDEN_INCOMING_IDS.includes(incomingItem.id) || + FORBIDDEN_INCOMING_PARENT_IDS.includes(incomingItem.parentid); + }, + + getValidator() { + return new BookmarkValidator(); + } +}; + +/** + * The original bookmarks engine. Uses an in-memory GUID map for deduping, and + * the default implementation for reconciling changes. Handles child ordering + * and deletions at the end of a sync. + */ +this.BookmarksEngine = function BookmarksEngine(service) { + BaseBookmarksEngine.apply(this, arguments); +}; + +BookmarksEngine.prototype = { + __proto__: BaseBookmarksEngine.prototype, + _storeObj: BookmarksStore, + emptyChangeset() { return new BookmarksChangeset(); }, @@ -522,34 +626,21 @@ BookmarksEngine.prototype = { this._store._childrenToOrder = {}; }, - async _syncFinish() { - await SyncEngine.prototype._syncFinish.call(this); - await PlacesSyncUtils.bookmarks.ensureMobileQuery(); - }, - async _syncCleanup() { await SyncEngine.prototype._syncCleanup.call(this); delete this._guidMap; }, async _createRecord(id) { - if (this._modified.isTombstone(id)) { - // If we already know a changed item is a tombstone, just create the - // record without dipping into Places. - return this._createTombstone(id); + let record = await super._createRecord(id); + if (record.deleted) { + return record; } - // Create the record as usual, but mark it as having dupes if necessary. - let record = await SyncEngine.prototype._createRecord.call(this, id); + // Mark the record as having dupes if necessary. let entry = await this._mapDupe(record); if (entry != null && entry.hasDupe) { record.hasDupe = true; } - if (record.deleted) { - // 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; }, @@ -569,28 +660,10 @@ BookmarksEngine.prototype = { return mapped ? mapped.toString() : mapped; }, - async pullAllChanges() { - return this.pullNewChanges(); - }, - async pullNewChanges() { return this._tracker.promiseChangedIDs(); }, - async trackRemainingChanges() { - let changes = this._modified.changes; - await PlacesSyncUtils.bookmarks.pushChanges(changes); - }, - - _deleteId(id) { - this._noteDeletedId(id); - }, - - async _resetClient() { - await SyncEngine.prototype._resetClient.call(this); - await PlacesSyncUtils.bookmarks.reset(); - }, - // Called when _findDupe returns a dupe item and the engine has decided to // switch the existing item to the new incoming item. async _switchItemToDupe(localDupeGUID, incomingItem) { @@ -600,13 +673,6 @@ BookmarksEngine.prototype = { this._modified.insert(newChanges); }, - // Cleans up the Places root, reading list items (ignored in bug 762118, - // removed in bug 1155684), and pinned sites. - _shouldDeleteRemotely(incomingItem) { - return FORBIDDEN_INCOMING_IDS.includes(incomingItem.id) || - FORBIDDEN_INCOMING_PARENT_IDS.includes(incomingItem.parentid); - }, - beforeRecordDiscard(localRecord, remoteRecord, remoteIsNewer) { if (localRecord.type != "folder" || remoteRecord.type != "folder") { return; @@ -629,18 +695,228 @@ BookmarksEngine.prototype = { this._log.debug("Recording children of " + localRecord.id, order); this._store._childrenToOrder[localRecord.id] = order; }, +}; - getValidator() { - return new BookmarkValidator(); +/** + * The buffered bookmarks engine uses a different store that stages downloaded + * bookmarks in a separate database, instead of writing directly to Places. The + * buffer handles reconciliation, so we stub out `_reconcile`, and wait to pull + * changes until we're ready to upload. + */ +this.BufferedBookmarksEngine = function BufferedBookmarksEngine() { + BaseBookmarksEngine.apply(this, arguments); +}; + +BufferedBookmarksEngine.prototype = { + __proto__: BaseBookmarksEngine.prototype, + _storeObj: BufferedBookmarksStore, + + async getLastSync() { + let mirror = await this._store.ensureOpenMirror(); + return mirror.getCollectionHighWaterMark(); + }, + + async setLastSync(lastSync) { + let mirror = await this._store.ensureOpenMirror(); + await mirror.setCollectionLastModified(lastSync); + // Update the pref so that reverting to the original bookmarks engine + // doesn't download records we've already applied. + super.lastSync = lastSync; + }, + + get lastSync() { + throw new TypeError("Use getLastSync"); + }, + + set lastSync(value) { + throw new TypeError("Use setLastSync"); + }, + + emptyChangeset() { + return new BufferedBookmarksChangeset(); + }, + + async _processIncoming(newitems) { + try { + await super._processIncoming(newitems); + } finally { + let buf = await this._store.ensureOpenMirror(); + let recordsToUpload = await buf.apply({ + remoteTimeSeconds: Resource.serverTime, + }); + this._modified.replace(recordsToUpload); + } + }, + + async _reconcile(item) { + return true; + }, + + async _createRecord(id) { + if (this._needWeakUpload.has(id)) { + return this._store.createRecord(id, this.name); + } + let change = this._modified.changes[id]; + if (!change) { + this._log.error("Creating record for item ${id} not in strong " + + "changeset", { id }); + throw new TypeError("Can't create record for unchanged item"); + } + let record = this._recordFromCleartext(id, change.cleartext); + record.sortindex = await this._store._calculateIndex(record); + return record; + }, + + _recordFromCleartext(id, cleartext) { + let recordObj = getTypeObject(cleartext.type); + if (!recordObj) { + this._log.warn("Creating record for item ${id} with unknown type ${type}", + { id, type: cleartext.type }); + recordObj = PlacesItem; + } + let record = new recordObj(this.name, id); + record.cleartext = cleartext; + return record; + }, + + async pullChanges() { + return {}; + }, + + /** + * Writes successfully uploaded records back to the mirror, so that the + * mirror matches the server. We update the mirror before updating Places, + * which has implications for interrupted syncs. + * + * 1. Sync interrupted during upload; server doesn't support atomic uploads. + * We'll download and reapply everything that we uploaded before the + * interruption. All locally changed items retain their change counters. + * 2. Sync interrupted during upload; atomic uploads enabled. The server + * discards the batch. All changed local items retain their change + * counters, so the next sync resumes cleanly. + * 3. Sync interrupted during upload; outgoing records can't fit in a single + * batch. We'll download and reapply all records through the most recent + * committed batch. This is a variation of (1). + * 4. Sync interrupted after we update the mirror, but before cleanup. The + * mirror matches the server, but locally changed items retain their change + * counters. Reuploading them on the next sync should be idempotent, though + * unnecessary. If another client makes a conflicting remote change before + * we sync again, we may incorrectly prefer the local state. + * 5. Sync completes successfully. We'll update the mirror, and reset the + * change counters for all items. + */ + async _onRecordsWritten(succeeded, failed, serverModifiedTime) { + let records = []; + for (let id of succeeded) { + let change = this._modified.changes[id]; + if (!change) { + // TODO (Bug 1433178): Write weakly uploaded records back to the mirror. + this._log.info("Uploaded record not in strong changeset", id); + continue; + } + if (!change.synced) { + this._log.info("Record in strong changeset not uploaded", id); + continue; + } + let cleartext = change.cleartext; + if (!cleartext) { + this._log.error("Missing Sync record cleartext for ${id} in ${change}", + { id, change }); + throw new TypeError("Missing cleartext for uploaded Sync record"); + } + let record = this._recordFromCleartext(id, cleartext); + record.modified = serverModifiedTime; + records.push(record); + } + let buf = await this._store.ensureOpenMirror(); + await buf.store(records, { needsMerge: false }); + }, + + async _resetClient() { + await super._resetClient(); + let buf = await this._store.ensureOpenMirror(); + await buf.reset(); + }, + + async finalize() { + await super.finalize(); + await this._store.finalize(); + }, +}; + +/** + * The only code shared between `BookmarksStore` and `BufferedBookmarksStore` + * is for creating Sync records from Places items. Everything else is + * different. + */ +function BaseBookmarksStore(name, engine) { + Store.call(this, name, engine); +} + +BaseBookmarksStore.prototype = { + __proto__: Store.prototype, + + // Create a record starting from the weave id (places guid) + async createRecord(id, collection) { + let item = await PlacesSyncUtils.bookmarks.fetch(id); + if (!item) { // deleted item + let record = new PlacesItem(collection, id); + record.deleted = true; + return record; + } + + let recordObj = getTypeObject(item.kind); + if (!recordObj) { + this._log.warn("Unknown item type, cannot serialize: " + item.kind); + recordObj = PlacesItem; + } + let record = new recordObj(collection, id); + record.fromSyncBookmark(item); + + record.sortindex = await this._calculateIndex(record); + + return record; + }, + + async _calculateIndex(record) { + // Ensure folders have a very high sort index so they're not synced last. + if (record.type == "folder") + return FOLDER_SORTINDEX; + + // For anything directly under the toolbar, give it a boost of more than an + // unvisited bookmark + let index = 0; + if (record.parentid == "toolbar") + index += 150; + + // Add in the bookmark's frecency if we have something. + if (record.bmkUri != null) { + let frecency = await PlacesSyncUtils.history.fetchURLFrecency(record.bmkUri); + if (frecency != -1) + index += frecency; + } + + return index; + }, + + async wipe() { + // Save a backup before clearing out all bookmarks. + await PlacesBackups.create(null, true); + await PlacesSyncUtils.bookmarks.wipe(); } }; -function BookmarksStore(name, engine) { - Store.call(this, name, engine); +/** + * The original store updates Places during the sync, using public methods. + * `BookmarksStore` implements all abstract `Store` methods, and behaves like + * the other stores. + */ +function BookmarksStore() { + BaseBookmarksStore.apply(this, arguments); this._itemsToDelete = new Set(); } BookmarksStore.prototype = { - __proto__: Store.prototype, + __proto__: BaseBookmarksStore.prototype, async itemExists(id) { return (await this.idForGUID(id)) > 0; @@ -777,29 +1053,6 @@ BookmarksStore.prototype = { this._itemsToDelete.clear(); }, - // Create a record starting from the weave id (places guid) - async createRecord(id, collection) { - let item = await PlacesSyncUtils.bookmarks.fetch(id); - if (!item) { // deleted item - let record = new PlacesItem(collection, id); - record.deleted = true; - return record; - } - - let recordObj = getTypeObject(item.kind); - if (!recordObj) { - this._log.warn("Unknown item type, cannot serialize: " + item.kind); - recordObj = PlacesItem; - } - let record = new recordObj(collection, id); - record.fromSyncBookmark(item); - - record.sortindex = await this._calculateIndex(record); - - return record; - }, - - async GUIDForId(id) { let guid = await PlacesUtils.promiseItemGuid(id); return PlacesSyncUtils.bookmarks.guidToRecordId(guid); @@ -816,35 +1069,74 @@ BookmarksStore.prototype = { } }, - async _calculateIndex(record) { - // Ensure folders have a very high sort index so they're not synced last. - if (record.type == "folder") - return FOLDER_SORTINDEX; - - // For anything directly under the toolbar, give it a boost of more than an - // unvisited bookmark - let index = 0; - if (record.parentid == "toolbar") - index += 150; - - // Add in the bookmark's frecency if we have something. - if (record.bmkUri != null) { - let frecency = await PlacesSyncUtils.history.fetchURLFrecency(record.bmkUri); - if (frecency != -1) - index += frecency; - } - - return index; - }, - async wipe() { this.clearPendingDeletions(); - // Save a backup before clearing out all bookmarks. - await PlacesBackups.create(null, true); - await PlacesSyncUtils.bookmarks.wipe(); + await super.wipe(); } }; +/** + * The buffered store delegates to the mirror for staging and applying + * records. Unlike `BookmarksStore`, `BufferedBookmarksStore` only + * implements `applyIncoming`, and `createRecord` via `BaseBookmarksStore`. + * These are the only two methods that `BufferedBookmarksEngine` calls during + * download and upload. + * + * The other `Store` methods intentionally remain abstract, so you can't use + * this store to create or update bookmarks in Places. All changes must go + * through the mirror, which takes care of merging and producing a valid tree. + */ +function BufferedBookmarksStore() { + BaseBookmarksStore.apply(this, arguments); +} + +BufferedBookmarksStore.prototype = { + __proto__: BaseBookmarksStore.prototype, + _openMirrorPromise: null, + + ensureOpenMirror() { + if (!this._openMirrorPromise) { + this._openMirrorPromise = this._openMirror().catch(err => { + // We may have failed to open the mirror temporarily; for example, if + // the database is locked. Clear the promise so that subsequent + // `ensureOpenMirror` calls can try to open the mirror again. + this._openMirrorPromise = null; + throw err; + }); + } + return this._openMirrorPromise; + }, + + async _openMirror() { + let mirrorPath = OS.Path.join(OS.Constants.Path.profileDir, "weave", + "bookmarks.sqlite"); + await OS.File.makeDir(OS.Path.dirname(mirrorPath), { + from: OS.Constants.Path.profileDir, + }); + + return SyncedBookmarksMirror.open({ + path: mirrorPath, + recordTelemetryEvent: (object, method, value, extra) => { + this.engine.service.recordTelemetryEvent(object, method, value, + extra); + }, + }); + }, + + async applyIncoming(record) { + let buf = await this.ensureOpenMirror(); + await buf.store([record]); + }, + + async finalize() { + if (!this._openMirrorPromise) { + return; + } + let buf = await this._openMirrorPromise; + await buf.finalize(); + }, +}; + // The bookmarks tracker is a special flower. Instead of listening for changes // via observer notifications, it queries Places for the set of items that have // changed since the last sync. Because it's a "pull-based" tracker, it ignores @@ -853,7 +1145,6 @@ BookmarksStore.prototype = { function BookmarksTracker(name, engine) { this._batchDepth = 0; this._batchSawScoreIncrement = false; - this._migratedOldEntries = false; Tracker.call(this, name, engine); } BookmarksTracker.prototype = { @@ -912,60 +1203,11 @@ BookmarksTracker.prototype = { throw new Error("Don't set initial changed bookmark IDs"); }, - // Migrates tracker entries from the old JSON-based tracker to Places. This - // is called the first time we start tracking changes. - async _migrateOldEntries() { - let existingIDs = await Utils.jsonLoad("changes/" + this.file, this); - if (existingIDs === null) { - // If the tracker file doesn't exist, we don't need to migrate, even if - // the engine is enabled. It's possible we're upgrading before the first - // sync. In the worst case, getting this wrong has the same effect as a - // restore: we'll reupload everything to the server. - this._log.debug("migrateOldEntries: Missing bookmarks tracker file; " + - "skipping migration"); - return null; - } - - if (!this._needsMigration()) { - // We have a tracker file, but bookmark syncing is disabled, or this is - // the first sync. It's likely the tracker file is stale. Remove it and - // skip migration. - this._log.debug("migrateOldEntries: Bookmarks engine disabled or " + - "first sync; skipping migration"); - return Utils.jsonRemove("changes/" + this.file, this); - } - - // At this point, we know the engine is enabled, we have a tracker file - // (though it may be empty), and we've synced before. - this._log.debug("migrateOldEntries: Migrating old tracker entries"); - let entries = []; - for (let id in existingIDs) { - let change = existingIDs[id]; - // Allow raw timestamps for backward-compatibility with changed IDs - // persisted before bug 1274496. - let timestamp = typeof change == "number" ? change : change.modified; - entries.push({ - recordId: id, - modified: timestamp * 1000, - }); - } - await PlacesSyncUtils.bookmarks.migrateOldTrackerEntries(entries); - return Utils.jsonRemove("changes/" + this.file, this); - }, - - _needsMigration() { - return this.engine && this.engineIsEnabled() && this.engine.lastSync > 0; - }, - observe: function observe(subject, topic, data) { Tracker.prototype.observe.call(this, subject, topic, data); switch (topic) { case "weave:engine:start-tracking": - if (!this._migratedOldEntries) { - this._migratedOldEntries = true; - Async.promiseSpinningly(this._migrateOldEntries()); - } break; case "bookmarks-restore-begin": this._log.debug("Ignoring changes from importing bookmarks."); @@ -1078,16 +1320,46 @@ BookmarksTracker.prototype = { onItemVisited() {} }; -class BookmarksChangeset extends Changeset { - - getStatus(id) { - let change = this.changes[id]; - if (!change) { - return PlacesUtils.bookmarks.SYNC_STATUS.UNKNOWN; - } - return change.status; +/** + * A changeset that stores extra metadata in a change record for each ID. The + * engine updates this metadata when uploading Sync records, and writes it back + * to Places in `BaseBookmarksEngine#trackRemainingChanges`. + * + * The `synced` property on a change record means its corresponding item has + * been uploaded, and we should pretend it doesn't exist in the changeset. + */ +class BufferedBookmarksChangeset extends Changeset { + // Only `_reconcile` calls `getModifiedTimestamp` and `has`, and the buffered + // engine does its own reconciliation. + getModifiedTimestamp(id) { + throw new Error("Don't use timestamps to resolve bookmark conflicts"); } + has(id) { + throw new Error("Don't use the changeset to resolve bookmark conflicts"); + } + + delete(id) { + let change = this.changes[id]; + if (change) { + // Mark the change as synced without removing it from the set. We do this + // so that we can update Places in `trackRemainingChanges`. + change.synced = true; + } + } + + ids() { + let results = new Set(); + for (let id in this.changes) { + if (!this.changes[id].synced) { + results.add(id); + } + } + return [...results]; + } +} + +class BookmarksChangeset extends BufferedBookmarksChangeset { getModifiedTimestamp(id) { let change = this.changes[id]; if (change) { @@ -1113,25 +1385,6 @@ class BookmarksChangeset extends Changeset { } } - delete(id) { - let change = this.changes[id]; - if (change) { - // Mark the change as synced without removing it from the set. We do this - // so that we can update Places in `trackRemainingChanges`. - change.synced = true; - } - } - - ids() { - let results = new Set(); - for (let id in this.changes) { - if (!this.changes[id].synced) { - results.add(id); - } - } - return [...results]; - } - isTombstone(id) { let change = this.changes[id]; if (change) { diff --git a/services/sync/modules/service.js b/services/sync/modules/service.js index 456aa7b5321f..1fa69cf0928a 100644 --- a/services/sync/modules/service.js +++ b/services/sync/modules/service.js @@ -39,7 +39,6 @@ Cu.import("resource://services-sync/util.js"); function getEngineModules() { let result = { Addons: {module: "addons.js", symbol: "AddonsEngine"}, - Bookmarks: {module: "bookmarks.js", symbol: "BookmarksEngine"}, Form: {module: "forms.js", symbol: "FormEngine"}, History: {module: "history.js", symbol: "HistoryEngine"}, Password: {module: "passwords.js", symbol: "PasswordEngine"}, @@ -59,6 +58,17 @@ function getEngineModules() { symbol: "CreditCardsEngine", }; } + if (Svc.Prefs.get("engine.bookmarks.buffer", false)) { + result.Bookmarks = { + module: "bookmarks.js", + symbol: "BufferedBookmarksEngine", + }; + } else { + result.Bookmarks = { + module: "bookmarks.js", + symbol: "BookmarksEngine", + }; + } return result; } diff --git a/services/sync/services-sync.js b/services/sync/services-sync.js index 7389412cf205..a13ba881eb02 100644 --- a/services/sync/services-sync.js +++ b/services/sync/services-sync.js @@ -25,6 +25,7 @@ pref("services.sync.errorhandler.networkFailureReportTimeout", 1209600); // 2 we pref("services.sync.engine.addons", true); pref("services.sync.engine.addresses", false); pref("services.sync.engine.bookmarks", true); +pref("services.sync.engine.bookmarks.buffer", false); pref("services.sync.engine.creditcards", false); pref("services.sync.engine.history", true); pref("services.sync.engine.passwords", true); diff --git a/services/sync/tests/unit/test_bookmark_engine.js b/services/sync/tests/unit/test_bookmark_engine.js index 8621a8bfeefa..7b4ffd4aa82b 100644 --- a/services/sync/tests/unit/test_bookmark_engine.js +++ b/services/sync/tests/unit/test_bookmark_engine.js @@ -38,11 +38,31 @@ add_task(async function setup() { await Service.engineManager.unregister("bookmarks"); }); -add_task(async function test_delete_invalid_roots_from_server() { +function add_bookmark_test(task) { + add_task(async function() { + _(`Running test ${task.name} with legacy bookmarks engine`); + let legacyEngine = new BookmarksEngine(Service); + await legacyEngine.initialize(); + try { + await task(legacyEngine); + } finally { + await legacyEngine.finalize(); + } + + _(`Running test ${task.name} with buffered bookmarks engine`); + let bufferedEngine = new BufferedBookmarksEngine(Service); + await bufferedEngine.initialize(); + try { + await task(bufferedEngine); + } finally { + await bufferedEngine.finalize(); + } + }); +} + +add_bookmark_test(async function test_delete_invalid_roots_from_server(engine) { _("Ensure that we delete the Places and Reading List roots from the server."); - let engine = new BookmarksEngine(Service); - await engine.initialize(); let store = engine._store; let server = await serverForFoo(engine); await SyncTestingInfrastructure(server); @@ -81,9 +101,13 @@ add_task(async function test_delete_invalid_roots_from_server() { await sync_engine_and_validate_telem(engine, false); - ok(!(await store.itemExists("readinglist")), "Should not apply Reading List root"); - ok(!(await store.itemExists(listBmk.id)), "Should not apply items in Reading List"); - ok((await store.itemExists(newBmk.id)), "Should apply new bookmark"); + await Assert.rejects(PlacesUtils.promiseItemId("readinglist"), + /no item found for the given GUID/, "Should not apply Reading List root"); + await Assert.rejects(PlacesUtils.promiseItemId(listBmk.id), + /no item found for the given GUID/, + "Should not apply items in Reading List"); + ok((await PlacesUtils.promiseItemId(newBmk.id)) > 0, + "Should apply new bookmark"); deepEqual(collection.keys().sort(), ["menu", "mobile", "toolbar", "unfiled", newBmk.id].sort(), "Should remove Places root and reading list items from server; upload local roots"); @@ -122,11 +146,9 @@ add_task(async function bad_record_allIDs() { await promiseStopServer(server); }); -add_task(async function test_processIncoming_error_orderChildren() { +add_bookmark_test(async function test_processIncoming_error_orderChildren(engine) { _("Ensure that _orderChildren() is called even when _processIncoming() throws an error."); - let engine = new BookmarksEngine(Service); - await engine.initialize(); let store = engine._store; let server = await serverForFoo(engine); await SyncTestingInfrastructure(server); @@ -152,6 +174,15 @@ add_task(async function test_processIncoming_error_orderChildren() { title: "Get Thunderbird!", }); + let toolbar_record = await store.createRecord("toolbar"); + collection.insert("toolbar", encryptPayload(toolbar_record.cleartext)); + + let bmk1_record = await store.createRecord(bmk1.guid); + collection.insert(bmk1.guid, encryptPayload(bmk1_record.cleartext)); + + let bmk2_record = await store.createRecord(bmk2.guid); + collection.insert(bmk2.guid, encryptPayload(bmk2_record.cleartext)); + // Create a server record for folder1 where we flip the order of // the children. let folder1_record = await store.createRecord(folder1.guid); @@ -169,7 +200,7 @@ add_task(async function test_processIncoming_error_orderChildren() { // Make the 10 minutes old so it will only be synced in the toFetch phase. bogus_record.modified = Date.now() / 1000 - 60 * 10; - engine.lastSync = Date.now() / 1000 - 60; + await engine.setLastSync(Date.now() / 1000 - 60); engine.toFetch = new SerializableSet([BOGUS_GUID]); let error; @@ -200,25 +231,23 @@ add_task(async function test_processIncoming_error_orderChildren() { } }); -add_task(async function test_restorePromptsReupload() { - await test_restoreOrImport(true); +add_bookmark_test(async function test_restorePromptsReupload(engine) { + await test_restoreOrImport(engine, { replace: true }); }); -add_task(async function test_importPromptsReupload() { - await test_restoreOrImport(false); +add_bookmark_test(async function test_importPromptsReupload(engine) { + await test_restoreOrImport(engine, { replace: false }); }); -// Test a JSON restore or HTML import. Use JSON if `aReplace` is `true`, or +// Test a JSON restore or HTML import. Use JSON if `replace` is `true`, or // HTML otherwise. -async function test_restoreOrImport(aReplace) { - let verb = aReplace ? "restore" : "import"; - let verbing = aReplace ? "restoring" : "importing"; - let bookmarkUtils = aReplace ? BookmarkJSONUtils : BookmarkHTMLUtils; +async function test_restoreOrImport(engine, { replace }) { + let verb = replace ? "restore" : "import"; + let verbing = replace ? "restoring" : "importing"; + let bookmarkUtils = replace ? BookmarkJSONUtils : BookmarkHTMLUtils; _(`Ensure that ${verbing} from a backup will reupload all records.`); - let engine = new BookmarksEngine(Service); - await engine.initialize(); let store = engine._store; let server = await serverForFoo(engine); await SyncTestingInfrastructure(server); @@ -278,10 +307,10 @@ async function test_restoreOrImport(aReplace) { Assert.equal(wbos[0], bmk2.guid); _(`Now ${verb} from a backup.`); - await bookmarkUtils.importFromFile(backupFilePath, aReplace); + await bookmarkUtils.importFromFile(backupFilePath, replace); let bookmarksCollection = server.user("foo").collection("bookmarks"); - if (aReplace) { + if (replace) { _("Verify that we wiped the server."); Assert.ok(!bookmarksCollection); } else { @@ -290,29 +319,30 @@ async function test_restoreOrImport(aReplace) { } _("Ensure we have the bookmarks we expect locally."); - let guids = await fetchAllRecordIds(); - _("GUIDs: " + JSON.stringify([...guids])); - let bookmarkGuids = new Map(); + let recordIds = await fetchAllRecordIds(); + _("GUIDs: " + JSON.stringify([...recordIds])); + + let bookmarkRecordIds = new Map(); let count = 0; - for (let guid of guids) { + for (let recordId of recordIds) { count++; let info = await PlacesUtils.bookmarks.fetch( - PlacesSyncUtils.bookmarks.recordIdToGuid(guid)); + PlacesSyncUtils.bookmarks.recordIdToGuid(recordId)); // Only one bookmark, so _all_ should be Firefox! if (info.type == PlacesUtils.bookmarks.TYPE_BOOKMARK) { - _(`Found URI ${info.url.href} for GUID ${guid}`); - bookmarkGuids.set(info.url.href, guid); + _(`Found URI ${info.url.href} for record ID ${recordId}`); + bookmarkRecordIds.set(info.url.href, recordId); } } - Assert.ok(bookmarkGuids.has("http://getfirefox.com/")); - if (!aReplace) { - Assert.ok(bookmarkGuids.has("http://getthunderbird.com/")); + Assert.ok(bookmarkRecordIds.has("http://getfirefox.com/")); + if (!replace) { + Assert.ok(bookmarkRecordIds.has("http://getthunderbird.com/")); } _("Have the correct number of IDs locally, too."); let expectedResults = ["menu", "toolbar", "mobile", "unfiled", folder1.guid, bmk1.guid]; - if (!aReplace) { + if (!replace) { expectedResults.push("toolbar", folder1.guid, bmk2.guid); } Assert.equal(count, expectedResults.length); @@ -343,18 +373,18 @@ async function test_restoreOrImport(aReplace) { }); let expectedFX = { - id: bookmarkGuids.get("http://getfirefox.com/"), + id: bookmarkRecordIds.get("http://getfirefox.com/"), bmkUri: "http://getfirefox.com/", title: "Get Firefox!" }; let expectedTB = { - id: bookmarkGuids.get("http://getthunderbird.com/"), + id: bookmarkRecordIds.get("http://getthunderbird.com/"), bmkUri: "http://getthunderbird.com/", title: "Get Thunderbird!" }; let expectedBookmarks; - if (aReplace) { + if (replace) { expectedBookmarks = [expectedFX]; } else { expectedBookmarks = [expectedTB, expectedFX]; @@ -366,7 +396,7 @@ async function test_restoreOrImport(aReplace) { let expectedFolder1 = { title: "Folder 1" }; let expectedFolders; - if (aReplace) { + if (replace) { expectedFolders = [expectedFolder1]; } else { expectedFolders = [expectedFolder1, expectedFolder1]; @@ -503,7 +533,7 @@ add_task(async function test_bookmark_guidMap_fail() { let itemPayload = itemRecord.cleartext; coll.insert(item.guid, encryptPayload(itemPayload)); - engine.lastSync = 1; // So we don't back up. + await engine.setLastSync(1); // So we don't back up. // Make building the GUID map fail. @@ -597,11 +627,9 @@ add_task(async function test_bookmark_tag_but_no_uri() { await store.update(record); }); -add_task(async function test_misreconciled_root() { +add_bookmark_test(async function test_misreconciled_root(engine) { _("Ensure that we don't reconcile an arbitrary record with a root."); - let engine = new BookmarksEngine(Service); - await engine.initialize(); let store = engine._store; let server = await serverForFoo(engine); await SyncTestingInfrastructure(server); @@ -618,10 +646,9 @@ add_task(async function test_misreconciled_root() { PlacesUtils.bookmarks.toolbarGuid); Assert.notEqual(-1, toolbarIDBefore); - let parentGUIDBefore = toolbarBefore.parentid; - let parentIDBefore = await PlacesUtils.promiseItemId( - PlacesSyncUtils.bookmarks.recordIdToGuid(parentGUIDBefore)); - Assert.notEqual(-1, parentIDBefore); + let parentRecordIDBefore = toolbarBefore.parentid; + let parentGUIDBefore = PlacesSyncUtils.bookmarks.recordIdToGuid(parentRecordIDBefore); + let parentIDBefore = await PlacesUtils.promiseItemId(parentGUIDBefore); Assert.equal("string", typeof(parentGUIDBefore)); _("Current parent: " + parentGUIDBefore + " (" + parentIDBefore + ")."); @@ -639,15 +666,16 @@ add_task(async function test_misreconciled_root() { let rec = new FakeRecord(BookmarkFolder, to_apply); _("Applying record."); - store.applyIncoming(rec); + store.applyIncomingBatch([rec]); // Ensure that afterwards, toolbar is still there. // As of 2012-12-05, this only passes because Places doesn't use "toolbar" as // the real GUID, instead using a generated one. Sync does the translation. let toolbarAfter = await store.createRecord("toolbar", "bookmarks"); - let parentGUIDAfter = toolbarAfter.parentid; - let parentIDAfter = await PlacesUtils.promiseItemId( - PlacesSyncUtils.bookmarks.recordIdToGuid(parentGUIDAfter)); + let parentRecordIDAfter = toolbarAfter.parentid; + let parentGUIDAfter = PlacesSyncUtils.bookmarks.recordIdToGuid( + parentRecordIDAfter); + let parentIDAfter = await PlacesUtils.promiseItemId(parentGUIDAfter); Assert.equal((await PlacesUtils.promiseItemGuid(toolbarIDBefore)), PlacesUtils.bookmarks.toolbarGuid); Assert.equal(parentGUIDBefore, parentGUIDAfter); @@ -659,11 +687,9 @@ add_task(async function test_misreconciled_root() { await promiseStopServer(server); }); -add_task(async function test_sync_dateAdded() { +add_bookmark_test(async function test_sync_dateAdded(engine) { await Service.recordManager.clearCache(); await PlacesSyncUtils.bookmarks.reset(); - let engine = new BookmarksEngine(Service); - await engine.initialize(); let store = engine._store; let server = await serverForFoo(engine); await SyncTestingInfrastructure(server); @@ -672,7 +698,7 @@ add_task(async function test_sync_dateAdded() { // TODO: Avoid random orange (bug 1374599), this is only necessary // intermittently - reset the last sync date so that we'll get all bookmarks. - engine.lastSync = 1; + await engine.setLastSync(1); Svc.Obs.notify("weave:engine:start-tracking"); // We skip usual startup... diff --git a/services/sync/tests/unit/test_bookmark_repair.js b/services/sync/tests/unit/test_bookmark_repair.js index 0f19bb81aa22..d17f84b4c081 100644 --- a/services/sync/tests/unit/test_bookmark_repair.js +++ b/services/sync/tests/unit/test_bookmark_repair.js @@ -5,6 +5,7 @@ // many mocks) Cu.import("resource://gre/modules/Log.jsm"); Cu.import("resource://gre/modules/osfile.jsm"); +Cu.import("resource://gre/modules/PlacesSyncUtils.jsm"); Cu.import("resource://services-sync/bookmark_repair.js"); Cu.import("resource://services-sync/constants.js"); Cu.import("resource://services-sync/doctor.js"); @@ -12,15 +13,9 @@ Cu.import("resource://services-sync/service.js"); Cu.import("resource://services-sync/engines/clients.js"); Cu.import("resource://services-sync/engines/bookmarks.js"); -const LAST_BOOKMARK_SYNC_PREFS = [ - "bookmarks.lastSync", - "bookmarks.lastSyncLocal", -]; - const BOOKMARK_REPAIR_STATE_PREFS = [ "client.GUID", "doctor.lastRepairAdvance", - ...LAST_BOOKMARK_SYNC_PREFS, ...Object.values(BookmarkRepairRequestor.PREF).map(name => `repairs.bookmarks.${name}` ), @@ -31,6 +26,9 @@ let bookmarksEngine; var recordedEvents = []; add_task(async function setup() { + await Service.engineManager.unregister("bookmarks"); + await Service.engineManager.register(BufferedBookmarksEngine); + clientsEngine = Service.clientsEngine; clientsEngine.ignoreLastModifiedOnProcessCommands = true; bookmarksEngine = Service.engineManager.get("bookmarks"); @@ -43,7 +41,9 @@ add_task(async function setup() { }); function checkRecordedEvents(expected, message) { - deepEqual(recordedEvents, expected, message); + // Ignore event telemetry from the merger. + let repairEvents = recordedEvents.filter(event => event.object != "mirror"); + deepEqual(repairEvents, expected, message); // and clear the list so future checks are easier to write. recordedEvents = []; } @@ -130,7 +130,16 @@ add_task(async function test_bookmark_repair_integration() { checkRecordedEvents([], "Should not start repair after first sync"); _("Back up last sync timestamps for remote client"); - let restoreRemoteLastBookmarkSync = backupPrefs(LAST_BOOKMARK_SYNC_PREFS); + let buf = await bookmarksEngine._store.ensureOpenMirror(); + let metaRows = await buf.db.execute(` + SELECT key, value FROM meta`); + let metaInfos = []; + for (let row of metaRows) { + metaInfos.push({ + key: row.getResultByName("key"), + value: row.getResultByName("value"), + }); + } _(`Delete ${bookmarkInfo.guid} locally and on server`); // Now we will reach into the server and hard-delete the bookmark @@ -140,9 +149,29 @@ add_task(async function test_bookmark_repair_integration() { await PlacesUtils.bookmarks.remove(bookmarkInfo.guid, { source: PlacesUtils.bookmarks.SOURCE_SYNC, }); - deepEqual((await bookmarksEngine.pullNewChanges()), {}, + deepEqual((await PlacesSyncUtils.bookmarks.pullChanges()), {}, `Should not upload tombstone for ${bookmarkInfo.guid}`); + // Remove the bookmark from the buffer, too. + let itemRows = await buf.db.execute(` + SELECT guid, kind, title, urlId + FROM items + WHERE guid = :guid`, + { guid: bookmarkInfo.guid }); + equal(itemRows.length, 1, `Bookmark ${ + bookmarkInfo.guid} should exist in buffer`); + let bufInfos = []; + for (let row of itemRows) { + bufInfos.push({ + guid: row.getResultByName("guid"), + kind: row.getResultByName("kind"), + title: row.getResultByName("title"), + urlId: row.getResultByName("urlId"), + }); + } + await buf.db.execute(`DELETE FROM items WHERE guid = :guid`, + { guid: bookmarkInfo.guid }); + // sync again - we should have a few problems... _("Sync again to trigger repair"); validationPromise = promiseValidationDone([ @@ -204,7 +233,14 @@ add_task(async function test_bookmark_repair_integration() { // repair instead of the sync. bookmarkInfo.source = PlacesUtils.bookmarks.SOURCE_SYNC; await PlacesUtils.bookmarks.insert(bookmarkInfo); - restoreRemoteLastBookmarkSync(); + await buf.db.execute(` + INSERT INTO items(guid, urlId, kind, title) + VALUES(:guid, :urlId, :kind, :title)`, + bufInfos); + await buf.db.execute(` + REPLACE INTO meta(key, value) + VALUES(:key, :value)`, + metaInfos); _("Sync as remote client"); await Service.sync(); @@ -350,9 +386,14 @@ add_task(async function test_repair_client_missing() { await PlacesUtils.bookmarks.remove(bookmarkInfo.guid, { source: PlacesUtils.bookmarks.SOURCE_SYNC, }); - // sanity check we aren't going to sync this removal. - do_check_empty((await bookmarksEngine.pullNewChanges())); - // sanity check that the bookmark is not there anymore + // Delete the bookmark from the buffer, too. + let buf = await bookmarksEngine._store.ensureOpenMirror(); + await buf.db.execute(`DELETE FROM items WHERE guid = :guid`, + { guid: bookmarkInfo.guid }); + + // Ensure we won't upload a tombstone for the removed bookmark. + Assert.deepEqual((await PlacesSyncUtils.bookmarks.pullChanges()), {}); + // Ensure the bookmark no longer exists in Places. Assert.equal(null, await PlacesUtils.bookmarks.fetch(bookmarkInfo.guid)); // sync again - we should have a few problems... @@ -481,6 +522,7 @@ add_task(async function test_repair_server_deleted() { // Now we will reach into the server and create a tombstone for that bookmark // but with a last-modified in the past - this way our sync isn't going to // pick up the record. + _(`Adding server tombstone for ${bookmarkInfo.guid}`); server.insertWBO("foo", "bookmarks", new ServerWBO(bookmarkInfo.guid, encryptPayload({ id: bookmarkInfo.guid, deleted: true, diff --git a/services/sync/tests/unit/test_bookmark_repair_responder.js b/services/sync/tests/unit/test_bookmark_repair_responder.js index a9c14629d23f..290b25364dcc 100644 --- a/services/sync/tests/unit/test_bookmark_repair_responder.js +++ b/services/sync/tests/unit/test_bookmark_repair_responder.js @@ -15,7 +15,9 @@ Svc.Prefs.set("engine.bookmarks.validation.enabled", false); var recordedEvents = []; function checkRecordedEvents(expected) { - deepEqual(recordedEvents, expected); + // Ignore event telemetry from the merger. + let repairEvents = recordedEvents.filter(event => event.object != "mirror"); + deepEqual(repairEvents, expected); // and clear the list so future checks are easier to write. recordedEvents = []; } diff --git a/services/sync/tests/unit/test_bookmark_tracker.js b/services/sync/tests/unit/test_bookmark_tracker.js index b96a59df0117..5dca565235ae 100644 --- a/services/sync/tests/unit/test_bookmark_tracker.js +++ b/services/sync/tests/unit/test_bookmark_tracker.js @@ -444,7 +444,7 @@ add_task(async function test_onItemAdded() { let syncFolderID = PlacesUtils.bookmarks.createFolder( PlacesUtils.bookmarks.bookmarksMenuFolder, "Sync Folder", PlacesUtils.bookmarks.DEFAULT_INDEX); - let syncFolderGUID = await engine._store.GUIDForId(syncFolderID); + let syncFolderGUID = await PlacesUtils.promiseItemGuid(syncFolderID); await verifyTrackedItems(["menu", syncFolderGUID]); Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE); @@ -456,7 +456,7 @@ add_task(async function test_onItemAdded() { CommonUtils.makeURI("https://example.org/sync"), PlacesUtils.bookmarks.DEFAULT_INDEX, "Sync Bookmark"); - let syncBmkGUID = await engine._store.GUIDForId(syncBmkID); + let syncBmkGUID = await PlacesUtils.promiseItemGuid(syncBmkID); await verifyTrackedItems([syncFolderGUID, syncBmkGUID]); Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE); @@ -467,7 +467,7 @@ add_task(async function test_onItemAdded() { let syncSepID = PlacesUtils.bookmarks.insertSeparator( PlacesUtils.bookmarks.bookmarksMenuFolder, PlacesUtils.bookmarks.getItemIndex(syncFolderID)); - let syncSepGUID = await engine._store.GUIDForId(syncSepID); + let syncSepGUID = await PlacesUtils.promiseItemGuid(syncSepID); await verifyTrackedItems(["menu", syncSepGUID]); Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE); } finally { @@ -568,7 +568,7 @@ add_task(async function test_onItemChanged_itemDates() { CommonUtils.makeURI("http://getfirefox.com"), PlacesUtils.bookmarks.DEFAULT_INDEX, "Get Firefox!"); - let fx_guid = await engine._store.GUIDForId(fx_id); + let fx_guid = await PlacesUtils.promiseItemGuid(fx_id); _(`Firefox GUID: ${fx_guid}`); await startTracking(); @@ -592,40 +592,6 @@ add_task(async function test_onItemChanged_itemDates() { } }); -add_task(async function test_onItemChanged_changeBookmarkURI() { - _("Changes to bookmark URIs should be tracked"); - - try { - await stopTracking(); - - _("Insert a bookmark"); - let bm = await PlacesUtils.bookmarks.insert({ - parentGuid: PlacesUtils.bookmarks.menuGuid, - url: "http://getfirefox.com", - title: "Get Firefox!" - }); - _(`Firefox GUID: ${bm.guid}`); - - _("Set a tracked annotation to make sure we only notify once"); - let id = await PlacesUtils.promiseItemId(bm.guid); - PlacesUtils.annotations.setItemAnnotation( - id, PlacesSyncUtils.bookmarks.DESCRIPTION_ANNO, "A test description", 0, - PlacesUtils.annotations.EXPIRE_NEVER); - - await startTracking(); - - _("Change the bookmark's URI"); - bm.url = "https://www.mozilla.org/firefox"; - bm = await PlacesUtils.bookmarks.update(bm); - - await verifyTrackedItems([bm.guid]); - Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE * 2); - } finally { - _("Clean up."); - await cleanup(); - } -}); - add_task(async function test_onItemTagged() { _("Items tagged using the synchronous API should be tracked"); @@ -636,7 +602,7 @@ add_task(async function test_onItemTagged() { let folder = PlacesUtils.bookmarks.createFolder( PlacesUtils.bookmarks.bookmarksMenuFolder, "Parent", PlacesUtils.bookmarks.DEFAULT_INDEX); - let folderGUID = await engine._store.GUIDForId(folder); + let folderGUID = await PlacesUtils.promiseItemGuid(folder); _("Folder ID: " + folder); _("Folder GUID: " + folderGUID); @@ -645,7 +611,7 @@ add_task(async function test_onItemTagged() { let b = PlacesUtils.bookmarks.insertBookmark( folder, uri, PlacesUtils.bookmarks.DEFAULT_INDEX, "Get Firefox!"); - let bGUID = await engine._store.GUIDForId(b); + let bGUID = await PlacesUtils.promiseItemGuid(b); _("New item is " + b); _("GUID: " + bGUID); @@ -674,12 +640,12 @@ add_task(async function test_onItemUntagged() { let fx1ID = PlacesUtils.bookmarks.insertBookmark( PlacesUtils.bookmarks.bookmarksMenuFolder, uri, PlacesUtils.bookmarks.DEFAULT_INDEX, "Get Firefox!"); - let fx1GUID = await engine._store.GUIDForId(fx1ID); + let fx1GUID = await PlacesUtils.promiseItemGuid(fx1ID); // Different parent and title; same URL. let fx2ID = PlacesUtils.bookmarks.insertBookmark( PlacesUtils.bookmarks.toolbarFolder, uri, PlacesUtils.bookmarks.DEFAULT_INDEX, "Download Firefox"); - let fx2GUID = await engine._store.GUIDForId(fx2ID); + let fx2GUID = await PlacesUtils.promiseItemGuid(fx2ID); PlacesUtils.tagging.tagURI(uri, ["foo"]); await startTracking(); @@ -809,7 +775,7 @@ add_task(async function test_onItemKeywordChanged() { let b = PlacesUtils.bookmarks.insertBookmark( folder, uri, PlacesUtils.bookmarks.DEFAULT_INDEX, "Get Firefox!"); - let bGUID = await engine._store.GUIDForId(b); + let bGUID = await PlacesUtils.promiseItemGuid(b); _("New item is " + b); _("GUID: " + bGUID); @@ -914,7 +880,7 @@ add_task(async function test_onItemPostDataChanged() { CommonUtils.makeURI("http://getfirefox.com"), PlacesUtils.bookmarks.DEFAULT_INDEX, "Get Firefox!"); - let fx_guid = await engine._store.GUIDForId(fx_id); + let fx_guid = await PlacesUtils.promiseItemGuid(fx_id); _(`Firefox GUID: ${fx_guid}`); await startTracking(); @@ -943,7 +909,7 @@ add_task(async function test_onItemAnnoChanged() { let b = PlacesUtils.bookmarks.insertBookmark( folder, CommonUtils.makeURI("http://getfirefox.com"), PlacesUtils.bookmarks.DEFAULT_INDEX, "Get Firefox!"); - let bGUID = await engine._store.GUIDForId(b); + let bGUID = await PlacesUtils.promiseItemGuid(b); _("New item is " + b); _("GUID: " + bGUID); @@ -977,7 +943,7 @@ add_task(async function test_onItemAdded_filtered_root() { PlacesUtils.bookmarks.placesRoot, "New root", PlacesUtils.bookmarks.DEFAULT_INDEX); - let rootGUID = await engine._store.GUIDForId(rootID); + let rootGUID = await PlacesUtils.promiseItemGuid(rootID); _(`New root GUID: ${rootGUID}`); _("Insert a bookmark underneath the new root"); @@ -986,7 +952,7 @@ add_task(async function test_onItemAdded_filtered_root() { CommonUtils.makeURI("http://getthunderbird.com"), PlacesUtils.bookmarks.DEFAULT_INDEX, "Get Thunderbird!"); - let untrackedBmkGUID = await engine._store.GUIDForId(untrackedBmkID); + let untrackedBmkGUID = await PlacesUtils.promiseItemGuid(untrackedBmkID); _(`New untracked bookmark GUID: ${untrackedBmkGUID}`); _("Insert a bookmark underneath the Places root"); @@ -994,7 +960,7 @@ add_task(async function test_onItemAdded_filtered_root() { PlacesUtils.bookmarks.placesRoot, CommonUtils.makeURI("http://getfirefox.com"), PlacesUtils.bookmarks.DEFAULT_INDEX, "Get Firefox!"); - let rootBmkGUID = await engine._store.GUIDForId(rootBmkID); + let rootBmkGUID = await PlacesUtils.promiseItemGuid(rootBmkID); _(`New Places root bookmark GUID: ${rootBmkGUID}`); _("New root and bookmark should be ignored"); @@ -1017,7 +983,7 @@ add_task(async function test_onItemDeleted_filtered_root() { PlacesUtils.bookmarks.placesRoot, CommonUtils.makeURI("http://getfirefox.com"), PlacesUtils.bookmarks.DEFAULT_INDEX, "Get Firefox!"); - let rootBmkGUID = await engine._store.GUIDForId(rootBmkID); + let rootBmkGUID = await PlacesUtils.promiseItemGuid(rootBmkID); _(`New Places root bookmark GUID: ${rootBmkGUID}`); await startTracking(); @@ -1170,14 +1136,14 @@ add_task(async function test_onItemMoved() { CommonUtils.makeURI("http://getfirefox.com"), PlacesUtils.bookmarks.DEFAULT_INDEX, "Get Firefox!"); - let fx_guid = await engine._store.GUIDForId(fx_id); + let fx_guid = await PlacesUtils.promiseItemGuid(fx_id); _("Firefox GUID: " + fx_guid); let tb_id = PlacesUtils.bookmarks.insertBookmark( PlacesUtils.bookmarks.bookmarksMenuFolder, CommonUtils.makeURI("http://getthunderbird.com"), PlacesUtils.bookmarks.DEFAULT_INDEX, "Get Thunderbird!"); - let tb_guid = await engine._store.GUIDForId(tb_id); + let tb_guid = await PlacesUtils.promiseItemGuid(tb_id); _("Thunderbird GUID: " + tb_guid); await startTracking(); @@ -1306,21 +1272,21 @@ add_task(async function test_onItemDeleted_removeFolderTransaction() { PlacesUtils.bookmarks.bookmarksMenuFolder, "Test folder", PlacesUtils.bookmarks.DEFAULT_INDEX); - let folder_guid = await engine._store.GUIDForId(folder_id); + let folder_guid = await PlacesUtils.promiseItemGuid(folder_id); _(`Folder GUID: ${folder_guid}`); let fx_id = PlacesUtils.bookmarks.insertBookmark( folder_id, CommonUtils.makeURI("http://getfirefox.com"), PlacesUtils.bookmarks.DEFAULT_INDEX, "Get Firefox!"); - let fx_guid = await engine._store.GUIDForId(fx_id); + let fx_guid = await PlacesUtils.promiseItemGuid(fx_id); _(`Firefox GUID: ${fx_guid}`); let tb_id = PlacesUtils.bookmarks.insertBookmark( folder_id, CommonUtils.makeURI("http://getthunderbird.com"), PlacesUtils.bookmarks.DEFAULT_INDEX, "Get Thunderbird!"); - let tb_guid = await engine._store.GUIDForId(tb_id); + let tb_guid = await PlacesUtils.promiseItemGuid(tb_id); _(`Thunderbird GUID: ${tb_guid}`); await startTracking(); @@ -1364,14 +1330,14 @@ add_task(async function test_treeMoved() { PlacesUtils.bookmarks.bookmarksMenuFolder, "First test folder", PlacesUtils.bookmarks.DEFAULT_INDEX); - let folder1_guid = await engine._store.GUIDForId(folder1_id); + let folder1_guid = await PlacesUtils.promiseItemGuid(folder1_id); // A second folder in the first. let folder2_id = PlacesUtils.bookmarks.createFolder( folder1_id, "Second test folder", PlacesUtils.bookmarks.DEFAULT_INDEX); - let folder2_guid = await engine._store.GUIDForId(folder2_id); + let folder2_guid = await PlacesUtils.promiseItemGuid(folder2_id); // Create a couple of bookmarks in the second folder. PlacesUtils.bookmarks.insertBookmark( @@ -1413,7 +1379,7 @@ add_task(async function test_onItemDeleted() { CommonUtils.makeURI("http://getthunderbird.com"), PlacesUtils.bookmarks.DEFAULT_INDEX, "Get Thunderbird!"); - let tb_guid = await engine._store.GUIDForId(tb_id); + let tb_guid = await PlacesUtils.promiseItemGuid(tb_id); await startTracking(); @@ -1553,7 +1519,7 @@ add_task(async function test_onItemDeleted_removeFolderChildren() { CommonUtils.makeURI("http://getfirefox.com"), PlacesUtils.bookmarks.DEFAULT_INDEX, "Get Firefox!"); - let fx_guid = await engine._store.GUIDForId(fx_id); + let fx_guid = await PlacesUtils.promiseItemGuid(fx_id); _(`Firefox GUID: ${fx_guid}`); let tb_id = PlacesUtils.bookmarks.insertBookmark( @@ -1561,7 +1527,7 @@ add_task(async function test_onItemDeleted_removeFolderChildren() { CommonUtils.makeURI("http://getthunderbird.com"), PlacesUtils.bookmarks.DEFAULT_INDEX, "Get Thunderbird!"); - let tb_guid = await engine._store.GUIDForId(tb_id); + let tb_guid = await PlacesUtils.promiseItemGuid(tb_id); _(`Thunderbird GUID: ${tb_guid}`); let moz_id = PlacesUtils.bookmarks.insertBookmark( @@ -1570,7 +1536,7 @@ add_task(async function test_onItemDeleted_removeFolderChildren() { PlacesUtils.bookmarks.DEFAULT_INDEX, "Mozilla" ); - let moz_guid = await engine._store.GUIDForId(moz_id); + let moz_guid = await PlacesUtils.promiseItemGuid(moz_id); _(`Mozilla GUID: ${moz_guid}`); await startTracking(); @@ -1595,14 +1561,14 @@ add_task(async function test_onItemDeleted_tree() { PlacesUtils.bookmarks.bookmarksMenuFolder, "First test folder", PlacesUtils.bookmarks.DEFAULT_INDEX); - let folder1_guid = await engine._store.GUIDForId(folder1_id); + let folder1_guid = await PlacesUtils.promiseItemGuid(folder1_id); // A second folder in the first. let folder2_id = PlacesUtils.bookmarks.createFolder( folder1_id, "Second test folder", PlacesUtils.bookmarks.DEFAULT_INDEX); - let folder2_guid = await engine._store.GUIDForId(folder2_id); + let folder2_guid = await PlacesUtils.promiseItemGuid(folder2_id); // Create a couple of bookmarks in the second folder. let fx_id = PlacesUtils.bookmarks.insertBookmark( @@ -1610,13 +1576,13 @@ add_task(async function test_onItemDeleted_tree() { CommonUtils.makeURI("http://getfirefox.com"), PlacesUtils.bookmarks.DEFAULT_INDEX, "Get Firefox!"); - let fx_guid = await engine._store.GUIDForId(fx_id); + let fx_guid = await PlacesUtils.promiseItemGuid(fx_id); let tb_id = PlacesUtils.bookmarks.insertBookmark( folder2_id, CommonUtils.makeURI("http://getthunderbird.com"), PlacesUtils.bookmarks.DEFAULT_INDEX, "Get Thunderbird!"); - let tb_guid = await engine._store.GUIDForId(tb_id); + let tb_guid = await PlacesUtils.promiseItemGuid(tb_id); await startTracking(); @@ -1630,170 +1596,3 @@ add_task(async function test_onItemDeleted_tree() { await cleanup(); } }); - -add_task(async function test_skip_migration() { - await insertBookmarksToMigrate(); - - let originalTombstones = await PlacesTestUtils.fetchSyncTombstones(); - let originalFields = await PlacesTestUtils.fetchBookmarkSyncFields( - "0gtWTOgYcoJD", "0dbpnMdxKxfg", "r5ouWdPB3l28", "YK5Bdq5MIqL6"); - - let filePath = OS.Path.join(OS.Constants.Path.profileDir, "weave", "changes", - "bookmarks.json"); - - _("No tracker file"); - { - await Utils.jsonRemove("changes/bookmarks", tracker); - ok(!(await OS.File.exists(filePath)), "Tracker file should not exist"); - - await tracker._migrateOldEntries(); - - let fields = await PlacesTestUtils.fetchBookmarkSyncFields( - "0gtWTOgYcoJD", "0dbpnMdxKxfg", "r5ouWdPB3l28", "YK5Bdq5MIqL6"); - deepEqual(fields, originalFields, - "Sync fields should not change if tracker file is missing"); - let tombstones = await PlacesTestUtils.fetchSyncTombstones(); - deepEqual(tombstones, originalTombstones, - "Tombstones should not change if tracker file is missing"); - } - - _("Existing tracker file; engine disabled"); - { - await Utils.jsonSave("changes/bookmarks", tracker, {}); - ok(await OS.File.exists(filePath), - "Tracker file should exist before disabled engine migration"); - - engine.disabled = true; - await tracker._migrateOldEntries(); - engine.disabled = false; - - let fields = await PlacesTestUtils.fetchBookmarkSyncFields( - "0gtWTOgYcoJD", "0dbpnMdxKxfg", "r5ouWdPB3l28", "YK5Bdq5MIqL6"); - deepEqual(fields, originalFields, - "Sync fields should not change on disabled engine migration"); - let tombstones = await PlacesTestUtils.fetchSyncTombstones(); - deepEqual(tombstones, originalTombstones, - "Tombstones should not change if tracker file is missing"); - - ok(!(await OS.File.exists(filePath)), - "Tracker file should be deleted after disabled engine migration"); - } - - _("Existing tracker file; first sync"); - { - await Utils.jsonSave("changes/bookmarks", tracker, {}); - ok(await OS.File.exists(filePath), - "Tracker file should exist before first sync migration"); - - engine.lastSync = 0; - await tracker._migrateOldEntries(); - - let fields = await PlacesTestUtils.fetchBookmarkSyncFields( - "0gtWTOgYcoJD", "0dbpnMdxKxfg", "r5ouWdPB3l28", "YK5Bdq5MIqL6"); - deepEqual(fields, originalFields, - "Sync fields should not change on first sync migration"); - let tombstones = await PlacesTestUtils.fetchSyncTombstones(); - deepEqual(tombstones, originalTombstones, - "Tombstones should not change if tracker file is missing"); - - ok(!(await OS.File.exists(filePath)), - "Tracker file should be deleted after first sync migration"); - } - - await cleanup(); -}); - -add_task(async function test_migrate_empty_tracker() { - _("Migration with empty tracker file"); - await insertBookmarksToMigrate(); - - await Utils.jsonSave("changes/bookmarks", tracker, {}); - - engine.lastSync = Date.now() / 1000; - await tracker._migrateOldEntries(); - - let fields = await PlacesTestUtils.fetchBookmarkSyncFields( - "0gtWTOgYcoJD", "0dbpnMdxKxfg", "r5ouWdPB3l28", "YK5Bdq5MIqL6"); - for (let field of fields) { - equal(field.syncStatus, PlacesUtils.bookmarks.SYNC_STATUS.NORMAL, - `Sync status of migrated bookmark ${field.guid} should be NORMAL`); - strictEqual(field.syncChangeCounter, 0, - `Change counter of migrated bookmark ${field.guid} should be 0`); - } - - let tombstones = await PlacesTestUtils.fetchSyncTombstones(); - deepEqual(tombstones, [], "Migration should delete old tombstones"); - - let filePath = OS.Path.join(OS.Constants.Path.profileDir, "weave", "changes", - "bookmarks.json"); - ok(!(await OS.File.exists(filePath)), - "Tracker file should be deleted after empty tracker migration"); - - await cleanup(); -}); - -add_task(async function test_migrate_existing_tracker() { - _("Migration with existing tracker entries"); - await insertBookmarksToMigrate(); - - let mozBmk = await PlacesUtils.bookmarks.fetch("0gtWTOgYcoJD"); - let fxBmk = await PlacesUtils.bookmarks.fetch("0dbpnMdxKxfg"); - let mozChangeTime = Math.floor(mozBmk.lastModified / 1000) - 60; - let fxChangeTime = Math.floor(fxBmk.lastModified / 1000) + 60; - await Utils.jsonSave("changes/bookmarks", tracker, { - "0gtWTOgYcoJD": mozChangeTime, - "0dbpnMdxKxfg": { - modified: fxChangeTime, - deleted: false, - }, - "3kdIPWHs9hHC": { - modified: 1479494951, - deleted: true, - }, - "l7DlMy2lL1jL": 1479496460, - }); - - engine.lastSync = Date.now() / 1000; - await tracker._migrateOldEntries(); - - let changedFields = await PlacesTestUtils.fetchBookmarkSyncFields( - "0gtWTOgYcoJD", "0dbpnMdxKxfg"); - for (let field of changedFields) { - if (field.guid == "0gtWTOgYcoJD") { - ok(field.lastModified.getTime(), mozBmk.lastModified.getTime(), - `Modified time for ${field.guid} should not be reset to older change time`); - } else if (field.guid == "0dbpnMdxKxfg") { - equal(field.lastModified.getTime(), fxChangeTime * 1000, - `Modified time for ${field.guid} should be updated to newer change time`); - } - equal(field.syncStatus, PlacesUtils.bookmarks.SYNC_STATUS.NORMAL, - `Sync status of migrated bookmark ${field.guid} should be NORMAL`); - ok(field.syncChangeCounter > 0, - `Change counter of migrated bookmark ${field.guid} should be > 0`); - } - - let unchangedFields = await PlacesTestUtils.fetchBookmarkSyncFields( - "r5ouWdPB3l28", "YK5Bdq5MIqL6"); - for (let field of unchangedFields) { - equal(field.syncStatus, PlacesUtils.bookmarks.SYNC_STATUS.NORMAL, - `Sync status of unchanged bookmark ${field.guid} should be NORMAL`); - strictEqual(field.syncChangeCounter, 0, - `Change counter of unchanged bookmark ${field.guid} should be 0`); - } - - let tombstones = await PlacesTestUtils.fetchSyncTombstones(); - await deepEqual(tombstones, [{ - guid: "3kdIPWHs9hHC", - dateRemoved: new Date(1479494951 * 1000), - }, { - guid: "l7DlMy2lL1jL", - dateRemoved: new Date(1479496460 * 1000), - }], "Should write tombstones for deleted tracked items"); - - let filePath = OS.Path.join(OS.Constants.Path.profileDir, "weave", "changes", - "bookmarks.json"); - ok(!(await OS.File.exists(filePath)), - "Tracker file should be deleted after existing tracker migration"); - - await cleanup(); -}); diff --git a/services/sync/tests/unit/test_telemetry.js b/services/sync/tests/unit/test_telemetry.js index d5720c18b81c..e63c84cfa64f 100644 --- a/services/sync/tests/unit/test_telemetry.js +++ b/services/sync/tests/unit/test_telemetry.js @@ -182,6 +182,7 @@ add_task(async function test_uploading() { title: "New Title", }); + await store.wipe(); await engine.resetClient(); ping = await sync_engine_and_validate_telem(engine, false); diff --git a/tools/lint/eslint/modules.json b/tools/lint/eslint/modules.json index f1ebea027831..399254ab2518 100644 --- a/tools/lint/eslint/modules.json +++ b/tools/lint/eslint/modules.json @@ -19,7 +19,7 @@ "bogus_element_type.jsm": [], "bookmark_repair.js": ["BookmarkRepairRequestor", "BookmarkRepairResponder"], "bookmark_validator.js": ["BookmarkValidator", "BookmarkProblemData"], - "bookmarks.js": ["BookmarksEngine", "PlacesItem", "Bookmark", "BookmarkFolder", "BookmarkQuery", "Livemark", "BookmarkSeparator"], + "bookmarks.js": ["BookmarksEngine", "PlacesItem", "Bookmark", "BookmarkFolder", "BookmarkQuery", "Livemark", "BookmarkSeparator", "BufferedBookmarksEngine"], "bookmarks.jsm": ["PlacesItem", "Bookmark", "Separator", "Livemark", "BookmarkFolder", "DumpBookmarks"], "BootstrapMonitor.jsm": ["monitor"], "browser-loader.js": ["BrowserLoader"],