Bug 627497: remove annotations from copied bookmarks. r=philiKON

services/sync/modules/engines/bookmarks.js

@@ -1139,12 +1139,17 @@ BookmarksStore.prototype = {
         "FROM moz_bookmarks " +
         "WHERE guid = :guid");
     } else {
+      // Order results by lastModified so we can preserve the ID of the oldest bookmark.
+      // Copying a record preserves its dateAdded, and only modifying the
+      // bookmark alters its lastModified, so we also order by its item_id --
+      // lowest wins ties. Of course, Places can still screw us by reassigning IDs...
       stmt = this._getStmt(
         "SELECT a.item_id AS item_id " +
         "FROM moz_items_annos a " +
         "JOIN moz_anno_attributes n ON n.id = a.anno_attribute_id " +
         "WHERE n.name = '" + GUID_ANNO + "' " +
-        "AND a.content = :guid");
+        "AND a.content = :guid " +
+        "ORDER BY a.lastModified, a.item_id");
     }
 
     return this.__idForGUIDStm = stmt;
@@ -1160,10 +1165,32 @@ BookmarksStore.prototype = {
     // guid might be a String object rather than a string.
     stmt.params.guid = guid.toString();
 
-    let result = Utils.queryAsync(stmt, ["item_id"])[0];
-    if (result)
-      return result.item_id;
-    return -1;
+    let results = Utils.queryAsync(stmt, ["item_id"]);
+    this._log.trace("Rows matching GUID " + guid + ": " + results.length);
+    
+    // Here's the one we care about: the first.
+    let result = results[0];
+    
+    if (!result)
+      return -1;
+    
+    if (!this._haveGUIDColumn) {
+      try {
+        // Assign new GUIDs to any that came later.
+        for (let i = 1; i < results.length; ++i) {
+          let surplus = results[i];
+          this._log.debug("Assigning new GUID to copied row " + surplus.item_id);
+          this._setGUID(surplus.item_id);
+        }
+      } catch (ex) {
+        // Just skip it and carry on. This shouldn't happen, but if it does we
+        // don't want to fail hard.
+        this._log.debug("Got exception assigning new GUIDs: " +
+                        Utils.exceptionStr(ex));
+      }
+    }
+    
+    return result.item_id;
   },
 
   _calculateIndex: function _calculateIndex(record) {

services/sync/tests/unit/test_bookmark_store.js

@@ -349,7 +349,65 @@ function test_reparentOrphans() {
   }
 }
 
+// Copying a bookmark in the UI also copies its annotations, including the GUID
+// annotation in 3.x.
+function test_copying_avoid_duplicate_guids() {
+  if (store._haveGUIDColumn) {
+    _("No GUID annotation handling functionality; returning without testing.");
+    return;
+  }
+    
+  try {
+    _("Ensure the record isn't present yet.");
+    let ids = Svc.Bookmark.getBookmarkIdsForURI(fxuri, {});
+    do_check_eq(ids.length, 0);
+
+    _("Let's create a new record.");
+    let fxrecord = new Bookmark("bookmarks", "get-firefox1");
+    fxrecord.bmkUri        = fxuri.spec;
+    fxrecord.description   = "Firefox is awesome.";
+    fxrecord.title         = "Get Firefox!";
+    fxrecord.tags          = ["firefox", "awesome", "browser"];
+    fxrecord.keyword       = "awesome";
+    fxrecord.loadInSidebar = false;
+    fxrecord.parentName    = "Bookmarks Toolbar";
+    fxrecord.parentid      = "toolbar";
+    store.applyIncoming(fxrecord);
+
+    _("Verify it has been created correctly.");
+    let id = store.idForGUID(fxrecord.id);
+    do_check_eq(store.GUIDForId(id), fxrecord.id);
+    do_check_true(Svc.Bookmark.getBookmarkURI(id).equals(fxuri));
+    do_check_eq(Utils.anno(id, "bookmarkProperties/description"),
+                fxrecord.description);
+    
+    _("Copy the record as happens in the UI: with the same GUID.");
+    let parentID = Svc.Bookmark.getFolderIdForItem(id);
+    let copy = Svc.Bookmark.insertBookmark(parentID, fxuri, -1, fxrecord.title);
+    Svc.Bookmark.setItemLastModified(copy, Svc.Bookmark.getItemLastModified(id));
+    Svc.Bookmark.setItemDateAdded(copy, Svc.Bookmark.getItemDateAdded(id));
+    store._setGUID(copy, fxrecord.id);
+    
+    do_check_eq(store.GUIDForId(copy), store.GUIDForId(id));
+    
+    _("Calling idForGUID fixes things.");
+    _("GUID before: " + store.GUIDForId(copy));
+    do_check_eq(store.idForGUID(fxrecord.id), id);           // Oldest wins.
+    _("GUID now: " + store.GUIDForId(copy));
+    do_check_neq(store.GUIDForId(copy), store.GUIDForId(id));
+    
+    _("Verify that the anno itself has changed.");
+    do_check_neq(Utils.anno(copy, "sync/guid"), fxrecord.id);
+    do_check_eq(Utils.anno(copy, "sync/guid"), store.GUIDForId(copy));
+    
+  } finally {
+    _("Clean up.");
+    store.wipe();
+  }
+}
+
 function run_test() {
+  initTestLogging('Trace');
   test_bookmark_create();
   test_bookmark_createRecord();
   test_bookmark_update();
@@ -360,4 +418,5 @@ function run_test() {
   test_move_order();
   test_orphan();
   test_reparentOrphans();
+  test_copying_avoid_duplicate_guids();
 }