Bug 1299978 - Reupload parents of revived bookmarks and buffer folder deletion during sync r=markh,rnewman

MozReview-Commit-ID: BDfp5FffCWh

--HG--
extra : rebase_source : 0caecb27d753577373e150dd770370bd328dbe32

services/sync/modules/engines.js

@@ -1278,6 +1278,16 @@ SyncEngine.prototype = {
     // By default, assume there's no dupe items for the engine
   },
 
+  // Called when the server has a record marked as deleted, but locally we've
+  // changed it more recently than the deletion. If we return false, the
+  // record will be deleted locally. If we return true, we'll reupload the
+  // record to the server -- any extra work that's needed as part of this
+  // process should be done at this point (such as mark the record's parent
+  // for reuploading in the case of bookmarks).
+  _shouldReviveRemotelyDeletedRecord(remoteItem) {
+    return true;
+  },
+
   _deleteId: function (id) {
     this._tracker.removeChangedID(id);
 
@@ -1353,15 +1363,18 @@ SyncEngine.prototype = {
                         "exists and isn't modified.");
         return true;
       }
+      this._log.trace("Incoming record is deleted but we had local changes.");
 
-      // TODO As part of bug 720592, determine whether we should do more here.
-      // In the case where the local changes are newer, it is quite possible
-      // that the local client will restore data a remote client had tried to
-      // delete. There might be a good reason for that delete and it might be
-      // enexpected for this client to restore that data.
-      this._log.trace("Incoming record is deleted but we had local changes. " +
-                      "Applying the youngest record.");
-      return remoteIsNewer;
+      if (remoteIsNewer) {
+        this._log.trace("Remote record is newer -- deleting local record.");
+        return true;
+      }
+      // If the local record is newer, we defer to individual engines for
+      // how to handle this. By default, we revive the record.
+      let willRevive = this._shouldReviveRemotelyDeletedRecord(item);
+      this._log.trace("Local record is newer -- reviving? " + willRevive);
+
+      return !willRevive;
     }
 
     // At this point the incoming record is not for a deletion and must have

services/sync/modules/engines/bookmarks.js

@@ -472,15 +472,71 @@ BookmarksEngine.prototype = {
     });
 
     this._store._childrenToOrder = {};
+    this._store.clearPendingDeletions();
+  },
+
+  _deletePending() {
+    // Delete pending items -- See the comment above BookmarkStore's deletePending
+    let newlyModified = Async.promiseSpinningly(this._store.deletePending());
+    let now = this._tracker._now();
+    this._log.debug("Deleted pending items", newlyModified);
+    for (let modifiedSyncID of newlyModified) {
+      if (!this._modified.has(modifiedSyncID)) {
+        this._modified.set(modifiedSyncID, { timestamp: now, deleted: false });
+      }
+    }
+  },
+
+  // We avoid reviving folders since reviving them properly would require
+  // reviving their children as well. Unfortunately, this is the wrong choice
+  // in the case of a bookmark restore where wipeServer failed -- if the
+  // server has the folder as deleted, we *would* want to reupload this folder.
+  // This is mitigated by the fact that we move any undeleted children to the
+  // grandparent when deleting the parent.
+  _shouldReviveRemotelyDeletedRecord(item) {
+    let kind = Async.promiseSpinningly(
+      PlacesSyncUtils.bookmarks.getKindForSyncId(item.id));
+    if (kind === PlacesSyncUtils.bookmarks.KINDS.FOLDER) {
+      return false;
+    }
+
+    // In addition to preventing the deletion of this record (handled by the caller),
+    // we need to mark the parent of this record for uploading next sync, in order
+    // to ensure its children array is accurate.
+    let modifiedTimestamp = this._modified.getModifiedTimestamp(item.id);
+    if (!modifiedTimestamp) {
+      // We only expect this to be called with items locally modified, so
+      // something strange is going on - play it safe and don't revive it.
+      this._log.error("_shouldReviveRemotelyDeletedRecord called on unmodified item: " + item.id);
+      return false;
+    }
+
+    let localID = this._store.idForGUID(item.id);
+    let localParentID = PlacesUtils.bookmarks.getFolderIdForItem(localID);
+    let localParentSyncID = this._store.GUIDForId(localParentID);
+
+    this._log.trace(`Reviving item "${item.id}" and marking parent ${localParentSyncID} as modified.`);
+
+    if (!this._modified.has(localParentSyncID)) {
+      this._modified.set(localParentSyncID, {
+        timestamp: modifiedTimestamp,
+        deleted: false
+      });
+    }
+    return true
   },
 
   _processIncoming: function (newitems) {
     try {
       SyncEngine.prototype._processIncoming.call(this, newitems);
     } finally {
-      // Reorder children.
-      this._store._orderChildren();
-      delete this._store._childrenToOrder;
+      try {
+        this._deletePending();
+      } finally {
+        // Reorder children.
+        this._store._orderChildren();
+        delete this._store._childrenToOrder;
+      }
     }
   },
 
@@ -657,7 +713,8 @@ BookmarksEngine.prototype = {
 
 function BookmarksStore(name, engine) {
   Store.call(this, name, engine);
-
+  this._foldersToDelete = new Set();
+  this._atomsToDelete = new Set();
   // Explicitly nullify our references to our cached services so we don't leak
   Svc.Obs.add("places-shutdown", function() {
     for (let query in this._stmts) {
@@ -732,14 +789,18 @@ BookmarksStore.prototype = {
   },
 
   remove: function BStore_remove(record) {
-    try {
-      let info = Async.promiseSpinningly(PlacesSyncUtils.bookmarks.remove(record.id));
-      if (info) {
-        this._log.debug(`Removed item ${record.id} with type ${record.type}`);
-      }
-    } catch (ex) {
-      // Likely already removed.
-      this._log.debug(`Error removing ${record.id}`, ex);
+    if (PlacesSyncUtils.bookmarks.isRootSyncID(record.id)) {
+      this._log.warn("Refusing to remove special folder " + record.id);
+      return;
+    }
+    let recordKind = Async.promiseSpinningly(
+      PlacesSyncUtils.bookmarks.getKindForSyncId(record.id));
+    let isFolder = recordKind === PlacesSyncUtils.bookmarks.KINDS.FOLDER;
+    this._log.trace(`Buffering removal of item "${record.id}" of type "${recordKind}".`);
+    if (isFolder) {
+      this._foldersToDelete.add(record.id);
+    } else {
+      this._atomsToDelete.add(record.id);
     }
   },
 
@@ -762,6 +823,132 @@ BookmarksStore.prototype = {
     Async.promiseSpinningly(Promise.all(promises));
   },
 
+  // There's some complexity here around pending deletions. Our goals:
+  //
+  // - Don't delete any bookmarks a user has created but not explicitly deleted
+  //   (This includes any bookmark that was not a child of the folder at the
+  //   time the deletion was recorded, and also bookmarks restored from a backup).
+  // - Don't undelete any bookmark without ensuring the server structure
+  //   includes it (see `BookmarkEngine.prototype._shouldReviveRemotelyDeletedRecord`)
+  //
+  // This leads the following approach:
+  //
+  // - Additions, moves, and updates are processed before deletions.
+  //     - To do this, all deletion operations are buffered during a sync. Folders
+  //       we plan on deleting have their sync id's stored in `this._foldersToDelete`,
+  //       and non-folders we plan on deleting have their sync id's stored in
+  //       `this._atomsToDelete`.
+  //     - The exception to this is the moves that occur to fix the order of bookmark
+  //       children, which are performed after we process deletions.
+  // - Non-folders are deleted before folder deletions, so that when we process
+  //   folder deletions we know the correct state.
+  // - Remote deletions always win for folders, but do not result in recursive
+  //   deletion of children. This is a hack because we're not able to distinguish
+  //   between value changes and structural changes to folders, and we don't even
+  //   have the old server record to compare to. See `BookmarkEngine`'s
+  //   `_shouldReviveRemotelyDeletedRecord` method.
+  // - When a folder is deleted, its remaining children are moved in order to
+  //   their closest living ancestor.  If this is interrupted (unlikely, but
+  //   possible given that we don't perform this operation in a transaction),
+  //   we revive the folder.
+  // - Remote deletions can lose for non-folders, but only until we handle
+  //   bookmark restores correctly (removing stale state from the server -- this
+  //   is to say, if bug 1230011 is fixed, we should never revive bookmarks).
+
+  deletePending: Task.async(function* deletePending() {
+    yield this._deletePendingAtoms();
+    let guidsToUpdate = yield this._deletePendingFolders();
+    this.clearPendingDeletions();
+    return guidsToUpdate;
+  }),
+
+  clearPendingDeletions() {
+    this._foldersToDelete.clear();
+    this._atomsToDelete.clear();
+  },
+
+  _deleteAtom: Task.async(function* _deleteAtom(syncID) {
+    try {
+      let info = yield PlacesSyncUtils.bookmarks.remove(syncID, {
+        preventRemovalOfNonEmptyFolders: true
+      });
+      this._log.trace(`Removed item ${syncID} with type ${info.type}`);
+    } catch (ex) {
+      // Likely already removed.
+      this._log.trace(`Error removing ${syncID}`, ex);
+    }
+  }),
+
+  _deletePendingAtoms() {
+    return Promise.all(
+      [...this._atomsToDelete.values()]
+        .map(syncID => this._deleteAtom(syncID)));
+  },
+
+  // Returns an array of sync ids that need updates.
+  _deletePendingFolders: Task.async(function* _deletePendingFolders() {
+    // To avoid data loss, we don't want to just delete the folder outright,
+    // so we buffer folder deletions and process them at the end (now).
+    //
+    // 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 needUpdate = new Set();
+    for (let syncId of this._foldersToDelete) {
+      let childSyncIds = yield PlacesSyncUtils.bookmarks.fetchChildSyncIds(syncId);
+      if (!childSyncIds.length) {
+        // No children -- just delete the folder.
+        yield this._deleteAtom(syncId)
+        continue;
+      }
+      // We could avoid some redundant work here by finding the nearest
+      // grandparent who isn't present in `this._toDelete`...
+
+      let grandparentSyncId = this.GUIDForId(
+        PlacesUtils.bookmarks.getFolderIdForItem(
+          this.idForGUID(PlacesSyncUtils.bookmarks.syncIdToGuid(syncId))));
+
+      this._log.trace(`Moving ${childSyncIds.length} children of "${syncId}" to ` +
+                      `grandparent "${grandparentSyncId}" before deletion.`);
+
+      // Move children out of the parent and into the grandparent
+      yield Promise.all(childSyncIds.map(child => PlacesSyncUtils.bookmarks.update({
+        syncId: child,
+        parentSyncId: grandparentSyncId
+      })));
+
+      // Delete the (now empty) parent
+      try {
+        yield PlacesSyncUtils.bookmarks.remove(syncId, {
+          preventRemovalOfNonEmptyFolders: true
+        });
+      } catch (e) {
+        // We failed, probably because someone added something to this folder
+        // between when we got the children and now (or the database is corrupt,
+        // or something else happened...) This is unlikely, but possible. To
+        // avoid corruption in this case, we need to reupload the record to the
+        // server.
+        //
+        // (Ideally this whole operation would be done in a transaction, and this
+        // wouldn't be possible).
+        needUpdate.add(syncId);
+      }
+
+      // Add children (for parentid) and grandparent (for children list) to set
+      // of records needing an update, *unless* they're marked for deletion.
+      if (!this._foldersToDelete.has(grandparentSyncId)) {
+        needUpdate.add(grandparentSyncId);
+      }
+      for (let childSyncId of childSyncIds) {
+        if (!this._foldersToDelete.has(childSyncId)) {
+          needUpdate.add(childSyncId);
+        }
+      }
+    }
+    return [...needUpdate];
+  }),
+
   changeItemID: function BStore_changeItemID(oldID, newID) {
     this._log.debug("Changing GUID " + oldID + " to " + newID);
 
@@ -874,6 +1061,7 @@ BookmarksStore.prototype = {
   },
 
   wipe: function BStore_wipe() {
+    this.clearPendingDeletions();
     Async.promiseSpinningly(Task.spawn(function* () {
       // Save a backup before clearing out all bookmarks.
       yield PlacesBackups.create(null, true);

services/sync/tests/tps/all_tests.json

@@ -1,4 +1,5 @@
 { "tests": [
+    "test_bookmark_conflict.js",
     "test_sync.js",
     "test_prefs.js",
     "test_tabs.js",

services/sync/tests/tps/test_bookmark_conflict.js

@@ -0,0 +1,143 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+/*
+ * The list of phases mapped to their corresponding profiles.  The object
+ * here must be in strict JSON format, as it will get parsed by the Python
+ * testrunner (no single quotes, extra comma's, etc).
+ */
+EnableEngines(["bookmarks"]);
+
+var phases = { "phase1": "profile1",
+               "phase2": "profile2",
+               "phase3": "profile1",
+               "phase4": "profile2" };
+
+
+// the initial list of bookmarks to add to the browser
+var bookmarksInitial = {
+  "menu": [
+    { folder: "foldera" },
+    { folder: "folderb" },
+    { folder: "folderc" },
+    { folder: "folderd" },
+  ],
+
+  "menu/foldera": [{ uri: "http://www.cnn.com", title: "CNN" }],
+  "menu/folderb": [{ uri: "http://www.apple.com", title: "Apple", tags: [] }],
+  "menu/folderc": [{ uri: "http://www.yahoo.com", title: "Yahoo" }],
+
+  "menu/folderd": []
+};
+
+// a list of bookmarks to delete during a 'delete' action on P2
+var bookmarksToDelete = {
+  "menu": [
+    { folder: "foldera" },
+    { folder: "folderb" },
+  ],
+  "menu/folderc": [{ uri: "http://www.yahoo.com", title: "Yahoo" }],
+};
+
+
+// the modifications to make on P1, after P2 has synced, but before P1 has gotten
+// P2's changes
+var bookmarkMods = {
+  "menu": [
+    { folder: "foldera" },
+    { folder: "folderb" },
+    { folder: "folderc" },
+    { folder: "folderd" },
+  ],
+
+  // we move this child out of its folder (p1), after deleting the folder (p2)
+  // and expect the child to come back to p2 after sync.
+  "menu/foldera": [{
+    uri: "http://www.cnn.com",
+    title: "CNN",
+    changes: { location: "menu/folderd" }
+  }],
+
+  // we rename this child (p1) after deleting the folder (p2), and expect the child
+  // to be moved into great grandparent (menu)
+  "menu/folderb": [{
+    uri: "http://www.apple.com",
+    title: "Apple",
+    tags: [],
+    changes: { title: "Mac" }
+  }],
+
+
+  // we move this child (p1) after deleting the child (p2) and expect it to survive
+  "menu/folderc": [{
+    uri: "http://www.yahoo.com",
+    title: "Yahoo",
+    changes: { location: "menu/folderd" }
+  }],
+
+  "menu/folderd": []
+};
+
+// a list of bookmarks to delete during a 'delete' action
+var bookmarksToDelete = {
+  "menu": [
+    { folder: "foldera" },
+    { folder: "folderb" },
+  ],
+  "menu/folderc": [
+    { uri: "http://www.yahoo.com", title: "Yahoo" },
+  ],
+};
+
+
+
+// expected bookmark state after conflict resolution
+var bookmarksExpected = {
+  "menu": [
+    { folder: "folderc" },
+    { folder: "folderd" },
+    { uri: "http://www.apple.com", title: "Mac", },
+  ],
+
+  "menu/folderc": [],
+
+  "menu/folderd": [
+    { uri: "http://www.cnn.com", title: "CNN" },
+    { uri: "http://www.yahoo.com", title: "Yahoo" }
+  ]
+};
+
+// Add bookmarks to profile1 and sync.
+Phase("phase1", [
+  [Bookmarks.add, bookmarksInitial],
+  [Bookmarks.verify, bookmarksInitial],
+  [Sync],
+  [Bookmarks.verify, bookmarksInitial],
+]);
+
+// Sync to profile2 and verify that the bookmarks are present. Delete
+// bookmarks/folders, verify that it's not present, and sync
+Phase("phase2", [
+  [Sync],
+  [Bookmarks.verify, bookmarksInitial],
+  [Bookmarks.delete, bookmarksToDelete],
+  [Bookmarks.verifyNot, bookmarksToDelete],
+  [Sync]
+]);
+
+// Using profile1, modify the bookmarks, and sync *after* the modification,
+// and then sync again to propagate the reconciliation changes.
+Phase("phase3", [
+  [Bookmarks.verify, bookmarksInitial],
+  [Bookmarks.modify, bookmarkMods],
+  [Sync],
+  [Bookmarks.verify, bookmarksExpected],
+  [Bookmarks.verifyNot, bookmarksToDelete],
+]);
+
+// Back in profile2, do a sync and verify that we're in the expected state
+Phase("phase4", [
+  [Sync],
+  [Bookmarks.verify, bookmarksExpected],
+  [Bookmarks.verifyNot, bookmarksToDelete],
+]);

services/sync/tests/unit/test_bookmark_store.js

@@ -21,7 +21,7 @@ tracker.persistChangedIDs = false;
 var fxuri = Utils.makeURI("http://getfirefox.com/");
 var tburi = Utils.makeURI("http://getthunderbird.com/");
 
-add_test(function test_ignore_specials() {
+add_task(function* test_ignore_specials() {
   _("Ensure that we can't delete bookmark roots.");
 
   // Belt...
@@ -30,6 +30,7 @@ add_test(function test_ignore_specials() {
   do_check_neq(null, store.idForGUID("toolbar"));
 
   store.applyIncoming(record);
+  yield store.deletePending();
 
   // Ensure that the toolbar exists.
   do_check_neq(null, store.idForGUID("toolbar"));
@@ -39,11 +40,11 @@ add_test(function test_ignore_specials() {
 
   // Braces...
   store.remove(record);
+  yield store.deletePending();
   do_check_neq(null, store.idForGUID("toolbar"));
   engine._buildGUIDMap();
 
   store.wipe();
-  run_next_test();
 });
 
 add_test(function test_bookmark_create() {
@@ -243,7 +244,7 @@ add_test(function test_folder_createRecord() {
   }
 });
 
-add_test(function test_deleted() {
+add_task(function* test_deleted() {
   try {
     _("Create a bookmark that will be deleted.");
     let bmk1_id = PlacesUtils.bookmarks.insertBookmark(
@@ -255,7 +256,7 @@ add_test(function test_deleted() {
     let record = new PlacesItem("bookmarks", bmk1_guid);
     record.deleted = true;
     store.applyIncoming(record);
-
+    yield store.deletePending();
     _("Ensure it has been deleted.");
     let error;
     try {
@@ -271,7 +272,6 @@ add_test(function test_deleted() {
   } finally {
     _("Clean up.");
     store.wipe();
-    run_next_test();
   }
 });
 
@@ -428,6 +428,106 @@ add_test(function test_empty_query_doesnt_die() {
   run_next_test();
 });
 
+function assertDeleted(id) {
+  let error;
+  try {
+    PlacesUtils.bookmarks.getItemType(id);
+  } catch (e) {
+    error = e;
+  }
+  equal(error.result, Cr.NS_ERROR_ILLEGAL_VALUE)
+}
+
+add_task(function* test_delete_buffering() {
+  store.wipe();
+  try {
+    _("Create a folder with two bookmarks.");
+    let folder = new BookmarkFolder("bookmarks", "testfolder-1");
+    folder.parentName = "Bookmarks Toolbar";
+    folder.parentid = "toolbar";
+    folder.title = "Test Folder";
+    store.applyIncoming(folder);
+
+
+    let fxRecord = new Bookmark("bookmarks", "get-firefox1");
+    fxRecord.bmkUri        = fxuri.spec;
+    fxRecord.title         = "Get Firefox!";
+    fxRecord.parentName    = "Test Folder";
+    fxRecord.parentid      = "testfolder-1";
+
+    let tbRecord = new Bookmark("bookmarks", "get-tndrbrd1");
+    tbRecord.bmkUri        = tburi.spec;
+    tbRecord.title         = "Get Thunderbird!";
+    tbRecord.parentName    = "Test Folder";
+    tbRecord.parentid      = "testfolder-1";
+
+    store.applyIncoming(fxRecord);
+    store.applyIncoming(tbRecord);
+
+    let folderId = store.idForGUID(folder.id);
+    let fxRecordId = store.idForGUID(fxRecord.id);
+    let tbRecordId = store.idForGUID(tbRecord.id);
+
+    _("Check everything was created correctly.");
+
+    equal(PlacesUtils.bookmarks.getItemType(fxRecordId),
+          PlacesUtils.bookmarks.TYPE_BOOKMARK);
+    equal(PlacesUtils.bookmarks.getItemType(tbRecordId),
+          PlacesUtils.bookmarks.TYPE_BOOKMARK);
+    equal(PlacesUtils.bookmarks.getItemType(folderId),
+          PlacesUtils.bookmarks.TYPE_FOLDER);
+
+    equal(PlacesUtils.bookmarks.getFolderIdForItem(fxRecordId), folderId);
+    equal(PlacesUtils.bookmarks.getFolderIdForItem(tbRecordId), folderId);
+    equal(PlacesUtils.bookmarks.getFolderIdForItem(folderId),
+          PlacesUtils.bookmarks.toolbarFolder);
+
+    _("Delete the folder and one bookmark.");
+
+    let deleteFolder = new PlacesItem("bookmarks", "testfolder-1");
+    deleteFolder.deleted = true;
+
+    let deleteFxRecord = new PlacesItem("bookmarks", "get-firefox1");
+    deleteFxRecord.deleted = true;
+
+    store.applyIncoming(deleteFolder);
+    store.applyIncoming(deleteFxRecord);
+
+    _("Check that we haven't deleted them yet, but that the deletions are queued");
+    // these will throw if we've deleted them
+    equal(PlacesUtils.bookmarks.getItemType(fxRecordId),
+           PlacesUtils.bookmarks.TYPE_BOOKMARK);
+
+    equal(PlacesUtils.bookmarks.getItemType(folderId),
+           PlacesUtils.bookmarks.TYPE_FOLDER);
+
+    equal(PlacesUtils.bookmarks.getFolderIdForItem(fxRecordId), folderId);
+
+    ok(store._foldersToDelete.has(folder.id));
+    ok(store._atomsToDelete.has(fxRecord.id));
+    ok(!store._atomsToDelete.has(tbRecord.id));
+
+    _("Process pending deletions and ensure that the right things are deleted.");
+    let updatedGuids = yield store.deletePending();
+
+    deepEqual(updatedGuids.sort(), ["get-tndrbrd1", "toolbar"]);
+
+    assertDeleted(fxRecordId);
+    assertDeleted(folderId);
+
+    ok(!store._foldersToDelete.has(folder.id));
+    ok(!store._atomsToDelete.has(fxRecord.id));
+
+    equal(PlacesUtils.bookmarks.getFolderIdForItem(tbRecordId),
+          PlacesUtils.bookmarks.toolbarFolder);
+
+  } finally {
+    _("Clean up.");
+    store.wipe();
+  }
+});
+
+
 function run_test() {
   initTestLogging('Trace');
   run_next_test();

toolkit/components/places/PlacesSyncUtils.jsm

@@ -132,19 +132,27 @@ const BookmarkSyncUtils = PlacesSyncUtils.bookmarks = Object.freeze({
   }),
 
   /**
-   * Removes an item from the database.
+   * Removes an item from the database. Options are passed through to
+   * PlacesUtils.bookmarks.remove.
    */
-  remove: Task.async(function* (syncId) {
+  remove: Task.async(function* (syncId, options = {}) {
     let guid = BookmarkSyncUtils.syncIdToGuid(syncId);
     if (guid in ROOT_GUID_TO_SYNC_ID) {
       BookmarkSyncLog.warn(`remove: Refusing to remove root ${syncId}`);
       return null;
     }
-    return PlacesUtils.bookmarks.remove(guid, {
+    return PlacesUtils.bookmarks.remove(guid, Object.assign({}, options, {
       source: SOURCE_SYNC,
-    });
+    }));
   }),
 
+  /**
+   * Returns true for sync IDs that are considered roots.
+   */
+  isRootSyncID(syncID) {
+    return ROOT_SYNC_ID_TO_GUID.hasOwnProperty(syncID);
+  },
+
   /**
    * Changes the GUID of an existing item. This method only allows Places GUIDs
    * because root sync IDs cannot be changed.
@@ -310,6 +318,30 @@ const BookmarkSyncUtils = PlacesSyncUtils.bookmarks = Object.freeze({
 
     return item;
   }),
+
+  /**
+   * Get the sync record kind for the record with provided sync id.
+   *
+   * @param syncId
+   *        Sync ID for the item in question
+   *
+   * @returns {Promise} A promise that resolves with the sync record kind (e.g.
+   *                    something under `PlacesSyncUtils.bookmarks.KIND`), or
+   *                    with `null` if no item with that guid exists.
+   * @throws if `guid` is invalid.
+   */
+  getKindForSyncId(syncId) {
+    PlacesUtils.SYNC_BOOKMARK_VALIDATORS.syncId(syncId);
+    let guid = BookmarkSyncUtils.syncIdToGuid(syncId);
+    return PlacesUtils.bookmarks.fetch(guid)
+    .then(item => {
+      if (!item) {
+        return null;
+      }
+      return getKindForItem(item)
+    });
+  },
+
 });
 
 XPCOMUtils.defineLazyGetter(this, "BookmarkSyncLog", () => {