Bug 1149023 - fix errors deleting an already synced readinglist item. r=adw

browser/components/readinglist/ReadingList.jsm

@@ -233,15 +233,17 @@ ReadingListImpl.prototype = {
    *         an Error on error.
    */
   forEachItem: Task.async(function* (callback, ...optsList) {
-    yield this._forEachItem(callback, optsList, STORE_OPTIONS_IGNORE_DELETED);
+    let thisCallback = record => callback(this._itemFromRecord(record));
+    yield this._forEachRecord(thisCallback, optsList, STORE_OPTIONS_IGNORE_DELETED);
   }),
 
   /**
-   * Like forEachItem, but enumerates only previously synced items that are
-   * marked as being locally deleted.
+   * Enumerates the GUIDs for previously synced items that are marked as being
+   * locally deleted.
    */
-  forEachSyncedDeletedItem: Task.async(function* (callback, ...optsList) {
-    yield this._forEachItem(callback, optsList, {
+  forEachSyncedDeletedGUID: Task.async(function* (callback, ...optsList) {
+    let thisCallback = record => callback(record.guid);
+    yield this._forEachRecord(thisCallback, optsList, {
       syncStatus: SYNC_STATUS_DELETED,
     });
   }),
@@ -252,12 +254,12 @@ ReadingListImpl.prototype = {
    * @param storeOptions An options object passed to the store as the "control"
    *        options.
    */
-  _forEachItem: Task.async(function* (callback, optsList, storeOptions) {
+  _forEachRecord: Task.async(function* (callback, optsList, storeOptions) {
     let promiseChain = Promise.resolve();
     yield this._store.forEachItem(record => {
       promiseChain = promiseChain.then(() => {
         return new Promise((resolve, reject) => {
-          let promise = callback(this._itemFromRecord(record));
+          let promise = callback(record);
           if (promise instanceof Promise) {
             return promise.then(resolve, reject);
           }
@@ -317,7 +319,9 @@ ReadingListImpl.prototype = {
       record.syncStatus = SYNC_STATUS_NEW;
     }
 
+    log.debug("addingItem with guid: ${guid}, url: ${url}", record);
     yield this._store.addItem(record);
+    log.trace("added item with guid: ${guid}, url: ${url}", record);
     this._invalidateIterators();
     let item = this._itemFromRecord(record);
     this._callListeners("onItemAdded", item);
@@ -345,7 +349,9 @@ ReadingListImpl.prototype = {
       throw new Error("The item must have a url");
     }
     this._ensureItemBelongsToList(item);
+    log.debug("updatingItem with guid: ${guid}, url: ${url}", item._record);
     yield this._store.updateItem(item._record);
+    log.trace("finished update of item guid: ${guid}, url: ${url}", item._record);
     this._invalidateIterators();
     this._callListeners("onItemUpdated", item);
   }),
@@ -367,6 +373,7 @@ ReadingListImpl.prototype = {
     // the store.  Otherwise mark it as deleted but don't actually delete it so
     // that its status can be synced.
     if (item._record.syncStatus == SYNC_STATUS_NEW) {
+      log.debug("deleteItem guid: ${guid}, url: ${url} - item is local so really deleting it", item._record);
       yield this._store.deleteItemByURL(item.url);
     }
     else {
@@ -378,12 +385,16 @@ ReadingListImpl.prototype = {
       }
       newRecord.guid = item._record.guid;
       newRecord.syncStatus = SYNC_STATUS_DELETED;
-      item._record = newRecord;
-      yield this._store.updateItemByGUID(item._record);
+      log.debug("deleteItem guid: ${guid}, url: ${url} - item has been synced so updating to deleted state", item._record);
+      yield this._store.updateItemByGUID(newRecord);
     }
 
+    log.trace("finished db operation deleting item with guid: ${guid}, url: ${url}", item._record);
     item.list = null;
-    this._itemsByNormalizedURL.delete(item.url);
+    // failing to remove the item from the map points at something bad!
+    if (!this._itemsByNormalizedURL.delete(item.url)) {
+      log.error("Failed to remove item from the map", item);
+    }
     this._invalidateIterators();
     let mm = Cc["@mozilla.org/globalmessagemanager;1"].getService(Ci.nsIMessageListenerManager);
     mm.broadcastAsyncMessage("Reader:Removed", item);
@@ -514,6 +525,9 @@ ReadingListImpl.prototype = {
    * @return The ReadingListItem.
    */
   _itemFromRecord(record) {
+    if (!record.url) {
+      throw new Error("record must have a URL");
+    }
     let itemWeakRef = this._itemsByNormalizedURL.get(record.url);
     let item = itemWeakRef ? itemWeakRef.get() : null;
     if (item) {

browser/components/readinglist/Sync.jsm

@@ -296,9 +296,9 @@ SyncImpl.prototype = {
 
     // Get deleted synced local items.
     let requests = [];
-    yield this.list.forEachSyncedDeletedItem(localItem => {
+    yield this.list.forEachSyncedDeletedGUID(guid => {
       requests.push({
-        path: "/articles/" + localItem.guid,
+        path: "/articles/" + guid,
       });
     });
     if (!requests.length) {

browser/components/readinglist/test/xpcshell/test_ReadingList.js

@@ -9,6 +9,10 @@ Cu.import("resource:///modules/readinglist/ReadingList.jsm");
 Cu.import("resource:///modules/readinglist/SQLiteStore.jsm");
 Cu.import("resource://gre/modules/Sqlite.jsm");
 Cu.import("resource://gre/modules/Timer.jsm");
+Cu.import("resource://gre/modules/Log.jsm");
+
+Log.repository.getLogger("readinglist.api").level = Log.Level.All;
+Log.repository.getLogger("readinglist.api").addAppender(new Log.DumpAppender());
 
 var gList;
 var gItems;
@@ -294,10 +298,10 @@ add_task(function* forEachSyncedDeletedItem() {
   });
   deletedItem._record.syncStatus = gList.SyncStatus.SYNCED;
   yield gList.deleteItem(deletedItem);
-  let items = [];
-  yield gList.forEachSyncedDeletedItem(item => items.push(item));
-  Assert.equal(items.length, 1);
-  Assert.equal(items[0].guid, deletedItem.guid);
+  let guids = [];
+  yield gList.forEachSyncedDeletedGUID(guid => guids.push(guid));
+  Assert.equal(guids.length, 1);
+  Assert.equal(guids[0], deletedItem.guid);
 });
 
 add_task(function* forEachItem_promises() {
@@ -654,7 +658,7 @@ add_task(function* listeners() {
   Assert.equal((yield gList.count()), gItems.length);
 });
 
-// This test deletes items so it should probably run last.
+// This test deletes items so it should probably run last of the 'gItems' tests...
 add_task(function* deleteItem() {
   // delete first item with item.delete()
   let iter = gList.iterator({
@@ -692,6 +696,28 @@ add_task(function* deleteItem() {
   checkItems(items, gItems.slice(3));
 });
 
+// Check that when we delete an item with a GUID it's no longer available as
+// an item
+add_task(function* deletedItemRemovedFromMap() {
+  yield gList.forEachItem(item => item.delete());
+  Assert.equal((yield gList.count()), 0);
+  let map = gList._itemsByNormalizedURL;
+  Assert.equal(gList._itemsByNormalizedURL.size, 0, [for (i of map.keys()) i]);
+  let record = {
+    guid: "test-item",
+    url: "http://localhost",
+    syncStatus: gList.SyncStatus.SYNCED,
+  }
+  let item = yield gList.addItem(record);
+  Assert.equal(map.size, 1);
+  yield item.delete();
+  Assert.equal(gList._itemsByNormalizedURL.size, 0, [for (i of map.keys()) i]);
+
+  // Now enumerate deleted items - should not come back.
+  yield gList.forEachSyncedDeletedGUID(() => {});
+  Assert.equal(gList._itemsByNormalizedURL.size, 0, [for (i of map.keys()) i]);
+});
+
 function checkItems(actualItems, expectedItems) {
   Assert.equal(actualItems.length, expectedItems.length);
   for (let i = 0; i < expectedItems.length; i++) {