Bug 1265419 - Implement a validator for bookmarks, that reports errors in the server-side bookmark store, and inconsistencies between server and client. r=markh

MozReview-Commit-ID: Ib3wnJt1buL

--HG--
extra : transplant_source : w%D4Z%EEQ%1Bj%24%29I%D3%C0l%EB%AC0%D8%87/%AA

services/sync/modules/bookmark_utils.js

@@ -0,0 +1,94 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+this.EXPORTED_SYMBOLS = ["BookmarkSpecialIds", "BookmarkAnnos"];
+
+const { utils: Cu, interfaces: Ci, classes: Cc } = Components;
+
+Cu.import("resource://gre/modules/PlacesUtils.jsm");
+
+let BookmarkAnnos = {
+  ALLBOOKMARKS_ANNO:    "AllBookmarks",
+  DESCRIPTION_ANNO:     "bookmarkProperties/description",
+  SIDEBAR_ANNO:         "bookmarkProperties/loadInSidebar",
+  MOBILEROOT_ANNO:      "mobile/bookmarksRoot",
+  MOBILE_ANNO:          "MobileBookmarks",
+  EXCLUDEBACKUP_ANNO:   "places/excludeFromBackup",
+  SMART_BOOKMARKS_ANNO: "Places/SmartBookmark",
+  PARENT_ANNO:          "sync/parent",
+  ORGANIZERQUERY_ANNO:  "PlacesOrganizer/OrganizerQuery",
+};
+
+let BookmarkSpecialIds = {
+
+  // Special IDs. Note that mobile can attempt to create a record on
+  // dereference; special accessors are provided to prevent recursion within
+  // observers.
+  guids: ["menu", "places", "tags", "toolbar", "unfiled", "mobile"],
+
+  // Create the special mobile folder to store mobile bookmarks.
+  createMobileRoot: function createMobileRoot() {
+    let root = PlacesUtils.placesRootId;
+    let mRoot = PlacesUtils.bookmarks.createFolder(root, "mobile", -1);
+    PlacesUtils.annotations.setItemAnnotation(
+      mRoot, BookmarkAnnos.MOBILEROOT_ANNO, 1, 0, PlacesUtils.annotations.EXPIRE_NEVER);
+    PlacesUtils.annotations.setItemAnnotation(
+      mRoot, BookmarkAnnos.EXCLUDEBACKUP_ANNO, 1, 0, PlacesUtils.annotations.EXPIRE_NEVER);
+    return mRoot;
+  },
+
+  findMobileRoot: function findMobileRoot(create) {
+    // Use the (one) mobile root if it already exists.
+    let root = PlacesUtils.annotations.getItemsWithAnnotation(
+      BookmarkAnnos.MOBILEROOT_ANNO, {});
+    if (root.length != 0)
+      return root[0];
+
+    if (create)
+      return this.createMobileRoot();
+
+    return null;
+  },
+
+  // Accessors for IDs.
+  isSpecialGUID: function isSpecialGUID(g) {
+    return this.guids.indexOf(g) != -1;
+  },
+
+  specialIdForGUID: function specialIdForGUID(guid, create) {
+    if (guid == "mobile") {
+      return this.findMobileRoot(create);
+    }
+    return this[guid];
+  },
+
+  // Don't bother creating mobile: if it doesn't exist, this ID can't be it!
+  specialGUIDForId: function specialGUIDForId(id) {
+    for (let guid of this.guids)
+      if (this.specialIdForGUID(guid, false) == id)
+        return guid;
+    return null;
+  },
+
+  get menu() {
+    return PlacesUtils.bookmarksMenuFolderId;
+  },
+  get places() {
+    return PlacesUtils.placesRootId;
+  },
+  get tags() {
+    return PlacesUtils.tagsFolderId;
+  },
+  get toolbar() {
+    return PlacesUtils.toolbarFolderId;
+  },
+  get unfiled() {
+    return PlacesUtils.unfiledBookmarksFolderId;
+  },
+  get mobile() {
+    return this.findMobileRoot(true);
+  },
+};

services/sync/modules/bookmark_validator.js

@@ -0,0 +1,462 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+const Cu = Components.utils;
+
+Cu.import("resource://gre/modules/PlacesUtils.jsm");
+Cu.import("resource://services-sync/util.js");
+Cu.import("resource://services-sync/bookmark_utils.js");
+
+this.EXPORTED_SYMBOLS = ["BookmarkValidator"];
+
+
+class BookmarkValidator {
+
+  createClientRecordsFromTree(clientTree) {
+    // Iterate over the treeNode, converting it to something more similar to what
+    // the server stores.
+    let records = [];
+    function traverse(treeNode) {
+      let guid = BookmarkSpecialIds.specialGUIDForId(treeNode.id) || treeNode.guid;
+      let itemType = 'item';
+      treeNode.id = guid;
+      switch (treeNode.type) {
+        case PlacesUtils.TYPE_X_MOZ_PLACE:
+          let query = null;
+          if (treeNode.annos && treeNode.uri.startsWith("place:")) {
+            query = treeNode.annos.find(({name}) =>
+              name === BookmarkAnnos.SMART_BOOKMARKS_ANNO);
+          }
+          if (query && query.value) {
+            itemType = 'query';
+          } else {
+            itemType = 'bookmark';
+          }
+          break;
+        case PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER:
+          itemType = 'folder';
+          break;
+        case PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR:
+          itemType = 'separator';
+          break;
+      }
+
+      treeNode.type = itemType;
+      treeNode.pos = treeNode.index;
+      treeNode.bmkUri = treeNode.uri;
+      records.push(treeNode);
+      if (treeNode.type === 'folder') {
+        treeNode.childGUIDs = [];
+        if (!treeNode.children) {
+          treeNode.children = [];
+        }
+        for (let child of treeNode.children) {
+          traverse(child);
+          child.parent = treeNode;
+          child.parentid = guid;
+          child.parentName = treeNode.title;
+          treeNode.childGUIDs.push(child.guid);
+        }
+      }
+    }
+    traverse(clientTree);
+    clientTree.id = 'places';
+    return records;
+  }
+
+  /**
+   * Process the server-side list. Mainly this builds the records into a tree,
+   * but it also records information about problems, and produces arrays of the
+   * deleted and non-deleted nodes.
+   *
+   * Returns an object containing:
+   * - records:Array of non-deleted records. Each record contains the following
+   *    properties
+   *     - childGUIDs (array of strings, only present if type is 'folder'): the
+   *       list of child GUIDs stored on the server.
+   *     - children (array of records, only present if type is 'folder'):
+   *       each record has these same properties. This may differ in content
+   *       from what you may expect from the childGUIDs list, as it won't
+   *       contain any records that could not be found.
+   *     - parent (record): The parent to this record.
+   *     - Unchanged properties send down from the server: id, title, type,
+   *       parentName, parentid, bmkURI, keyword, tags, pos, queryId, loadInSidebar
+   * - root: Root of the server-side bookmark tree. Has the same properties as
+   *   above.
+   * - deletedRecords: As above, but only contains items that the server sent
+   *   where it also sent indication that the item should be deleted.
+   * - problemData: Object containing info about problems recorded.
+   *     - missingIDs (number): # of objects with missing ids
+   *     - duplicates (array of ids): ids seen more than once
+   *     - parentChildMismatches (array of {parent: parentid, child: childid}):
+   *       instances where the child's parentid and the parent's children array
+   *       do not match
+   *     - cycles (array of array of ids). List of cycles found in the "tree".
+   *     - orphans (array of {id: string, parent: string}): List of nodes with
+   *       either no parentid, or where the parent could not be found.
+   *     - missingChildren (array of {parent: id, child: id}):
+   *       List of parent/children where the child id couldn't be found
+   *     - multipleParents (array of {child: id, parents: array of ids}):
+   *       List of children that were part of multiple parent arrays
+   *     - deletedParents (array of ids) : List of records that aren't deleted but
+   *       had deleted parents
+   *     - childrenOnNonFolder (array of ids): list of non-folders that still have
+   *       children arrays
+   *     - duplicateChildren (array of ids): list of records who have the same
+   *       child listed multiple times in their children array
+   *     - parentNotFolder (array of ids): list of records that have parents that
+   *       aren't folders
+   *     - wrongParentName (array of ids): list of records whose parentName does
+   *       not match the parent's actual title
+   *     - rootOnServer (boolean): true if the root came from the server
+   */
+  inspectServerRecords(serverRecords) {
+    let deletedItemIds = new Set();
+    let idToRecord = new Map();
+    let deletedRecords = [];
+
+    let folders = [];
+    let problems = [];
+
+    let problemData = {
+      missingIDs: 0,
+      duplicates: [],
+      parentChildMismatches: [],
+      cycles: [],
+      orphans: [],
+      missingChildren: [],
+      multipleParents: [],
+      deletedParents: [],
+      childrenOnNonFolder: [],
+      duplicateChildren: [],
+      parentNotFolder: [],
+      wrongParentName: [],
+      rootOnServer: false
+    };
+
+    let resultRecords = [];
+
+    for (let record of serverRecords) {
+      if (!record.id) {
+        ++problemData.missingIDs;
+        continue;
+      }
+      if (record.deleted) {
+        deletedItemIds.add(record.id);
+      } else {
+        if (idToRecord.has(record.id)) {
+          problemData.duplicates.push(record.id);
+          continue;
+        }
+        idToRecord.set(record.id, record);
+      }
+      if (record.children) {
+        if (record.type !== 'folder') {
+          problemData.childrenOnNonFolder.push(record.id);
+        }
+        folders.push(record);
+
+        if (new Set(record.children).size !== record.children.length) {
+          problemData.duplicateChildren.push(record.id)
+        }
+
+        // This whole next part is a huge hack.
+        // The children array stores special guids as their local guid values,
+        // e.g. 'menu________' instead of 'menu', but all other parts of the
+        // serverside bookmark info stores it as the special value ('menu').
+        //
+        // Since doing a sql query for every entry would be extremely slow, and
+        // wouldn't even be necessarially accurate (since these values are only
+        // the local values for whichever client created the records) We just
+        // strip off the trailing _ and see if that results in a special id.
+        //
+        // To make things worse, this doesn't even work for root________, which has
+        // the special id 'places'.
+        record.childGUIDs = record.children;
+        record.children = record.children.map(childID => {
+          let match = childID.match(/_+$/);
+          if (!match) {
+            return childID;
+          }
+          let possibleSpecialID = childID.slice(0, match.index);
+          if (possibleSpecialID === 'root') {
+            possibleSpecialID = 'places';
+          }
+          if (BookmarkSpecialIds.isSpecialGUID(possibleSpecialID)) {
+            return possibleSpecialID;
+          }
+          return childID;
+        });
+      }
+    }
+
+    for (let deletedId of deletedItemIds) {
+      let record = idToRecord.get(deletedId);
+      if (record && !record.isDeleted) {
+        deletedRecords.push(record);
+        record.isDeleted = true;
+      }
+    }
+
+    let root = idToRecord.get('places');
+
+    if (!root) {
+      // Fabricate a root. We want to remember that it's fake so that we can
+      // avoid complaining about stuff like it missing it's childGUIDs later.
+      root = { id: 'places', children: [], type: 'folder', title: '' };
+      resultRecords.push(root);
+      idToRecord.set('places', root);
+    } else {
+      problemData.rootOnServer = true;
+    }
+
+    // Build the tree, find orphans, and record most problems having to do with
+    // the tree structure.
+    for (let [id, record] of idToRecord) {
+      if (record === root) {
+        continue;
+      }
+
+      let parentID = record.parentid;
+      if (!parentID) {
+        problemData.orphans.push({id: record.id, parent: parentID});
+        continue;
+      }
+
+      let parent = idToRecord.get(parentID);
+      if (!parent) {
+        problemData.orphans.push({id: record.id, parent: parentID});
+        continue;
+      }
+
+      if (parent.type !== 'folder') {
+        problemData.parentNotFolder.push(record.id);
+      }
+
+      if (!record.isDeleted) {
+        resultRecords.push(record);
+      }
+
+      record.parent = parent;
+      if (parent !== root) {
+        let childIndex = parent.children.indexOf(id);
+        if (childIndex < 0) {
+          problemData.parentChildMismatches.push({parent: parent.id, child: record.id});
+        } else {
+          parent.children[childIndex] = record;
+        }
+      } else {
+        parent.children.push(record);
+      }
+
+      if (parent.isDeleted && !record.isDeleted) {
+        problemData.deletedParents.push(record.id);
+      }
+
+      if (record.parentName !== parent.title && parent.id !== 'unfiled') {
+        problemData.wrongParentName.push(record.id);
+      }
+    }
+
+    // Check that we aren't missing any children.
+    for (let folder of folders) {
+      for (let ci = 0; ci < folder.children.length; ++ci) {
+        let child = folder.children[ci];
+        if (typeof child === 'string') {
+          let childObject = idToRecord.get(child);
+          if (!childObject) {
+            problemData.missingChildren.push({parent: folder.id, child});
+          } else {
+            if (childObject.parentid === folder.id) {
+              // Probably impossible, would have been caught in the loop above.
+              continue;
+            }
+
+            // The child is in multiple `children` arrays.
+            let currentProblemRecord = problemData.multipleParents.find(pr =>
+              pr.child === child);
+
+            if (currentProblemRecord) {
+              currentProblemRecord.parents.push(folder.id);
+            } else {
+              problemData.multipleParents.push({ child, parents: [childObject.parentid, folder.id] });
+            }
+          }
+          // Remove it from the array to avoid needing to special case this later.
+          folder.children.splice(ci, 1);
+          --ci;
+        }
+      }
+    }
+
+    problemData.cycles = this._detectCycles(resultRecords);
+
+    return {
+      deletedRecords,
+      records: resultRecords,
+      problemData,
+      root,
+    };
+  }
+
+  // helper for inspectServerRecords
+  _detectCycles(records) {
+    // currentPath and pathLookup contain the same data. pathLookup is faster to
+    // query, but currentPath gives is the order of traversal that we need in
+    // order to report the members of the cycles.
+    let pathLookup = new Set();
+    let currentPath = [];
+    let cycles = [];
+    let seenEver = new Set();
+    const traverse = node => {
+      if (pathLookup.has(node)) {
+        let cycleStart = currentPath.lastIndexOf(node);
+        let cyclePath = currentPath.slice(cycleStart).map(n => n.id);
+        cycles.push(cyclePath);
+        return;
+      } else if (seenEver.has(node)) {
+        // This is a problem, but we catch it earlier (multipleParents)
+        return;
+      }
+      seenEver.add(node);
+
+      if (node.children) {
+        pathLookup.add(node);
+        currentPath.push(node);
+        for (let child of node.children) {
+          traverse(child);
+        }
+        currentPath.pop();
+        pathLookup.delete(node);
+      }
+    };
+    for (let record of records) {
+      if (!seenEver.has(record)) {
+        traverse(record);
+      }
+    }
+
+    return cycles;
+  }
+
+  /**
+   * Compare the list of server records with the client tree.
+   *
+   * Returns the problemData described in the inspectServerRecords comment,
+   * with the following additional fields.
+   * - clientMissing: Array of ids on the server missing from the client
+   * - serverMissing: Array of ids on the client missing from the server
+   * - differences: Array of {id: string, differences: string array} recording
+   *   the properties that are differente between the client and server
+   */
+  compareServerWithClient(serverRecords, clientTree) {
+
+    let clientRecords = this.createClientRecordsFromTree(clientTree);
+    let inspectionInfo = this.inspectServerRecords(serverRecords);
+
+    // Mainly do this to remove deleted items and normalize child guids.
+    serverRecords = inspectionInfo.records;
+    let problemData = inspectionInfo.problemData;
+
+    problemData.clientMissing = [];
+    problemData.serverMissing = [];
+    problemData.differences = [];
+    problemData.good = [];
+
+    let matches = [];
+
+    let allRecords = new Map();
+
+    for (let sr of serverRecords) {
+      allRecords.set(sr.id, {client: null, server: sr});
+    }
+
+    for (let cr of clientRecords) {
+      let unified = allRecords.get(cr.id);
+      if (!unified) {
+        allRecords.set(cr.id, {client: cr, server: null});
+      } else {
+        unified.client = cr;
+      }
+    }
+
+
+    for (let [id, {client, server}] of allRecords) {
+      if (!client && server) {
+        problemData.clientMissing.push(id);
+        continue;
+      }
+      if (!server && client) {
+        problemData.serverMissing.push(id);
+        continue;
+      }
+      let differences = [];
+      if (client.title !== server.title) {
+        differences.push('title');
+      }
+
+      if (client.parentid || server.parentid) {
+        if (client.parentid !== server.parentid) {
+          differences.push('parentid');
+        }
+        // Need to special case 'unfiled' due to it's recent name change
+        // ("Other Bookmarks" vs "Unsorted Bookmarks"), otherwise this has a lot
+        // of false positives.
+        if (client.parentName !== server.parentName && server.parentid !== 'unfiled') {
+          differences.push('parentName');
+        }
+      }
+
+      if (client.tags || server.tags) {
+        let cl = client.tags || [];
+        let sl = server.tags || [];
+        if (cl.length !== sl.length || !cl.every((tag, i) => sl.indexOf(tag) >= 0)) {
+          differences.push('tags');
+        }
+      }
+
+      if (client.type !== server.type) {
+        differences.push('type');
+      } else {
+        switch (server.type) {
+          case 'bookmark':
+          case 'query':
+            if (server.bmkUri !== client.bmkUri) {
+              differences.push('bmkUri');
+            }
+            break;
+          case 'separator':
+            if (server.pos !== client.pos) {
+              differences.push('pos');
+            }
+            break;
+          case 'folder':
+            if (server.id === 'places' && !problemData.rootOnServer) {
+              // It's the fabricated places root. It won't have the GUIDs, but
+              // it doesn't matter.
+              break;
+            }
+            if (client.childGUIDs || server.childGUIDs) {
+              let cl = client.childGUIDs || [];
+              let sl = server.childGUIDs || [];
+              if (cl.length !== sl.length || !cl.every((id, i) => sl[i] === id)) {
+                differences.push('childGUIDs');
+              }
+            }
+            break;
+        }
+      }
+
+      if (differences.length) {
+        problemData.differences.push({id, differences});
+      }
+    }
+    return problemData;
+  }
+};
+
+
+

services/sync/modules/engines/bookmarks.js

@@ -17,19 +17,11 @@ Cu.import("resource://services-sync/constants.js");
 Cu.import("resource://services-sync/engines.js");
 Cu.import("resource://services-sync/record.js");
 Cu.import("resource://services-sync/util.js");
+Cu.import("resource://services-sync/bookmark_utils.js");
 Cu.import("resource://gre/modules/Task.jsm");
 Cu.import("resource://gre/modules/PlacesBackups.jsm");
 
-const ALLBOOKMARKS_ANNO    = "AllBookmarks";
-const DESCRIPTION_ANNO     = "bookmarkProperties/description";
-const SIDEBAR_ANNO         = "bookmarkProperties/loadInSidebar";
-const MOBILEROOT_ANNO      = "mobile/bookmarksRoot";
-const MOBILE_ANNO          = "MobileBookmarks";
-const EXCLUDEBACKUP_ANNO   = "places/excludeFromBackup";
-const SMART_BOOKMARKS_ANNO = "Places/SmartBookmark";
-const PARENT_ANNO          = "sync/parent";
-const ORGANIZERQUERY_ANNO  = "PlacesOrganizer/OrganizerQuery";
-const ANNOS_TO_TRACK = [DESCRIPTION_ANNO, SIDEBAR_ANNO,
+const ANNOS_TO_TRACK = [BookmarkAnnos.DESCRIPTION_ANNO, BookmarkAnnos.SIDEBAR_ANNO,
                         PlacesUtils.LMANNO_FEEDURI, PlacesUtils.LMANNO_SITEURI];
 
 const SERVICE_NOT_SUPPORTED = "Service not supported on this platform";
@@ -134,77 +126,6 @@ BookmarkSeparator.prototype = {
 
 Utils.deferGetSet(BookmarkSeparator, "cleartext", "pos");
 
-
-var kSpecialIds = {
-
-  // Special IDs. Note that mobile can attempt to create a record on
-  // dereference; special accessors are provided to prevent recursion within
-  // observers.
-  guids: ["menu", "places", "tags", "toolbar", "unfiled", "mobile"],
-
-  // Create the special mobile folder to store mobile bookmarks.
-  createMobileRoot: function createMobileRoot() {
-    let root = PlacesUtils.placesRootId;
-    let mRoot = PlacesUtils.bookmarks.createFolder(root, "mobile", -1);
-    PlacesUtils.annotations.setItemAnnotation(
-      mRoot, MOBILEROOT_ANNO, 1, 0, PlacesUtils.annotations.EXPIRE_NEVER);
-    PlacesUtils.annotations.setItemAnnotation(
-      mRoot, EXCLUDEBACKUP_ANNO, 1, 0, PlacesUtils.annotations.EXPIRE_NEVER);
-    return mRoot;
-  },
-
-  findMobileRoot: function findMobileRoot(create) {
-    // Use the (one) mobile root if it already exists.
-    let root = PlacesUtils.annotations.getItemsWithAnnotation(MOBILEROOT_ANNO, {});
-    if (root.length != 0)
-      return root[0];
-
-    if (create)
-      return this.createMobileRoot();
-
-    return null;
-  },
-
-  // Accessors for IDs.
-  isSpecialGUID: function isSpecialGUID(g) {
-    return this.guids.indexOf(g) != -1;
-  },
-
-  specialIdForGUID: function specialIdForGUID(guid, create) {
-    if (guid == "mobile") {
-      return this.findMobileRoot(create);
-    }
-    return this[guid];
-  },
-
-  // Don't bother creating mobile: if it doesn't exist, this ID can't be it!
-  specialGUIDForId: function specialGUIDForId(id) {
-    for (let guid of this.guids)
-      if (this.specialIdForGUID(guid, false) == id)
-        return guid;
-    return null;
-  },
-
-  get menu() {
-    return PlacesUtils.bookmarksMenuFolderId;
-  },
-  get places() {
-    return PlacesUtils.placesRootId;
-  },
-  get tags() {
-    return PlacesUtils.tagsFolderId;
-  },
-  get toolbar() {
-    return PlacesUtils.toolbarFolderId;
-  },
-  get unfiled() {
-    return PlacesUtils.unfiledBookmarksFolderId;
-  },
-  get mobile() {
-    return this.findMobileRoot(true);
-  },
-};
-
 this.BookmarksEngine = function BookmarksEngine(service) {
   SyncEngine.call(this, "Bookmarks", service);
 }
@@ -291,7 +212,7 @@ BookmarksEngine.prototype = {
 
     function* walkBookmarksRoots(tree, rootGUIDs) {
       for (let guid of rootGUIDs) {
-        let id = kSpecialIds.specialIdForGUID(guid, false);
+        let id = BookmarkSpecialIds.specialIdForGUID(guid, false);
         let bookmarkRoot = id === null ? null :
           tree.children.find(child => child.id === id);
         if (bookmarkRoot === null) {
@@ -301,19 +222,19 @@ BookmarksEngine.prototype = {
       }
     }
 
-    let rootsToWalk = kSpecialIds.guids.filter(guid =>
+    let rootsToWalk = BookmarkSpecialIds.guids.filter(guid =>
       guid !== 'places' && guid !== 'tags');
 
     for (let [node, parent] of walkBookmarksRoots(tree, rootsToWalk)) {
       let {guid, id, type: placeType} = node;
-      guid = kSpecialIds.specialGUIDForId(id) || guid;
+      guid = BookmarkSpecialIds.specialGUIDForId(id) || 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 === SMART_BOOKMARKS_ANNO);
+            query = node.annos.find(({name}) => name === BookmarkAnnos.SMART_BOOKMARKS_ANNO);
           }
           if (query && query.value) {
             key = "q" + query.value;
@@ -584,7 +505,7 @@ BookmarksStore.prototype = {
   
   applyIncoming: function BStore_applyIncoming(record) {
     this._log.debug("Applying record " + record.id);
-    let isSpecial = record.id in kSpecialIds;
+    let isSpecial = record.id in BookmarkSpecialIds;
 
     if (record.deleted) {
       if (isSpecial) {
@@ -651,7 +572,7 @@ BookmarksStore.prototype = {
       // Create an annotation to remember that it needs reparenting.
       if (record._orphan) {
         PlacesUtils.annotations.setItemAnnotation(
-          itemId, PARENT_ANNO, parentGUID, 0,
+          itemId, BookmarkAnnos.PARENT_ANNO, parentGUID, 0,
           PlacesUtils.annotations.EXPIRE_NEVER);
       }
     }
@@ -673,13 +594,13 @@ BookmarksStore.prototype = {
   _reparentOrphans: function _reparentOrphans(parentId) {
     // Find orphans and reunite with this folder parent
     let parentGUID = this.GUIDForId(parentId);
-    let orphans = this._findAnnoItems(PARENT_ANNO, parentGUID);
+    let orphans = this._findAnnoItems(BookmarkAnnos.PARENT_ANNO, parentGUID);
 
     this._log.debug("Reparenting orphans " + orphans + " to " + parentId);
     orphans.forEach(function(orphan) {
       // Move the orphan to the parent and drop the missing parent annotation
       if (this._reparentItem(orphan, parentId)) {
-        PlacesUtils.annotations.removeItemAnnotation(orphan, PARENT_ANNO);
+        PlacesUtils.annotations.removeItemAnnotation(orphan, BookmarkAnnos.PARENT_ANNO);
       }
     }, this);
   },
@@ -737,7 +658,7 @@ BookmarksStore.prototype = {
     // false for that, too!
     if (!(record._parent > 0)) {
       this._log.debug("Parent is " + record._parent + "; reparenting to unfiled.");
-      record._parent = kSpecialIds.unfiled;
+      record._parent = BookmarkSpecialIds.unfiled;
     }
 
     switch (record.type) {
@@ -754,7 +675,7 @@ BookmarksStore.prototype = {
       // Smart bookmark annotations are strings.
       if (record.queryId) {
         PlacesUtils.annotations.setItemAnnotation(
-          newId, SMART_BOOKMARKS_ANNO, record.queryId, 0,
+          newId, BookmarkAnnos.SMART_BOOKMARKS_ANNO, record.queryId, 0,
           PlacesUtils.annotations.EXPIRE_NEVER);
       }
 
@@ -764,13 +685,13 @@ BookmarksStore.prototype = {
       PlacesUtils.bookmarks.setKeywordForBookmark(newId, record.keyword);
       if (record.description) {
         PlacesUtils.annotations.setItemAnnotation(
-          newId, DESCRIPTION_ANNO, record.description, 0,
+          newId, BookmarkAnnos.DESCRIPTION_ANNO, record.description, 0,
           PlacesUtils.annotations.EXPIRE_NEVER);
       }
 
       if (record.loadInSidebar) {
         PlacesUtils.annotations.setItemAnnotation(
-          newId, SIDEBAR_ANNO, true, 0,
+          newId, BookmarkAnnos.SIDEBAR_ANNO, true, 0,
           PlacesUtils.annotations.EXPIRE_NEVER);
       }
 
@@ -784,7 +705,7 @@ BookmarksStore.prototype = {
 
       if (record.description) {
         PlacesUtils.annotations.setItemAnnotation(
-          newId, DESCRIPTION_ANNO, record.description, 0,
+          newId, BookmarkAnnos.DESCRIPTION_ANNO, record.description, 0,
           PlacesUtils.annotations.EXPIRE_NEVER);
       }
 
@@ -874,7 +795,7 @@ BookmarksStore.prototype = {
   },
 
   remove: function BStore_remove(record) {
-    if (kSpecialIds.isSpecialGUID(record.id)) {
+    if (BookmarkSpecialIds.isSpecialGUID(record.id)) {
       this._log.warn("Refusing to remove special folder " + record.id);
       return;
     }
@@ -950,24 +871,24 @@ BookmarksStore.prototype = {
       case "description":
         if (val) {
           PlacesUtils.annotations.setItemAnnotation(
-            itemId, DESCRIPTION_ANNO, val, 0,
+            itemId, BookmarkAnnos.DESCRIPTION_ANNO, val, 0,
             PlacesUtils.annotations.EXPIRE_NEVER);
         } else {
-          PlacesUtils.annotations.removeItemAnnotation(itemId, DESCRIPTION_ANNO);
+          PlacesUtils.annotations.removeItemAnnotation(itemId, BookmarkAnnos.DESCRIPTION_ANNO);
         }
         break;
       case "loadInSidebar":
         if (val) {
           PlacesUtils.annotations.setItemAnnotation(
-            itemId, SIDEBAR_ANNO, true, 0,
+            itemId, BookmarkAnnos.SIDEBAR_ANNO, true, 0,
             PlacesUtils.annotations.EXPIRE_NEVER);
         } else {
-          PlacesUtils.annotations.removeItemAnnotation(itemId, SIDEBAR_ANNO);
+          PlacesUtils.annotations.removeItemAnnotation(itemId, BookmarkAnnos.SIDEBAR_ANNO);
         }
         break;
       case "queryId":
         PlacesUtils.annotations.setItemAnnotation(
-          itemId, SMART_BOOKMARKS_ANNO, val, 0,
+          itemId, BookmarkAnnos.SMART_BOOKMARKS_ANNO, val, 0,
           PlacesUtils.annotations.EXPIRE_NEVER);
         break;
       }
@@ -1032,14 +953,14 @@ BookmarksStore.prototype = {
 
   _getDescription: function BStore__getDescription(id) {
     try {
-      return PlacesUtils.annotations.getItemAnnotation(id, DESCRIPTION_ANNO);
+      return PlacesUtils.annotations.getItemAnnotation(id, BookmarkAnnos.DESCRIPTION_ANNO);
     } catch (e) {
       return null;
     }
   },
 
   _isLoadInSidebar: function BStore__isLoadInSidebar(id) {
-    return PlacesUtils.annotations.itemHasAnnotation(id, SIDEBAR_ANNO);
+    return PlacesUtils.annotations.itemHasAnnotation(id, BookmarkAnnos.SIDEBAR_ANNO);
   },
 
   get _childGUIDsStm() {
@@ -1095,9 +1016,9 @@ BookmarksStore.prototype = {
         
         // Persist the Smart Bookmark anno, if found.
         try {
-          let anno = PlacesUtils.annotations.getItemAnnotation(placeId, SMART_BOOKMARKS_ANNO);
+          let anno = PlacesUtils.annotations.getItemAnnotation(placeId, BookmarkAnnos.SMART_BOOKMARKS_ANNO);
           if (anno != null) {
-            this._log.trace("query anno: " + SMART_BOOKMARKS_ANNO +
+            this._log.trace("query anno: " + BookmarkAnnos.SMART_BOOKMARKS_ANNO +
                             " = " + anno);
             record.queryId = anno;
           }
@@ -1207,7 +1128,7 @@ BookmarksStore.prototype = {
   _guidForIdCols: ["guid"],
 
   GUIDForId: function GUIDForId(id) {
-    let special = kSpecialIds.specialGUIDForId(id);
+    let special = BookmarkSpecialIds.specialGUIDForId(id);
     if (special)
       return special;
 
@@ -1234,8 +1155,8 @@ BookmarksStore.prototype = {
   // noCreate is provided as an optional argument to prevent the creation of
   // non-existent special records, such as "mobile".
   idForGUID: function idForGUID(guid, noCreate) {
-    if (kSpecialIds.isSpecialGUID(guid))
-      return kSpecialIds.specialIdForGUID(guid, !noCreate);
+    if (BookmarkSpecialIds.isSpecialGUID(guid))
+      return BookmarkSpecialIds.specialIdForGUID(guid, !noCreate);
 
     let stmt = this._idForGUIDStm;
     // guid might be a String object rather than a string.
@@ -1356,10 +1277,10 @@ BookmarksStore.prototype = {
     // We also want "mobile" but only if a local mobile folder already exists
     // (otherwise we'll later end up creating it, which we want to avoid until
     // we actually need it.)
-    if (kSpecialIds.findMobileRoot(false)) {
+    if (BookmarkSpecialIds.findMobileRoot(false)) {
       items["mobile"] = true;
     }
-    for (let guid of kSpecialIds.guids) {
+    for (let guid of BookmarkSpecialIds.guids) {
       if (guid != "places" && guid != "tags")
         this._getChildren(guid, items);
     }
@@ -1371,9 +1292,9 @@ BookmarksStore.prototype = {
     Task.spawn(function* () {
       // Save a backup before clearing out all bookmarks.
       yield PlacesBackups.create(null, true);
-      for (let guid of kSpecialIds.guids)
+      for (let guid of BookmarkSpecialIds.guids)
         if (guid != "places") {
-          let id = kSpecialIds.specialIdForGUID(guid);
+          let id = BookmarkSpecialIds.specialIdForGUID(guid);
           if (id)
             PlacesUtils.bookmarks.removeFolderChildren(id);
         }
@@ -1442,7 +1363,7 @@ BookmarksTracker.prototype = {
    *        GUID of the bookmark to upload.
    */
   _add: function BMT__add(itemId, guid) {
-    guid = kSpecialIds.specialGUIDForId(itemId) || guid;
+    guid = BookmarkSpecialIds.specialGUIDForId(itemId) || guid;
     if (this.addChangedID(guid))
       this._upScore();
   },
@@ -1480,7 +1401,7 @@ BookmarksTracker.prototype = {
     }
 
     // Ignore changes to tags (folders under the tags folder).
-    let tags = kSpecialIds.tags;
+    let tags = BookmarkSpecialIds.tags;
     if (folder == tags)
       return true;
 
@@ -1489,7 +1410,7 @@ BookmarksTracker.prototype = {
       return true;
 
     // Make sure to remove items that have the exclude annotation.
-    if (PlacesUtils.annotations.itemHasAnnotation(itemId, EXCLUDEBACKUP_ANNO)) {
+    if (PlacesUtils.annotations.itemHasAnnotation(itemId, BookmarkAnnos.EXCLUDEBACKUP_ANNO)) {
       this.removeChangedID(guid);
       return true;
     }
@@ -1521,33 +1442,33 @@ BookmarksTracker.prototype = {
 
   _ensureMobileQuery: function _ensureMobileQuery() {
     let find = val =>
-      PlacesUtils.annotations.getItemsWithAnnotation(ORGANIZERQUERY_ANNO, {}).filter(
-        id => PlacesUtils.annotations.getItemAnnotation(id, ORGANIZERQUERY_ANNO) == val
+      PlacesUtils.annotations.getItemsWithAnnotation(BookmarkAnnos.ORGANIZERQUERY_ANNO, {}).filter(
+        id => PlacesUtils.annotations.getItemAnnotation(id, BookmarkAnnos.ORGANIZERQUERY_ANNO) == val
       );
 
     // Don't continue if the Library isn't ready
-    let all = find(ALLBOOKMARKS_ANNO);
+    let all = find(BookmarkAnnos.ALLBOOKMARKS_ANNO);
     if (all.length == 0)
       return;
 
     // Disable handling of notifications while changing the mobile query
     this.ignoreAll = true;
 
-    let mobile = find(MOBILE_ANNO);
-    let queryURI = Utils.makeURI("place:folder=" + kSpecialIds.mobile);
+    let mobile = find(BookmarkAnnos.MOBILE_ANNO);
+    let queryURI = Utils.makeURI("place:folder=" + BookmarkSpecialIds.mobile);
     let title = Str.sync.get("mobile.label");
 
     // Don't add OR remove the mobile bookmarks if there's nothing.
-    if (PlacesUtils.bookmarks.getIdForItemAt(kSpecialIds.mobile, 0) == -1) {
+    if (PlacesUtils.bookmarks.getIdForItemAt(BookmarkSpecialIds.mobile, 0) == -1) {
       if (mobile.length != 0)
         PlacesUtils.bookmarks.removeItem(mobile[0]);
     }
     // Add the mobile bookmarks query if it doesn't exist
     else if (mobile.length == 0) {
       let query = PlacesUtils.bookmarks.insertBookmark(all[0], queryURI, -1, title);
-      PlacesUtils.annotations.setItemAnnotation(query, ORGANIZERQUERY_ANNO, MOBILE_ANNO, 0,
+      PlacesUtils.annotations.setItemAnnotation(query, BookmarkAnnos.ORGANIZERQUERY_ANNO, BookmarkAnnos.MOBILE_ANNO, 0,
                                   PlacesUtils.annotations.EXPIRE_NEVER);
-      PlacesUtils.annotations.setItemAnnotation(query, EXCLUDEBACKUP_ANNO, 1, 0,
+      PlacesUtils.annotations.setItemAnnotation(query, BookmarkAnnos.EXCLUDEBACKUP_ANNO, 1, 0,
                                   PlacesUtils.annotations.EXPIRE_NEVER);
     }
     // Make sure the existing title is correct
@@ -1599,7 +1520,7 @@ BookmarksTracker.prototype = {
     }
 
     // Remove any position annotations now that the user moved the item
-    PlacesUtils.annotations.removeItemAnnotation(itemId, PARENT_ANNO);
+    PlacesUtils.annotations.removeItemAnnotation(itemId, BookmarkAnnos.PARENT_ANNO);
   },
 
   onBeginUpdateBatch: function () {},

services/sync/moz.build

@@ -19,6 +19,8 @@ EXTRA_COMPONENTS += [
 EXTRA_JS_MODULES['services-sync'] += [
     'modules/addonsreconciler.js',
     'modules/addonutils.js',
+    'modules/bookmark_utils.js',
+    'modules/bookmark_validator.js',
     'modules/browserid_identity.js',
     'modules/engines.js',
     'modules/FxaMigrator.jsm',

services/sync/tests/unit/test_bookmark_validator.js

@@ -0,0 +1,242 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+Components.utils.import("resource://services-sync/bookmark_validator.js");
+
+function inspectServerRecords(data) {
+  return new BookmarkValidator().inspectServerRecords(data);
+}
+
+add_test(function test_isr_rootOnServer() {
+  let c = inspectServerRecords([{
+    id: 'places',
+    type: 'folder',
+    children: [],
+  }]);
+  ok(c.problemData.rootOnServer);
+  run_next_test();
+});
+
+add_test(function test_isr_empty() {
+  let c = inspectServerRecords([]);
+  ok(!c.problemData.rootOnServer);
+  notEqual(c.root, null);
+  run_next_test();
+});
+
+add_test(function test_isr_cycles() {
+  let c = inspectServerRecords([
+    {id: 'C', type: 'folder', children: ['A', 'B'], parentid: 'places'},
+    {id: 'A', type: 'folder', children: ['B'], parentid: 'B'},
+    {id: 'B', type: 'folder', children: ['A'], parentid: 'A'},
+  ]).problemData;
+
+  equal(c.cycles.length, 1);
+  ok(c.cycles[0].indexOf('A') >= 0);
+  ok(c.cycles[0].indexOf('B') >= 0);
+  run_next_test();
+});
+
+add_test(function test_isr_orphansMultiParents() {
+  let c = inspectServerRecords([
+    { id: 'A', type: 'bookmark', parentid: 'D' },
+    { id: 'B', type: 'folder', parentid: 'places', children: ['A']},
+    { id: 'C', type: 'folder', parentid: 'places', children: ['A']},
+  ]).problemData;
+  equal(c.orphans.length, 1);
+  equal(c.orphans[0].id, 'A');
+  equal(c.multipleParents.length, 1);
+  equal(c.multipleParents[0].child, 'A');
+  ok(c.multipleParents[0].parents.indexOf('B') >= 0);
+  ok(c.multipleParents[0].parents.indexOf('C') >= 0);
+  run_next_test();
+});
+
+add_test(function test_isr_deletedParents() {
+  let c = inspectServerRecords([
+    { id: 'A', type: 'bookmark', parentid: 'B' },
+    { id: 'B', type: 'folder', parentid: 'places', children: ['A']},
+    { id: 'B', type: 'item', deleted: true},
+  ]).problemData;
+  deepEqual(c.deletedParents, ['A'])
+  run_next_test();
+});
+
+add_test(function test_isr_badChildren() {
+  let c = inspectServerRecords([
+    { id: 'A', type: 'bookmark', parentid: 'places', children: ['B', 'C'] },
+    { id: 'C', type: 'bookmark', parentid: 'A' }
+  ]).problemData;
+  deepEqual(c.childrenOnNonFolder, ['A'])
+  deepEqual(c.missingChildren, [{parent: 'A', child: 'B'}]);
+  deepEqual(c.parentNotFolder, ['C']);
+  run_next_test();
+});
+
+
+add_test(function test_isr_parentChildMismatches() {
+  let c = inspectServerRecords([
+    { id: 'A', type: 'folder', parentid: 'places', children: [] },
+    { id: 'B', type: 'bookmark', parentid: 'A' }
+  ]).problemData;
+  deepEqual(c.parentChildMismatches, [{parent: 'A', child: 'B'}]);
+  run_next_test();
+});
+
+add_test(function test_isr_duplicatesAndMissingIDs() {
+  let c = inspectServerRecords([
+    {id: 'A', type: 'folder', parentid: 'places', children: []},
+    {id: 'A', type: 'folder', parentid: 'places', children: []},
+    {type: 'folder', parentid: 'places', children: []}
+  ]).problemData;
+  equal(c.missingIDs, 1);
+  deepEqual(c.duplicates, ['A']);
+  run_next_test();
+});
+
+add_test(function test_isr_wrongParentName() {
+  let c = inspectServerRecords([
+    {id: 'A', type: 'folder', title: 'My Amazing Bookmarks', parentName: '', parentid: 'places', children: ['B']},
+    {id: 'B', type: 'bookmark', title: '', parentName: 'My Awesome Bookmarks', parentid: 'A'},
+  ]).problemData;
+  deepEqual(c.wrongParentName, ['B'])
+  run_next_test();
+});
+
+add_test(function test_isr_duplicateChildren()  {
+  let c = inspectServerRecords([
+    {id: 'A', type: 'folder', parentid: 'places', children: ['B', 'B']},
+    {id: 'B', type: 'bookmark', parentid: 'A'},
+  ]).problemData;
+  deepEqual(c.duplicateChildren, ['A']);
+  run_next_test();
+});
+
+// Each compareServerWithClient test mutates these, so we can't just keep them
+// global
+function getDummyServerAndClient() {
+  let server = [
+    {
+      id: 'aaaaaaaaaaaa',
+      parentid: 'places',
+      type: 'folder',
+      parentName: '',
+      title: 'foo',
+      children: ['bbbbbbbbbbbb', 'cccccccccccc']
+    },
+    {
+      id: 'bbbbbbbbbbbb',
+      type: 'bookmark',
+      parentid: 'aaaaaaaaaaaa',
+      parentName: 'foo',
+      title: 'bar',
+      bmkUri: 'http://baz.com'
+    },
+    {
+      id: 'cccccccccccc',
+      parentid: 'aaaaaaaaaaaa',
+      parentName: 'foo',
+      title: '',
+      type: 'query',
+      bmkUri: 'place:type=6&sort=14&maxResults=10'
+    }
+  ];
+
+  let client = {
+    "guid": "root________",
+    "title": "",
+    "id": 1,
+    "type": "text/x-moz-place-container",
+    "children": [
+      {
+        "guid": "aaaaaaaaaaaa",
+        "title": "foo",
+        "id": 1000,
+        "type": "text/x-moz-place-container",
+        "children": [
+          {
+            "guid": "bbbbbbbbbbbb",
+            "title": "bar",
+            "id": 1001,
+            "type": "text/x-moz-place",
+            "uri": "http://baz.com"
+          },
+          {
+            "guid": "cccccccccccc",
+            "title": "",
+            "id": 1002,
+            "annos": [{
+              "name": "Places/SmartBookmark",
+              "flags": 0,
+              "expires": 4,
+              "value": "RecentTags"
+            }],
+            "type": "text/x-moz-place",
+            "uri": "place:type=6&sort=14&maxResults=10"
+          }
+        ]
+      }
+    ]
+  };
+  return {server, client};
+}
+
+
+add_test(function test_cswc_valid() {
+  let {server, client} = getDummyServerAndClient();
+
+  let c = new BookmarkValidator().compareServerWithClient(server, client);
+  equal(c.clientMissing.length, 0);
+  equal(c.serverMissing.length, 0);
+  equal(c.differences.length, 0);
+  run_next_test();
+});
+
+add_test(function test_cswc_serverMissing() {
+  let {server, client} = getDummyServerAndClient();
+  // remove c
+  server.pop();
+  server[0].children.pop();
+
+  let c = new BookmarkValidator().compareServerWithClient(server, client);
+  deepEqual(c.serverMissing, ['cccccccccccc']);
+  equal(c.clientMissing.length, 0);
+  deepEqual(c.differences, [{id: 'aaaaaaaaaaaa', differences: ['childGUIDs']}]);
+  run_next_test();
+});
+
+add_test(function test_cswc_clientMissing() {
+  let {server, client} = getDummyServerAndClient();
+  client.children[0].children.pop();
+
+  let c = new BookmarkValidator().compareServerWithClient(server, client);
+  deepEqual(c.clientMissing, ['cccccccccccc']);
+  equal(c.serverMissing.length, 0);
+  deepEqual(c.differences, [{id: 'aaaaaaaaaaaa', differences: ['childGUIDs']}]);
+  run_next_test();
+});
+
+add_test(function test_cswc_differences() {
+  {
+    let {server, client} = getDummyServerAndClient();
+    client.children[0].children[0].title = 'asdf';
+    let c = new BookmarkValidator().compareServerWithClient(server, client);
+    equal(c.clientMissing.length, 0);
+    equal(c.serverMissing.length, 0);
+    deepEqual(c.differences, [{id: 'bbbbbbbbbbbb', differences: ['title']}]);
+  }
+
+  {
+    let {server, client} = getDummyServerAndClient();
+    server[2].type = 'bookmark';
+    let c = new BookmarkValidator().compareServerWithClient(server, client);
+    equal(c.clientMissing.length, 0);
+    equal(c.serverMissing.length, 0);
+    deepEqual(c.differences, [{id: 'cccccccccccc', differences: ['type']}]);
+  }
+  run_next_test();
+});
+
+function run_test() {
+  run_next_test();
+}

services/sync/tests/unit/xpcshell.ini

@@ -154,6 +154,7 @@ tags = addons
 # Too many intermittent "ASSERTION: thread pool wasn't shutdown: '!mPool'" (bug 804479)
 skip-if = debug
 [test_bookmark_tracker.js]
+[test_bookmark_validator.js]
 [test_clients_engine.js]
 [test_clients_escape.js]
 [test_forms_store.js]