Bug 1323333 - Remove the GUID map. draft
authorKit Cambridge <kit@yakshaving.ninja>
Thu, 27 Apr 2017 15:19:35 -0700
changeset 569795 ec2806ff1649b345e7999a42876592e5dd36ff63
parent 569794 36703e4082d5430e47ca84f0348f157abca7a7db
child 569796 73531e63fdae220e3008e5ab5404b139eb0e8001
push id56282
push userbmo:kit@mozilla.com
push dateThu, 27 Apr 2017 22:47:44 +0000
bugs1323333
milestone55.0a1
Bug 1323333 - Remove the GUID map. MozReview-Commit-ID: JUrF08E27Kx
services/sync/modules/engines/bookmarks.js
toolkit/components/places/PlacesSyncUtils.jsm
--- 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();