tracker now caches places id -> guid mappings, since otherwise it can't retrieve the correct guid on item removal (because the callback happens after the item is removed)

services/sync/modules/engines/bookmarks.js

@@ -952,26 +952,17 @@ BookmarksStore.prototype = {
     return null;
   },
 
-  _applyIncoming: function BStore__applyIncoming(record) {
-    let self = yield;
-
-    if (!this._lookup)
-      this.wrap();
-
-    this._log.trace("RECORD: " + record.id + " -> " + uneval(record.cleartext));
-
-    if (!record.cleartext)
-      this._removeCommand({GUID: record.id});
-    else if (this._getItemIdForGUID(record.id) < 0)
-      this._createCommand({GUID: record.id, data: record.cleartext});
-    else {
-      let data = Utils.deepCopy(record.cleartext);
-      delete data.GUID;
-      this._editCommand({GUID: record.id, data: data});
-    }
-  },
   applyIncoming: function BStore_applyIncoming(onComplete, record) {
-    this._applyIncoming.async(this, onComplete, record);
+    let fn = function(rec) {
+      let self = yield;
+      if (!record.cleartext)
+        this._removeCommand({GUID: record.id});
+      else if (this._getItemIdForGUID(record.id) < 0)
+        this._createCommand({GUID: record.id, data: record.cleartext});
+      else
+        this._editCommand({GUID: record.id, data: record.cleartext});
+    };
+    fn.async(this, onComplete, record);
   },
 
   _createCommand: function BStore__createCommand(command) {
@@ -1071,10 +1062,13 @@ BookmarksStore.prototype = {
     }
     if (newId) {
       this._log.trace("Setting GUID of new item " + newId + " to " + command.GUID);
-      this._bms.setItemGUID(newId, command.GUID);
+      let cur = this._bms.getItemGUID(newId);
-      let foo = this._bms.getItemGUID(command.GUID);
+      if (cur == command.GUID)
-      if (foo == newId)
+        this._log.warn("Item " + newId + " already has GUID " + command.GUID);
-        this._log.debug("OK!");
+      else {
+        this._bms.setItemGUID(newId, command.GUID);
+        Engines.get("bookmarks")._tracker._all[newId] = command.GUID; // HACK - see tracker
+      }
     }
   },
 
@@ -1088,10 +1082,8 @@ BookmarksStore.prototype = {
     }
 
     var itemId = this._bms.getItemIdForGUID(command.GUID);
-    this._log.debug("woo: " + itemId);
     if (itemId < 0) {
-      this._log.warn("Attempted to remove item " + command.GUID +
+      this._log.debug("Item " + command.GUID + "already removed");
-                     ", but it does not exist.  Skipping.");
       return;
     }
     var type = this._bms.getItemType(itemId);
@@ -1240,6 +1232,7 @@ BookmarksStore.prototype = {
 
     this._log.debug("Changing GUID " + oldID + " to " + newID);
     this._bms.setItemGUID(itemId, newID);
+    Engines.get("bookmarks")._tracker._all[itemId] = newID; // HACK - see tracker
   },
 
   _getNode: function BSS__getNode(folder) {
@@ -1467,47 +1460,73 @@ BookmarksTracker.prototype = {
 
   _init: function BMT__init() {
     this.__proto__.__proto__._init.call(this);
+
+    // NOTE: since the callbacks give us item IDs (not GUIDs), we use
+    // getItemGUID to get it within the callback.  For removals, however,
+    // that doesn't work because the item is already gone! (and worse, Places
+    // has a bug where it will generate a new one instead of throwing).
+    // Our solution: cache item IDs -> GUIDs
+
+    // FIXME: very roundabout way of getting id -> guid mapping!
+    let store = new BookmarksStore();
+    let all = store.wrap();
+    this._all = {};
+    for (let guid in all) {
+      this._all[this._bms.getItemIdForGUID(guid)] = guid;
+    }
+
     this._bms.addObserver(this, false);
   },
 
-  // FIXME: not getting any events whatsoever!
-
   /* Every add/remove/change is worth 10 points */
-
+  _upScore: function BMT__upScore() {
-  onItemAdded: function BMT_onEndUpdateBatch(itemId, folder, index) {
     if (!this.enabled)
       return;
-    let guid = this._bms.getItemGUID(itemId);
-    this._log.debug("Item " + guid + " added, adding to queue");
-    this.addChangedID(guid);
     this._score += 10;
   },
 
+  onItemAdded: function BMT_onEndUpdateBatch(itemId, folder, index) {
+    this._log.trace("onItemAdded: " + itemId);
+
+    this._all[itemId] = this._bms.getItemGUID(itemId);
+    this.addChangedID(this._all[itemId]);
+
+    this._upScore();
+  },
+
   onItemRemoved: function BMT_onItemRemoved(itemId, folder, index) {
-    if (!this.enabled)
+    this._log.trace("onItemRemoved: " + itemId);
-      return;
+
-    let guid = this._bms.getItemGUID(itemId);
+    this.addChangedID(this._all[itemId]);
-    this._log.debug("Item " + guid + " removed, adding to queue");
+    delete this._all[itemId];
-    this.addChangedID(guid);
+
-    this._score += 10;
+    this._upScore();
   },
 
   onItemChanged: function BMT_onItemChanged(itemId, property, isAnnotationProperty, value) {
-    if (!this.enabled)
+    this._log.trace("onItemChanged: " + itemId + ", property: " + property +
-      return;
+                    ", isAnno: " + isAnnotationProperty + ", value: " + value);
+
+    // NOTE: we get onItemChanged too much, when adding an item, changing its
+    //       GUID, before removal... it makes removals break, because trying
+    //       to update our cache before removals makes it so we think the
+    //       temporary guid was removed, instead of the real one.
+    //       Therefore, we ignore all guid changes.  When *Weave* changes the
+    //       GUID, we take care to update the tracker's cache.  If anyone else
+    //       changes the GUID, that will case breakage.
     let guid = this._bms.getItemGUID(itemId);
-    this._log.debug("Item " + guid + " changed, adding to queue");
+    if (guid != this._all[itemId])
-    this.addChangedID(guid);
+      this._log.trace("GUID change, ignoring");
-    this._score += 10;
+    else
+      this.addChangedID(this._all[itemId]); // some other change
+
+    this._upScore();
   },
 
   onItemMoved: function BMT_onItemMoved(itemId, oldParent, oldIndex, newParent, newIndex) {
-    if (!this.enabled)
+    this._log.trace("onItemMoved: " + itemId);
-      return;
+    this.addChangedID(this._all[itemId]);
-    let guid = this._bms.getItemGUID(itemId);
+    this._upScore();
-    this._log.debug("Item " + guid + " moved, adding to queue");
-    this.addChangedID(guid);
-    this._score += 10;
   },
 
   onBeginUpdateBatch: function BMT_onBeginUpdateBatch() {},