Bug 1149023 - fix errors deleting an already synced readinglist item. r=adw, a=readinglist
authorMark Hammond <mhammond@skippinet.com.au>
Wed, 01 Apr 2015 16:40:25 +1100
changeset 258179 4e05802f6eb4
parent 258178 6604edc0f5da
child 258180 cb279e8b39ec
child 258182 7dded32396ba
push id4615
push usermhammond@skippinet.com.au
push date2015-04-01 05:41 +0000
treeherdermozilla-beta@4e05802f6eb4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw, readinglist
bugs1149023
milestone38.0
Bug 1149023 - fix errors deleting an already synced readinglist item. r=adw, a=readinglist
browser/components/readinglist/ReadingList.jsm
browser/components/readinglist/Sync.jsm
browser/components/readinglist/test/xpcshell/test_ReadingList.js
--- a/browser/components/readinglist/ReadingList.jsm
+++ b/browser/components/readinglist/ReadingList.jsm
@@ -239,41 +239,43 @@ ReadingListImpl.prototype = {
    *        is resolved.
    * @param optsList A variable number of options objects that control the
    *        items that are matched.  See Options Objects.
    * @return Promise<null> Resolved when the enumeration completes *and* the
    *         last promise returned by the callback is resolved.  Rejected with
    *         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,
     });
   }),
 
   /**
    * See forEachItem.
    *
    * @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);
           }
           resolve();
           return undefined;
         });
       });
     }, optsList, storeOptions);
@@ -323,17 +325,19 @@ ReadingListImpl.prototype = {
       } catch (ex) {
         record.addedBy = SyncUtils.getDefaultDeviceName();
       }
     }
     if (!("syncStatus" in record)) {
       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);
     let mm = Cc["@mozilla.org/globalmessagemanager;1"].getService(Ci.nsIMessageListenerManager);
     mm.broadcastAsyncMessage("Reader:Added", item);
     return item;
   }),
 
@@ -351,17 +355,19 @@ ReadingListImpl.prototype = {
    * @return Promise<null> Resolved when the list is updated.  Rejected with an
    *         Error on error.
    */
   updateItem: Task.async(function* (item) {
     if (!item._record.url) {
       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);
   }),
 
   /**
    * Deletes an item from the list.  The item must have a `url`.
    *
    * It's an error to call this for an item that doesn't belong to the list.
@@ -373,33 +379,38 @@ ReadingListImpl.prototype = {
    */
   deleteItem: Task.async(function* (item) {
     this._ensureItemBelongsToList(item);
 
     // If the item is new and therefore hasn't been synced yet, delete it from
     // 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 {
       // To prevent data leakage, only keep the record fields needed to sync
       // the deleted status: guid and syncStatus.
       let newRecord = {};
       for (let prop of ITEM_RECORD_PROPERTIES) {
         newRecord[prop] = null;
       }
       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);
     this._callListeners("onItemDeleted", item);
   }),
 
   /**
    * Finds the first item that matches the given options.
@@ -501,16 +512,19 @@ ReadingListImpl.prototype = {
   /**
    * Returns the ReadingListItem represented by the given record object.  If
    * the item doesn't exist yet, it's created first.
    *
    * @param record A simple object with *normalized* item record properties.
    * @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) {
       item._record = record;
     }
     else {
       item = new ReadingListItem(record);
       item.list = this;
--- a/browser/components/readinglist/Sync.jsm
+++ b/browser/components/readinglist/Sync.jsm
@@ -291,19 +291,19 @@ SyncImpl.prototype = {
    *
    * Uploads deleted synced items.
    */
   _uploadDeletedItems: Task.async(function* () {
     log.debug("Phase 1 part 3: Uploading deleted items");
 
     // 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) {
       log.debug("No local deleted synced items to upload");
       return;
     }
 
     // Send the request.
--- a/browser/components/readinglist/test/xpcshell/test_ReadingList.js
+++ b/browser/components/readinglist/test/xpcshell/test_ReadingList.js
@@ -4,16 +4,20 @@
 "use strict";
 
 let gDBFile = do_get_profile();
 
 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;
 
 function run_test() {
   run_next_test();
 }
 
@@ -289,20 +293,20 @@ add_task(function* forEachItem() {
 
 add_task(function* forEachSyncedDeletedItem() {
   let deletedItem = yield gList.addItem({
     guid: "forEachSyncedDeletedItem",
     url: "http://example.com/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() {
   // promises resolved immediately
   let items = [];
   yield gList.forEachItem(item => {
     items.push(item);
     return Promise.resolve();
@@ -649,17 +653,17 @@ add_task(function* listeners() {
   items[0].delete();
   listenerItem = yield listenerPromise;
   Assert.ok(listenerItem);
   Assert.ok(listenerItem === items[0]);
   gList.removeListener(listener);
   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({
     sort: "guid",
   });
   let item = (yield iter.items(1))[0];
   Assert.ok(item);
   item.delete();
@@ -687,16 +691,38 @@ add_task(function* deleteItem() {
   Assert.equal((yield gList.count()), gItems.length - 3);
   items = [];
   yield gList.forEachItem(i => items.push(i), {
     sort: "guid",
   });
   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++) {
     for (let prop in expectedItems[i]._record) {
       Assert.ok(prop in actualItems[i]._record, prop);
       Assert.equal(actualItems[i]._record[prop], expectedItems[i][prop]);
     }
   }