Bug 1323333 - Remove the GUID map.
MozReview-Commit-ID: JUrF08E27Kx
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -284,156 +284,16 @@ BookmarksEngine.prototype = {
syncPriority: 4,
allowSkippedRecord: false,
emptyChangeset() {
return new BookmarksChangeset();
},
-
- _guidMapFailed: false,
- _buildGUIDMap: function _buildGUIDMap() {
- let store = this._store;
- let guidMap = {};
- let tree = Async.promiseSpinningly(PlacesUtils.promiseBookmarksTree(""));
-
- function* walkBookmarksTree(tree, parent = null) {
- if (tree) {
- // Skip root node
- if (parent) {
- yield [tree, parent];
- }
- if (tree.children) {
- for (let child of tree.children) {
- store._sleep(0); // avoid jank while looping.
- yield* walkBookmarksTree(child, tree);
- }
- }
- }
- }
-
- function* walkBookmarksRoots(tree) {
- for (let child of tree.children) {
- if (isSyncedRootNode(child)) {
- yield* walkBookmarksTree(child, tree);
- }
- }
- }
-
- for (let [node, parent] of walkBookmarksRoots(tree)) {
- let {guid, type: placeType} = node;
- guid = PlacesSyncUtils.bookmarks.guidToSyncId(guid);
- let key;
- switch (placeType) {
- case PlacesUtils.TYPE_X_MOZ_PLACE:
- // Bookmark
- let query = null;
- if (node.annos && node.uri.startsWith("place:")) {
- query = node.annos.find(({name}) =>
- name === PlacesSyncUtils.bookmarks.SMART_BOOKMARKS_ANNO);
- }
- if (query && query.value) {
- key = "q" + query.value;
- } else {
- key = "b" + node.uri + ":" + (node.title || "");
- }
- break;
- case PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER:
- // Folder
- key = "f" + (node.title || "");
- break;
- case PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR:
- // Separator
- key = "s" + node.index;
- break;
- default:
- this._log.error("Unknown place type: '" + placeType + "'");
- continue;
- }
-
- let parentName = parent.title || "";
- if (guidMap[parentName] == null)
- guidMap[parentName] = {};
-
- // If the entry already exists, remember that there are explicit dupes.
- let entry = new String(guid);
- entry.hasDupe = guidMap[parentName][key] != null;
-
- // Remember this item's GUID for its parent-name/key pair.
- guidMap[parentName][key] = entry;
- this._log.trace("Mapped: " + [parentName, key, entry, entry.hasDupe]);
- }
-
- return guidMap;
- },
-
- // Helper function to get a dupe GUID for an item.
- _mapDupe: function _mapDupe(item) {
- // Figure out if we have something to key with.
- let key;
- let altKey;
- switch (item.type) {
- case "query":
- // Prior to Bug 610501, records didn't carry their Smart Bookmark
- // anno, so we won't be able to dupe them correctly. This altKey
- // hack should get them to dupe correctly.
- if (item.queryId) {
- key = "q" + item.queryId;
- altKey = "b" + item.bmkUri + ":" + (item.title || "");
- break;
- }
- // No queryID? Fall through to the regular bookmark case.
- case "bookmark":
- key = "b" + item.bmkUri + ":" + (item.title || "");
- break;
- case "folder":
- case "livemark":
- key = "f" + (item.title || "");
- break;
- case "separator":
- key = "s" + item.pos;
- break;
- default:
- return undefined;
- }
-
- // Figure out if we have a map to use!
- // This will throw in some circumstances. That's fine.
- let guidMap = this._guidMap;
-
- // Give the GUID if we have the matching pair.
- let parentName = item.parentName || "";
- this._log.trace("Finding mapping: " + parentName + ", " + key);
- let parent = guidMap[parentName];
-
- if (!parent) {
- this._log.trace("No parent => no dupe.");
- return undefined;
- }
-
- let dupe = parent[key];
-
- if (dupe) {
- this._log.trace("Mapped dupe: " + dupe);
- return dupe;
- }
-
- if (altKey) {
- dupe = parent[altKey];
- if (dupe) {
- this._log.trace("Mapped dupe using altKey " + altKey + ": " + dupe);
- return dupe;
- }
- }
-
- this._log.trace("No dupe found for key " + key + "/" + altKey + ".");
- return undefined;
- },
-
_syncStartup: function _syncStart() {
SyncEngine.prototype._syncStartup.call(this);
let cb = Async.makeSpinningCallback();
Task.spawn(function* () {
// For first-syncs, make a backup for the user to restore
if (this.lastSync == 0) {
this._log.debug("Bookmarks backup starting.");
@@ -447,35 +307,16 @@ BookmarksEngine.prototype = {
// continue.
this._log.warn("Error while backing up bookmarks, but continuing with sync", ex);
cb();
}
);
cb.wait();
- this.__defineGetter__("_guidMap", function() {
- // Create a mapping of folder titles and separator positions to GUID.
- // We do this lazily so that we don't do any work unless we reconcile
- // incoming items.
- let guidMap;
- try {
- guidMap = this._buildGUIDMap();
- } catch (ex) {
- if (Async.isShutdownException(ex)) {
- throw ex;
- }
- this._log.warn("Error while building GUID map, skipping all other incoming items", ex);
- throw {code: Engine.prototype.eEngineAbortApplyIncoming,
- cause: ex};
- }
- delete this._guidMap;
- return this._guidMap = guidMap;
- });
-
this._store._childrenToOrder = {};
this._store.clearPendingDeletions();
},
async _deletePending() {
// Delete pending items -- See the comment above BookmarkStore's deletePending
let newlyModified = await this._store.deletePending();
if (newlyModified) {
@@ -525,33 +366,24 @@ BookmarksEngine.prototype = {
this._store._childrenToOrder = {};
},
_syncFinish: function _syncFinish() {
SyncEngine.prototype._syncFinish.call(this);
this._tracker._ensureMobileQuery();
},
- _syncCleanup: function _syncCleanup() {
- SyncEngine.prototype._syncCleanup.call(this);
- delete this._guidMap;
- },
-
_createRecord: function _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);
}
// Create the record as usual, but mark it as having dupes if necessary.
let record = SyncEngine.prototype._createRecord.call(this, id);
- let entry = 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;
},
@@ -578,32 +410,16 @@ BookmarksEngine.prototype = {
let map = SyncEngine.prototype.buildWeakReuploadMap.call(this, idSet);
let changes = Async.promiseSpinningly(PlacesSyncUtils.bookmarks.getChangedIds());
for (let id of changes) {
map.delete(id);
}
return map;
},
- _findDupe: function _findDupe(item) {
- this._log.trace("Finding dupe for " + item.id +
- " (already duped: " + item.hasDupe + ").");
-
- // Don't bother finding a dupe if the incoming item has duplicates.
- if (item.hasDupe) {
- this._log.trace(item.id + " already a dupe: not finding one.");
- return null;
- }
- let mapped = this._mapDupe(item);
- this._log.debug(item.id + " mapped to " + mapped);
- // We must return a string, not an object, and the entries in the GUIDMap
- // are created via "new String()" making them an object.
- return mapped ? mapped.toString() : mapped;
- },
-
pullAllChanges() {
return this.pullNewChanges();
},
pullNewChanges() {
return Async.promiseSpinningly(this._tracker.promiseChangedIDs());
},
@@ -616,24 +432,16 @@ BookmarksEngine.prototype = {
this._noteDeletedId(id);
},
_resetClient() {
SyncEngine.prototype._resetClient.call(this);
Async.promiseSpinningly(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.
- _switchItemToDupe(localDupeGUID, incomingItem) {
- let newChanges = Async.promiseSpinningly(PlacesSyncUtils.bookmarks.dedupe(
- localDupeGUID, incomingItem.id, incomingItem.parentid));
- 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(record) {
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -572,49 +572,16 @@ const BookmarkSyncUtils = PlacesSyncUtil
// Drop stale tombstones.
yield db.executeCached("DELETE FROM moz_bookmarks_deleted");
});
}
);
}),
/**
- * De-dupes an item by changing its sync ID to match the ID on the server.
- * Sync calls this method when it detects an incoming item is a duplicate of
- * an existing local item.
- *
- * Note that this method doesn't move the item if the local and remote sync
- * IDs are different. That happens after de-duping, when the bookmarks engine
- * calls `update` to update the item.
- *
- * @param localSyncId
- * The local ID to change.
- * @param remoteSyncId
- * The remote ID that should replace the local ID.
- * @param remoteParentSyncId
- * The remote record's parent ID.
- * @return {Promise} resolved once the ID has been changed.
- * @resolves to an object containing new change records for the old item,
- * the local parent, and the remote parent if different from the
- * local parent. The bookmarks engine merges these records into the
- * changeset for the current sync.
- */
- dedupe: Task.async(function* (localSyncId, remoteSyncId, remoteParentSyncId) {
- PlacesUtils.SYNC_BOOKMARK_VALIDATORS.syncId(localSyncId);
- PlacesUtils.SYNC_BOOKMARK_VALIDATORS.syncId(remoteSyncId);
- PlacesUtils.SYNC_BOOKMARK_VALIDATORS.syncId(remoteParentSyncId);
-
- return PlacesUtils.withConnectionWrapper("BookmarkSyncUtils: dedupe", db =>
- dedupeSyncBookmark(db, BookmarkSyncUtils.syncIdToGuid(localSyncId),
- BookmarkSyncUtils.syncIdToGuid(remoteSyncId),
- BookmarkSyncUtils.syncIdToGuid(remoteParentSyncId))
- );
- }),
-
- /**
* Updates a bookmark with synced properties. Only Sync should call this
* method; other callers should use `Bookmarks.update`.
*
* The following properties are supported:
* - kind: Optional.
* - guid: Required.
* - parentGuid: Optional; reparents the bookmark if specified.
* - title: Optional.
@@ -1770,103 +1737,16 @@ var touchSyncBookmark = Task.async(funct
{ guid: bookmarkItem.guid, parentGuid: bookmarkItem.parentGuid });
// TODO (Bug 1313890): Refactor the bookmarks engine to pull change records
// before uploading, instead of returning records to merge into the engine's
// initial changeset.
return pullSyncChanges(db);
});
-var dedupeSyncBookmark = Task.async(function* (db, localGuid, remoteGuid,
- remoteParentGuid) {
- let rows = yield db.executeCached(`
- SELECT b.id, p.guid AS parentGuid, b.syncStatus
- FROM moz_bookmarks b
- JOIN moz_bookmarks p ON p.id = b.parent
- WHERE b.guid = :localGuid`,
- { localGuid });
- if (!rows.length) {
- throw new Error(`Local item ${localGuid} does not exist`);
- }
-
- let localId = rows[0].getResultByName("id");
- if (PlacesUtils.isRootItem(localId)) {
- throw new Error(`Cannot de-dupe local root ${localGuid}`);
- }
-
- let localParentGuid = rows[0].getResultByName("parentGuid");
- let sameParent = localParentGuid == remoteParentGuid;
- let modified = PlacesUtils.toPRTime(Date.now());
-
- yield db.executeTransaction(function* () {
- // Change the item's old GUID to the new remote GUID. This will throw a
- // constraint error if the remote GUID already exists locally.
- BookmarkSyncLog.debug("dedupeSyncBookmark: Switching local GUID " +
- localGuid + " to incoming GUID " + remoteGuid);
- yield db.executeCached(`UPDATE moz_bookmarks
- SET guid = :remoteGuid
- WHERE id = :localId`,
- { remoteGuid, localId });
- PlacesUtils.invalidateCachedGuidFor(localId);
-
- // And mark the parent as being modified. Given we de-dupe based on the
- // parent *name* it's possible the item having its GUID changed has a
- // different parent from the incoming record.
- // So we need to return a change record for the parent, and bump its
- // counter to ensure we don't lose the change if the current sync is
- // interrupted.
- yield db.executeCached(`UPDATE moz_bookmarks
- SET syncChangeCounter = syncChangeCounter + 1
- WHERE guid = :localParentGuid`,
- { localParentGuid });
-
- // And we also add the parent as reflected in the incoming record as the
- // de-dupe process might have used an existing item in a different folder.
- // This statement is a no-op if we don't have the new parent yet, but that's
- // fine: applying the record will add our special SYNC_PARENT_ANNO
- // annotation and move it to unfiled. If the parent arrives in the future
- // (either this Sync or a later one), the item will be reparented. Note that
- // this scenario will still leave us with inconsistent client and server
- // states; the incoming record on the server references a parent that isn't
- // the actual parent locally - see bug 1297955.
- if (!sameParent) {
- yield db.executeCached(`UPDATE moz_bookmarks
- SET syncChangeCounter = syncChangeCounter + 1
- WHERE guid = :remoteParentGuid`,
- { remoteParentGuid });
- }
-
- // The local, duplicate ID is always deleted on the server - but for
- // bookmarks it is a logical delete.
- let localSyncStatus = rows[0].getResultByName("syncStatus");
- if (localSyncStatus == PlacesUtils.bookmarks.SYNC_STATUS.NORMAL) {
- yield db.executeCached(`
- INSERT INTO moz_bookmarks_deleted (guid, dateRemoved)
- VALUES (:localGuid, :modified)`,
- { localGuid, modified });
- }
- });
-
- // TODO (Bug 1313890): Refactor the bookmarks engine to pull change records
- // before uploading, instead of returning records to merge into the engine's
- // initial changeset.
- let changeRecords = yield pullSyncChanges(db);
-
- if (BookmarkSyncLog.level <= Log.Level.Debug && !sameParent) {
- let remoteParentSyncId = BookmarkSyncUtils.guidToSyncId(remoteParentGuid);
- if (!changeRecords.hasOwnProperty(remoteParentSyncId)) {
- BookmarkSyncLog.debug("dedupeSyncBookmark: Incoming duplicate item " +
- remoteGuid + " specifies non-existing parent " +
- remoteParentGuid);
- }
- }
-
- return changeRecords;
-});
-
// Moves a synced folder's remaining children to its parent, and deletes the
// folder if it's empty.
var deleteSyncedFolder = Task.async(function* (bookmarkItem) {
// At this point, any member in the folder that remains is either a folder
// pending deletion (which we'll get to in this function), or an item that
// should not be deleted. To avoid deleting these items, we first move them
// to the parent of the folder we're about to delete.
let db = yield PlacesUtils.promiseDBConnection();