Bug 618403 - Orphan reparenting too aggressive, can lead to double bookmarks on storage version upgrade. r=rnewman

services/sync/modules/engines/bookmarks.js

@@ -325,6 +325,12 @@ BookmarksStore.prototype = {
   },
 
   applyIncoming: function BStore_applyIncoming(record) {
+    // Don't bother with pre and post-processing for deletions.
+    if (record.deleted) {
+      Store.prototype.applyIncoming.apply(this, arguments);
+      return;
+    }
+
     // For special folders we're only interested in child ordering.
     if ((record.id in kSpecialIds) && record.children) {
       this._log.debug("Processing special node: " + record.id);
@@ -367,19 +373,19 @@ BookmarksStore.prototype = {
 
     // Figure out the local id of the parent GUID if available
     let parentGUID = record.parentid;
-    record._orphan = false;
+    if (!parentGUID) {
-    if (parentGUID != null) {
+      throw "Record " + record.id + " has invalid parentid: " + parentGUID;
-      let parentId = this.idForGUID(parentGUID);
+    }
-
-      // Default to unfiled if we don't have the parent yet
-      if (parentId <= 0) {
-        this._log.trace("Reparenting to unfiled until parent is synced");
-        record._orphan = true;
-        parentId = kSpecialIds.unfiled;
-      }
 
+    let parentId = this.idForGUID(parentGUID);
+    if (parentId > 0) {
       // Save the parent id for modifying the bookmark later
       record._parent = parentId;
+      record._orphan = false;
+    } else {
+      this._log.trace("Record " + record.id +
+                      " is an orphan: could not find parent " + parentGUID);
+      record._orphan = true;
     }
 
     // Do the normal processing of incoming records
@@ -396,7 +402,7 @@ BookmarksStore.prototype = {
           this._childrenToOrder[record.id] = record.children;
       }
 
-      // Create an annotation to remember that it needs a parent
+      // Create an annotation to remember that it needs reparenting.
       if (record._orphan)
         Utils.anno(itemId, PARENT_ANNO, parentGUID);
     }
@@ -421,12 +427,32 @@ BookmarksStore.prototype = {
     this._log.debug("Reparenting orphans " + orphans + " to " + parentId);
     orphans.forEach(function(orphan) {
       // Move the orphan to the parent and drop the missing parent annotation
-      Svc.Bookmark.moveItem(orphan, parentId, Svc.Bookmark.DEFAULT_INDEX);
+      if (this._reparentItem(orphan, parentId)) {
-      Svc.Annos.removeItemAnnotation(orphan, PARENT_ANNO);
+        Svc.Annos.removeItemAnnotation(orphan, PARENT_ANNO);
+      }
     }, this);
   },
 
+  _reparentItem: function _reparentItem(itemId, parentId) {
+    this._log.trace("Attempting to move item " + itemId + " to new parent " +
+                    parentId);
+    try {
+      if (parentId > 0) {
+        Svc.Bookmark.moveItem(itemId, parentId, Svc.Bookmark.DEFAULT_INDEX);
+        return true;
+      }
+    } catch(ex) {
+      this._log.debug("Failed to reparent item. " + Utils.exceptionStr(ex));
+    }
+    return false;
+  },
+
   create: function BStore_create(record) {
+    // Default to unfiled if we don't have the parent yet
+    if (!record._parent) {
+      record._parent = kSpecialIds.unfiled;
+    }
+
     let newId;
     switch (record.type) {
     case "bookmark":
@@ -438,7 +464,9 @@ BookmarksStore.prototype = {
       this._log.debug(["created bookmark", newId, "under", record._parent,
                        "as", record.title, record.bmkUri].join(" "));
 
-      this._tagURI(uri, record.tags);
+      if (Utils.isArray(record.tags)) {
+        this._tagURI(uri, record.tags);
+      }
       this._bms.setKeywordForBookmark(newId, record.keyword);
       if (record.description)
         Utils.anno(newId, "bookmarkProperties/description", record.description);
@@ -540,26 +568,30 @@ BookmarksStore.prototype = {
     this._log.trace("Updating " + record.id + " (" + itemId + ")");
 
     // Move the bookmark to a new parent or new position if necessary
-    if (Svc.Bookmark.getFolderIdForItem(itemId) != record._parent) {
+    if (record._parent > 0 &&
-      this._log.trace("Moving item to a new parent.");
+        Svc.Bookmark.getFolderIdForItem(itemId) != record._parent) {
-      Svc.Bookmark.moveItem(itemId, record._parent, Svc.Bookmark.DEFAULT_INDEX);
+      this._reparentItem(itemId, record._parent);
     }
 
     for (let [key, val] in Iterator(record.cleartext)) {
       switch (key) {
       case "title":
+        val = val || "";
         this._bms.setItemTitle(itemId, val);
         break;
       case "bmkUri":
         this._bms.changeBookmarkURI(itemId, Utils.makeURI(val));
         break;
       case "tags":
-        this._tagURI(this._bms.getBookmarkURI(itemId), val);
+        if (Utils.isArray(val)) {
+          this._tagURI(this._bms.getBookmarkURI(itemId), val);
+        }
         break;
       case "keyword":
         this._bms.setKeywordForBookmark(itemId, val);
         break;
       case "description":
+        val = val || "";
         Utils.anno(itemId, "bookmarkProperties/description", val);
         break;
       case "loadInSidebar":
@@ -644,7 +676,7 @@ BookmarksStore.prototype = {
     try {
       return Utils.anno(id, "bookmarkProperties/description");
     } catch (e) {
-      return undefined;
+      return null;
     }
   },
 

services/sync/tests/unit/test_bookmark_store.js

@@ -3,12 +3,15 @@ Cu.import("resource://services-sync/engines/bookmarks.js");
 Cu.import("resource://services-sync/type_records/bookmark.js");
 Cu.import("resource://services-sync/util.js");
 
+const PARENT_ANNO = "sync/parent";
+
 Engines.register(BookmarksEngine);
 let engine = Engines.get("bookmarks");
 let store = engine._store;
 let fxuri = Utils.makeURI("http://getfirefox.com/");
 let tburi = Utils.makeURI("http://getthunderbird.com/");
 
+
 function test_bookmark_create() {
   try {
     _("Ensure the record isn't present yet.");
@@ -31,7 +34,10 @@ function test_bookmark_create() {
     let id = store.idForGUID(fxrecord.id);
     do_check_eq(store.GUIDForId(id), fxrecord.id);
     do_check_eq(Svc.Bookmark.getItemType(id), Svc.Bookmark.TYPE_BOOKMARK);
+    do_check_true(Svc.Bookmark.getBookmarkURI(id).equals(fxuri));
     do_check_eq(Svc.Bookmark.getItemTitle(id), fxrecord.title);
+    do_check_eq(Utils.anno(id, "bookmarkProperties/description"),
+                fxrecord.description);
     do_check_eq(Svc.Bookmark.getFolderIdForItem(id),
                 Svc.Bookmark.toolbarFolder);
     do_check_eq(Svc.Bookmark.getKeywordForBookmark(id), fxrecord.keyword);
@@ -40,13 +46,85 @@ function test_bookmark_create() {
     let newrecord = store.createRecord(fxrecord.id);
     do_check_true(newrecord instanceof Bookmark);
     for each (let property in ["type", "bmkUri", "description", "title",
-                               "keyword", "parentName", "parentid"])
+                               "keyword", "parentName", "parentid"]) {
       do_check_eq(newrecord[property], fxrecord[property]);
+    }
     do_check_true(Utils.deepEquals(newrecord.tags.sort(),
                                    fxrecord.tags.sort()));
 
     _("The calculated sort index is based on frecency data.");
     do_check_true(newrecord.sortindex >= 150);
+
+    _("Create a record with some values missing.");
+    let tbrecord = new Bookmark("bookmarks", "thunderbird1");
+    tbrecord.bmkUri        = tburi.spec;
+    tbrecord.parentName    = "Bookmarks Toolbar";
+    tbrecord.parentid      = "toolbar";
+    store.applyIncoming(tbrecord);
+
+    _("Verify it has been created correctly.");
+    id = store.idForGUID(tbrecord.id);
+    do_check_eq(store.GUIDForId(id), tbrecord.id);
+    do_check_eq(Svc.Bookmark.getItemType(id), Svc.Bookmark.TYPE_BOOKMARK);
+    do_check_true(Svc.Bookmark.getBookmarkURI(id).equals(tburi));
+    do_check_eq(Svc.Bookmark.getItemTitle(id), null);
+    let error;
+    try {
+      Utils.anno(id, "bookmarkProperties/description");
+    } catch(ex) {
+      error = ex;
+    }
+    do_check_eq(error.result, Cr.NS_ERROR_NOT_AVAILABLE);
+    do_check_eq(Svc.Bookmark.getFolderIdForItem(id),
+                Svc.Bookmark.toolbarFolder);
+    do_check_eq(Svc.Bookmark.getKeywordForBookmark(id), null);
+  } finally {
+    _("Clean up.");
+    store.wipe();
+  }
+}
+
+function test_bookmark_update() {
+  try {
+    _("Create a bookmark whose values we'll change.");
+    let bmk1_id = Svc.Bookmark.insertBookmark(
+      Svc.Bookmark.toolbarFolder, fxuri, Svc.Bookmark.DEFAULT_INDEX,
+      "Get Firefox!");
+    Utils.anno(bmk1_id, "bookmarkProperties/description", "Firefox is awesome.");
+    Svc.Bookmark.setKeywordForBookmark(bmk1_id, "firefox");
+    let bmk1_guid = store.GUIDForId(bmk1_id);
+
+    _("Update the record with some null values.");
+    let record = store.createRecord(bmk1_guid);
+    record.title = null;
+    record.description = null;
+    record.keyword = null;
+    record.tags = null;
+    store.applyIncoming(record);
+
+    _("Verify that the values have been cleared.");
+    do_check_eq(Utils.anno(bmk1_id, "bookmarkProperties/description"), "");
+    do_check_eq(Svc.Bookmark.getItemTitle(bmk1_id), "");
+    do_check_eq(Svc.Bookmark.getKeywordForBookmark(bmk1_id), null);
+  } finally {
+    _("Clean up.");
+    store.wipe();
+  }
+}
+
+function test_bookmark_createRecord() {
+  try {
+    _("Create a bookmark without a description or title.");
+    let bmk1_id = Svc.Bookmark.insertBookmark(
+      Svc.Bookmark.toolbarFolder, fxuri, Svc.Bookmark.DEFAULT_INDEX, null);
+    let bmk1_guid = store.GUIDForId(bmk1_id);
+
+    _("Verify that the record is created accordingly.");
+    let record = store.createRecord(bmk1_guid);
+    do_check_eq(record.title, null);
+    do_check_eq(record.description, null);
+    do_check_eq(record.keyword, null);
+
   } finally {
     _("Clean up.");
     store.wipe();
@@ -116,6 +194,37 @@ function test_folder_createRecord() {
   }
 }
 
+function test_deleted() {
+  try {
+    _("Create a bookmark that will be deleted.");
+    let bmk1_id = Svc.Bookmark.insertBookmark(
+      Svc.Bookmark.toolbarFolder, fxuri, Svc.Bookmark.DEFAULT_INDEX,
+      "Get Firefox!");
+    let bmk1_guid = store.GUIDForId(bmk1_id);
+
+    _("Delete the bookmark through the store.");
+    let record = new PlacesItem("bookmarks", bmk1_guid);
+    record.deleted = true;
+    store.applyIncoming(record);
+
+    _("Ensure it has been deleted.");
+    let error;
+    try {
+      Svc.Bookmark.getBookmarkURI(bmk1_id);
+    } catch(ex) {
+      error = ex;
+    }
+    do_check_eq(error.result, Cr.NS_ERROR_ILLEGAL_VALUE);
+
+    let newrec = store.createRecord(bmk1_guid);
+    do_check_eq(newrec.deleted, true);
+
+  } finally {
+    _("Clean up.");
+    store.wipe();
+  }
+}
+
 function test_move_folder() {
   try {
     _("Create two folders and a bookmark in one of them.");
@@ -133,7 +242,6 @@ function test_move_folder() {
     let record = store.createRecord(bmk_guid);
     do_check_eq(record.parentid, folder1_guid);
     record.parentid = folder2_guid;
-    record.description = ""; //TODO for some reason we need this
     store.applyIncoming(record);
 
     _("Verify the new parent.");
@@ -188,10 +296,69 @@ function test_move_order() {
   }
 }
 
+function test_orphan() {
+  try {
+
+    _("Add a new bookmark locally.");
+    let bmk1_id = Svc.Bookmark.insertBookmark(
+      Svc.Bookmark.toolbarFolder, fxuri, Svc.Bookmark.DEFAULT_INDEX,
+      "Get Firefox!");
+    let bmk1_guid = store.GUIDForId(bmk1_id);
+    do_check_eq(Svc.Bookmark.getFolderIdForItem(bmk1_id), Svc.Bookmark.toolbarFolder);
+    let error;
+    try {
+      Utils.anno(bmk1_id, PARENT_ANNO);
+    } catch(ex) {
+      error = ex;
+    }
+    do_check_eq(error.result, Cr.NS_ERROR_NOT_AVAILABLE);
+
+    _("Apply a server record that is the same but refers to non-existent folder.");
+    let record = store.createRecord(bmk1_guid);
+    record.parentid = "non-existent";
+    store.applyIncoming(record);
+
+    _("Verify that bookmark has been flagged as orphan, has not moved.");
+    do_check_eq(Svc.Bookmark.getFolderIdForItem(bmk1_id), Svc.Bookmark.toolbarFolder);
+    do_check_eq(Utils.anno(bmk1_id, PARENT_ANNO), "non-existent");
+
+  } finally {
+    _("Clean up.");
+    store.wipe();
+  }
+}
+
+function test_reparentOrphans() {
+  try {
+    let folder1_id = Svc.Bookmark.createFolder(
+      Svc.Bookmark.toolbarFolder, "Folder1", 0);
+    let folder1_guid = store.GUIDForId(folder1_id);
+
+    _("Create a bogus orphan record and write the record back to the store to trigger _reparentOrphans.");
+    Utils.anno(folder1_id, PARENT_ANNO, folder1_guid);
+    let record = store.createRecord(folder1_guid);
+    record.title = "New title for Folder 1";
+    store._childrenToOrder = {};
+    store.applyIncoming(record);
+
+    _("Verify that is has been marked as an orphan even though it couldn't be moved into itself.");
+    do_check_eq(Utils.anno(folder1_id, PARENT_ANNO), folder1_guid);
+
+  } finally {
+    _("Clean up.");
+    store.wipe();
+  }
+}
+
 function run_test() {
   test_bookmark_create();
+  test_bookmark_createRecord();
+  test_bookmark_update();
   test_folder_create();
   test_folder_createRecord();
+  test_deleted();
   test_move_folder();
   test_move_order();
+  test_orphan();
+  test_reparentOrphans();
 }