Bug 1149105 - Fix various desktop reading list sync failures, add more logging. r=markh, a=readinglist
authorDrew Willcoxon <adw@mozilla.com>
Tue, 31 Mar 2015 16:41:13 +1100
changeset 258185 818e63fbfba2
parent 258184 9104d1f928a7
child 258187 83bcf11d00ef
push id4617
push userdwillcoxon@mozilla.com
push date2015-04-02 02:05 +0000
treeherdermozilla-beta@818e63fbfba2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh, readinglist
bugs1149105
milestone38.0
Bug 1149105 - Fix various desktop reading list sync failures, add more logging. r=markh, a=readinglist
browser/components/readinglist/ReadingList.jsm
browser/components/readinglist/SQLiteStore.jsm
browser/components/readinglist/Sync.jsm
browser/components/readinglist/test/xpcshell/test_ReadingList.js
browser/components/readinglist/test/xpcshell/test_SQLiteStore.js
--- a/browser/components/readinglist/ReadingList.jsm
+++ b/browser/components/readinglist/ReadingList.jsm
@@ -21,26 +21,16 @@ XPCOMUtils.defineLazyModuleGetter(this, 
 
 // We use Sync's "Utils" module for the device name, which is unfortunate,
 // but let's give it a better name here.
 XPCOMUtils.defineLazyGetter(this, "SyncUtils", function() {
   const {Utils} = Cu.import("resource://services-sync/util.js", {});
   return Utils;
 });
 
-{ // Prevent the parent log setup from leaking into the global scope.
-  let parentLog = Log.repository.getLogger("readinglist");
-  parentLog.level = Preferences.get("browser.readinglist.logLevel", Log.Level.Warn);
-  Preferences.observe("browser.readinglist.logLevel", value => {
-    parentLog.level = value;
-  });
-  let formatter = new Log.BasicFormatter();
-  parentLog.addAppender(new Log.ConsoleAppender(formatter));
-  parentLog.addAppender(new Log.DumpAppender(formatter));
-}
 let log = Log.repository.getLogger("readinglist.api");
 
 
 // Each ReadingListItem has a _record property, an object containing the raw
 // data from the server and local store.  These are the names of the properties
 // in that object.
 //
 // Not important, but FYI: The order that these are listed in follows the order
@@ -95,16 +85,40 @@ const STORE_OPTIONS_IGNORE_DELETED = {
 const SYNC_STATUS_PROPERTIES_STATUS = `
   favorite
   markedReadBy
   markedReadOn
   readPosition
   unread
 `.trim().split(/\s+/);
 
+function ReadingListError(message) {
+  this.message = message;
+  this.name = this.constructor.name;
+  this.stack = (new Error()).stack;
+
+  // Consumers can set this to an Error that this ReadingListError wraps.
+  this.originalError = null;
+}
+ReadingListError.prototype = new Error();
+ReadingListError.prototype.constructor = ReadingListError;
+
+function ReadingListExistsError(message) {
+  message = message || "The item already exists";
+  ReadingListError.call(this, message);
+}
+ReadingListExistsError.prototype = new ReadingListError();
+ReadingListExistsError.prototype.constructor = ReadingListExistsError;
+
+function ReadingListDeletedError(message) {
+  message = message || "The item has been deleted";
+  ReadingListError.call(this, message);
+}
+ReadingListDeletedError.prototype = new ReadingListError();
+ReadingListDeletedError.prototype.constructor = ReadingListDeletedError;
 
 /**
  * A reading list contains ReadingListItems.
  *
  * A list maintains only one copy of an item per URL.  So if for example you use
  * an iterator to get two references to items with the same URL, your references
  * actually refer to the same JS object.
  *
@@ -156,16 +170,22 @@ function ReadingListImpl(store) {
   this._store = store;
   this._itemsByNormalizedURL = new Map();
   this._iterators = new Set();
   this._listeners = new Set();
 }
 
 ReadingListImpl.prototype = {
 
+  Error: {
+    Error: ReadingListError,
+    Exists: ReadingListExistsError,
+    Deleted: ReadingListDeletedError,
+  },
+
   ItemRecordProperties: ITEM_RECORD_PROPERTIES,
 
   SyncStatus: {
     SYNCED: SYNC_STATUS_SYNCED,
     NEW: SYNC_STATUS_NEW,
     CHANGED_STATUS: SYNC_STATUS_CHANGED_STATUS,
     CHANGED_MATERIAL: SYNC_STATUS_CHANGED_MATERIAL,
     DELETED: SYNC_STATUS_DELETED,
@@ -298,35 +318,35 @@ ReadingListImpl.prototype = {
    *
    * @param record A simple object representing an item.
    * @return Promise<ReadingListItem> Resolved with the new item when the list
    *         is updated.  Rejected with an Error on error.
    */
   addItem: Task.async(function* (record) {
     record = normalizeRecord(record);
     if (!record.url) {
-      throw new Error("The item must have a url");
+      throw new ReadingListError("The item to be added must have a url");
     }
     if (!("addedOn" in record)) {
       record.addedOn = Date.now();
     }
     if (!("addedBy" in record)) {
       try {
         record.addedBy = Services.prefs.getCharPref("services.sync.client.name");
       } catch (ex) {
         record.addedBy = SyncUtils.getDefaultDeviceName();
       }
     }
     if (!("syncStatus" in record)) {
       record.syncStatus = SYNC_STATUS_NEW;
     }
 
-    log.debug("addingItem with guid: ${guid}, url: ${url}", record);
+    log.debug("Adding item with guid: ${guid}, url: ${url}", record);
     yield this._store.addItem(record);
-    log.trace("added item with guid: ${guid}, url: ${url}", 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;
   }),
 
@@ -340,62 +360,72 @@ ReadingListImpl.prototype = {
    * It's an error to call this for an item that doesn't belong to the list.
    * The returned promise is rejected in that case.
    *
    * @param item The ReadingListItem to update.
    * @return Promise<null> Resolved when the list is updated.  Rejected with an
    *         Error on error.
    */
   updateItem: Task.async(function* (item) {
+    if (item._deleted) {
+      throw new ReadingListDeletedError("The item to be updated has been deleted");
+    }
     if (!item._record.url) {
-      throw new Error("The item must have a url");
+      throw new ReadingListError("The item to be updated must have a url");
     }
     this._ensureItemBelongsToList(item);
-    log.debug("updatingItem with guid: ${guid}, url: ${url}", item._record);
+    log.debug("Updating item 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);
+    log.trace("Finished updating item with 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.
    * The returned promise is rejected in that case.
    *
    * @param item The ReadingListItem to delete.
    * @return Promise<null> Resolved when the list is updated.  Rejected with an
    *         Error on error.
    */
   deleteItem: Task.async(function* (item) {
+    if (item._deleted) {
+      throw new ReadingListDeletedError("The item has already been deleted");
+    }
     this._ensureItemBelongsToList(item);
 
+    log.debug("Deleting item with guid: ${guid}, url: ${url}");
+
     // 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);
+      log.debug("Item is new, truly deleting it", item._record);
       yield this._store.deleteItemByURL(item.url);
     }
     else {
+      log.debug("Item has been synced, only marking it as deleted",
+                item._record);
       // 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;
-      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);
+    log.trace("Finished deleting item with guid: ${guid}, url: ${url}", item._record);
     item.list = null;
+    item._deleted = true;
     // 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);
@@ -552,17 +582,17 @@ ReadingListImpl.prototype = {
           Cu.reportError(err);
         }
       }
     }
   },
 
   _ensureItemBelongsToList(item) {
     if (!item || !item._ensureBelongsToList) {
-      throw new Error("The item is not a ReadingListItem");
+      throw new ReadingListError("The item is not a ReadingListItem");
     }
     item._ensureBelongsToList();
   },
 };
 
 
 let _unserializable = () => {}; // See comments in the ReadingListItem ctor.
 
@@ -572,16 +602,17 @@ let _unserializable = () => {}; // See c
  * Each item belongs to a list, and it's an error to use an item with a
  * ReadingList that the item doesn't belong to.
  *
  * @param record A simple object with the properties of the item, as few or many
  *        as you want.  This will be normalized.
  */
 function ReadingListItem(record={}) {
   this._record = record;
+  this._deleted = false;
 
   // |this._unserializable| works around a problem when sending one of these
   // items via a message manager. If |this.list| is set, the item can't be
   // transferred directly, so .toJSON is implicitly called and the object
   // returned via that is sent. However, once the item is deleted and |this.list|
   // is null, the item *can* be directly serialized - so the message handler
   // sees the "raw" object - ie, it sees "_record" etc.
   // We work around this problem by *always* having an unserializable property
@@ -820,19 +851,21 @@ ReadingListItem.prototype = {
    },
 
   /**
    * Deletes the item from its list.
    *
    * @return Promise<null> Resolved when the list has been updated.
    */
   delete: Task.async(function* () {
+    if (this._deleted) {
+      throw new ReadingListDeletedError("The item has already been deleted");
+    }
     this._ensureBelongsToList();
     yield this.list.deleteItem(this);
-    this.delete = () => Promise.reject("The item has already been deleted");
   }),
 
   toJSON() {
     return this._record;
   },
 
   /**
    * Do not use this at all unless you know what you're doing.  Use the public
@@ -879,17 +912,17 @@ ReadingListItem.prototype = {
     for (let prop in partialRecord) {
       record[prop] = partialRecord[prop];
     }
     this._record = record;
   },
 
   _ensureBelongsToList() {
     if (!this.list) {
-      throw new Error("The item must belong to a reading list");
+      throw new ReadingListError("The item must belong to a list");
     }
   },
 };
 
 /**
  * An object that enumerates over items in a list.
  *
  * You can enumerate items a chunk at a time by passing counts to forEach() and
@@ -972,17 +1005,17 @@ ReadingListItemIterator.prototype = {
    * you're a ReadingList.
    */
   invalidate() {
     this.invalid = true;
   },
 
   _ensureValid() {
     if (this.invalid) {
-      throw new Error("The iterator has been invalidated");
+      throw new ReadingListError("The iterator has been invalidated");
     }
   },
 };
 
 
 /**
  * Normalizes the properties of a record object, which represents a
  * ReadingListItem.  Throws an error if the record contains properties that
@@ -990,17 +1023,17 @@ ReadingListItemIterator.prototype = {
  *
  * @param record A non-normalized record object.
  * @return The new normalized record.
  */
 function normalizeRecord(nonNormalizedRecord) {
   let record = {};
   for (let prop in nonNormalizedRecord) {
     if (ITEM_RECORD_PROPERTIES.indexOf(prop) < 0) {
-      throw new Error("Unrecognized item property: " + prop);
+      throw new ReadingListError("Unrecognized item property: " + prop);
     }
     switch (prop) {
     case "url":
     case "resolvedURL":
       if (nonNormalizedRecord[prop]) {
         record[prop] = normalizeURI(nonNormalizedRecord[prop]);
       }
       else {
--- a/browser/components/readinglist/SQLiteStore.jsm
+++ b/browser/components/readinglist/SQLiteStore.jsm
@@ -85,19 +85,24 @@ this.SQLiteStore.prototype = {
   addItem: Task.async(function* (item) {
     let colNames = [];
     let paramNames = [];
     for (let propName in item) {
       colNames.push(propName);
       paramNames.push(`:${propName}`);
     }
     let conn = yield this._connectionPromise;
-    yield conn.executeCached(`
-      INSERT INTO items (${colNames}) VALUES (${paramNames});
-    `, item);
+    try {
+      yield conn.executeCached(`
+        INSERT INTO items (${colNames}) VALUES (${paramNames});
+      `, item);
+    }
+    catch (err) {
+      throwExistsError(err);
+    }
   }),
 
   /**
    * Updates the properties of an item that's already present in the store.  See
    * ReadingList.prototype.updateItem.
    *
    * @param item The item to update.  It must have a `url`.
    * @return Promise<null> Resolved when the store is updated.  Rejected with an
@@ -202,21 +207,26 @@ this.SQLiteStore.prototype = {
    */
   _updateItem: Task.async(function* (item, keyProp) {
     let assignments = [];
     for (let propName in item) {
       assignments.push(`${propName} = :${propName}`);
     }
     let conn = yield this._connectionPromise;
     if (!item[keyProp]) {
-      throw new Error("Item must have " + keyProp);
+      throw new ReadingList.Error.Error("Item must have " + keyProp);
     }
-    yield conn.executeCached(`
-      UPDATE items SET ${assignments} WHERE ${keyProp} = :${keyProp};
-    `, item);
+    try {
+      yield conn.executeCached(`
+        UPDATE items SET ${assignments} WHERE ${keyProp} = :${keyProp};
+      `, item);
+    }
+    catch (err) {
+      throwExistsError(err);
+    }
   }),
 
   // The current schema version.
   _schemaVersion: 1,
 
   _checkSchema: Task.async(function* (conn) {
     let version = parseInt(yield conn.getSchemaVersion());
     for (; version < this._schemaVersion; version++) {
@@ -284,16 +294,36 @@ function itemFromRow(row) {
   let item = {};
   for (let name of ReadingList.ItemRecordProperties) {
     item[name] = row.getResultByName(name);
   }
   return item;
 }
 
 /**
+ * If the given Error indicates that a unique constraint failed, then wraps that
+ * error in a ReadingList.Error.Exists and throws it.  Otherwise throws the
+ * given error.
+ *
+ * @param err An Error object.
+ */
+function throwExistsError(err) {
+  let match =
+    /UNIQUE constraint failed: items\.([a-zA-Z0-9_]+)/.exec(err.message);
+  if (match) {
+    let newErr = new ReadingList.Error.Exists(
+      "An item with the following property already exists: " + match[1]
+    );
+    newErr.originalError = err;
+    err = newErr;
+  }
+  throw err;
+}
+
+/**
  * Returns the back part of a SELECT statement generated from the given list of
  * options.
  *
  * @param userOptsList A variable number of options objects that control the
  *        items that are matched.  See Options Objects in ReadingList.jsm.
  * @param controlOpts A single options object.  Use this to filter out items
  *        that don't match it -- in other words, to override the user options.
  *        See Options Objects in ReadingList.jsm.
--- a/browser/components/readinglist/Sync.jsm
+++ b/browser/components/readinglist/Sync.jsm
@@ -355,42 +355,49 @@ SyncImpl.prototype = {
     let response = yield this._sendRequest(request);
     if (response.status != 200) {
       this._handleUnexpectedResponse("downloading modified items", response);
       return;
     }
 
     // Update local items based on the response.
     for (let serverRecord of response.body.items) {
+      if (serverRecord.deleted) {
+        // _deleteItemForGUID is a no-op if no item exists with the GUID.
+        yield this._deleteItemForGUID(serverRecord.id);
+        continue;
+      }
       let localItem = yield this._itemForGUID(serverRecord.id);
       if (localItem) {
         if (localItem.serverLastModified == serverRecord.last_modified) {
           // We just uploaded this item in the new-items phase.
           continue;
         }
         // The local item may have materially changed.  In that case, don't
         // overwrite the local changes with the server record.  Instead, mark
         // the item as having material changes and reconcile and upload it in
         // the material-changes phase.
         // TODO
 
-        if (serverRecord.deleted) {
-          yield this._deleteItemForGUID(serverRecord.id);
-          continue;
-        }
         yield this._updateItemWithServerRecord(localItem, serverRecord);
         continue;
       }
-      // new item
+      // A potentially new item.  addItem() will fail here when an item was
+      // added to the local list between the time we uploaded new items and
+      // now.
       let localRecord = localRecordFromServerRecord(serverRecord);
       try {
         yield this.list.addItem(localRecord);
       } catch (ex) {
-        log.warn("Failed to add a new item from server record ${serverRecord}: ${ex}",
-                 {serverRecord, ex});
+        if (ex instanceof ReadingList.Error.Exists) {
+          log.debug("Tried to add an item that already exists.");
+        } else {
+          log.error("Error adding an item from server record ${serverRecord} ${ex}",
+                    { serverRecord, ex });
+        }
       }
     }
 
     // Now that changes have been successfully applied, advance the server
     // last-modified timestamp so that next time we fetch items starting from
     // the current point.  Response header names are lowercase.
     if (response.headers && "last-modified" in response.headers) {
       this._serverLastModifiedHeader = response.headers["last-modified"];
@@ -423,24 +430,34 @@ SyncImpl.prototype = {
    * local item's sync status is updated to reflect the fact that the item has
    * been synced and is up to date.
    *
    * @param item A local ReadingListItem.
    * @param serverRecord A server record representing the item.
    */
   _updateItemWithServerRecord: Task.async(function* (localItem, serverRecord) {
     if (!localItem) {
-      throw new Error("Item should exist");
+      // The item may have been deleted from the local list between the time we
+      // saw that it needed updating and now.
+      log.debug("Tried to update a null local item from server record",
+                serverRecord);
+      return;
     }
     localItem._record = localRecordFromServerRecord(serverRecord);
     try {
       yield this.list.updateItem(localItem);
     } catch (ex) {
-      log.warn("Failed to update an item from server record ${serverRecord}: ${ex}",
-               {serverRecord, ex});
+      // The item may have been deleted from the local list after we fetched it.
+      if (ex instanceof ReadingList.Error.Deleted) {
+        log.debug("Tried to update an item that was deleted from server record",
+                  serverRecord);
+      } else {
+        log.error("Error updating an item from server record ${serverRecord} ${ex}",
+                  { serverRecord, ex });
+      }
     }
   }),
 
   /**
    * Truly deletes the local ReadingListItem with the given GUID.
    *
    * @param guid The item's GUID.
    */
@@ -450,31 +467,31 @@ SyncImpl.prototype = {
       // If item is non-null, then it hasn't been deleted locally.  Therefore
       // it's important to delete it through its list so that the list and its
       // consumers are notified properly.  Set the syncStatus to NEW so that the
       // list truly deletes the item.
       item._record.syncStatus = ReadingList.SyncStatus.NEW;
       try {
         yield this.list.deleteItem(item);
       } catch (ex) {
-        log.warn("Failed delete local item with id ${guid}: ${ex}",
-                 {guid, ex});
+        log.error("Failed delete local item with id ${guid} ${ex}",
+                  { guid, ex });
       }
       return;
     }
     // If item is null, then it may not actually exist locally, or it may have
     // been synced and then deleted so that it's marked as being deleted.  In
     // that case, try to delete it directly from the store.  As far as the list
     // is concerned, the item has already been deleted.
     log.debug("Item not present in list, deleting it by GUID instead");
     try {
       this.list._store.deleteItemByGUID(guid);
     } catch (ex) {
-      log.warn("Failed to delete local item with id ${guid}: ${ex}",
-               {guid, ex});
+      log.error("Failed to delete local item with id ${guid} ${ex}",
+                { guid, ex });
     }
   }),
 
   /**
    * Sends a request to the server.
    *
    * @param req The request object: { method, path, body, headers }.
    * @return Promise<response> Resolved with the server's response object:
@@ -483,17 +500,17 @@ SyncImpl.prototype = {
   _sendRequest: Task.async(function* (req) {
     log.debug("Sending request", req);
     let response = yield this._client.request(req);
     log.debug("Received response", response);
     return response;
   }),
 
   _handleUnexpectedResponse(contextMsgFragment, response) {
-    log.warn(`Unexpected response ${contextMsgFragment}`, response);
+    log.error(`Unexpected response ${contextMsgFragment}`, response);
   },
 
   // TODO: Wipe this pref when user logs out.
   get _serverLastModifiedHeader() {
     if (!("__serverLastModifiedHeader" in this)) {
       this.__serverLastModifiedHeader =
         Preferences.get(SERVER_LAST_MODIFIED_HEADER_PREF, undefined);
     }
--- a/browser/components/readinglist/test/xpcshell/test_ReadingList.js
+++ b/browser/components/readinglist/test/xpcshell/test_ReadingList.js
@@ -87,98 +87,105 @@ add_task(function* constraints() {
   // add an item again
   let err = null;
   try {
     yield gList.addItem(gItems[0]);
   }
   catch (e) {
     err = e;
   }
-  checkError(err);
+  Assert.ok(err);
+  Assert.ok(err instanceof ReadingList.Error.Exists);
 
   // add a new item with an existing guid
   let item = kindOfClone(gItems[0]);
   item.guid = gItems[0].guid;
   err = null;
   try {
     yield gList.addItem(item);
   }
   catch (e) {
     err = e;
   }
-  checkError(err);
+  Assert.ok(err);
+  Assert.ok(err instanceof ReadingList.Error.Exists);
 
   // add a new item with an existing url
   item = kindOfClone(gItems[0]);
   item.url = gItems[0].url;
   err = null;
   try {
     yield gList.addItem(item);
   }
   catch (e) {
     err = e;
   }
-  checkError(err);
+  Assert.ok(err);
+  Assert.ok(err instanceof ReadingList.Error.Exists);
 
   // add a new item with an existing resolvedURL
   item = kindOfClone(gItems[0]);
   item.resolvedURL = gItems[0].resolvedURL;
   err = null;
   try {
     yield gList.addItem(item);
   }
   catch (e) {
     err = e;
   }
-  checkError(err);
+  Assert.ok(err);
+  Assert.ok(err instanceof ReadingList.Error.Exists);
 
   // add a new item with no url
   item = kindOfClone(gItems[0]);
   delete item.url;
   err = null;
   try {
     yield gList.addItem(item);
   }
   catch (e) {
     err = e;
   }
   Assert.ok(err);
-  Assert.ok(err instanceof Cu.getGlobalForObject(ReadingList).Error, err);
-  Assert.equal(err.message, "The item must have a url");
+  Assert.ok(err instanceof ReadingList.Error.Error);
+  Assert.ok(!(err instanceof ReadingList.Error.Exists));
+  Assert.ok(!(err instanceof ReadingList.Error.Deleted));
 
   // update an item with no url
   item = (yield gList.item({ guid: gItems[0].guid }));
   Assert.ok(item);
   let oldURL = item._record.url;
   item._record.url = null;
   err = null;
   try {
     yield gList.updateItem(item);
   }
   catch (e) {
     err = e;
   }
   item._record.url = oldURL;
   Assert.ok(err);
-  Assert.ok(err instanceof Cu.getGlobalForObject(ReadingList).Error, err);
-  Assert.equal(err.message, "The item must have a url");
+  Assert.ok(err instanceof ReadingList.Error.Error);
+  Assert.ok(!(err instanceof ReadingList.Error.Exists));
+  Assert.ok(!(err instanceof ReadingList.Error.Deleted));
 
   // add an item with a bogus property
   item = kindOfClone(gItems[0]);
   item.bogus = "gnarly";
   err = null;
   try {
     yield gList.addItem(item);
   }
   catch (e) {
     err = e;
   }
   Assert.ok(err);
-  Assert.ok(err.message);
-  Assert.ok(err.message.indexOf("Unrecognized item property:") >= 0);
+  Assert.ok(err instanceof ReadingList.Error.Error);
+  Assert.ok(!(err instanceof ReadingList.Error.Exists));
+  Assert.ok(!(err instanceof ReadingList.Error.Deleted));
 
   // add a new item with no guid, which is allowed
   item = kindOfClone(gItems[0]);
   delete item.guid;
   err = null;
   let rlitem1;
   try {
     rlitem1 = yield gList.addItem(item);
@@ -661,27 +668,47 @@ add_task(function* listeners() {
 // 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();
+  let {url, guid} = item;
+  Assert.ok((yield gList.itemForURL(url)), "should be able to get the item by URL before deletion");
+  Assert.ok((yield gList.item({guid})), "should be able to get the item by GUID before deletion");
+
+  yield item.delete();
+  try {
+    yield item.delete();
+    Assert.ok(false, "should not successfully delete the item a second time")
+  } catch(ex) {
+    Assert.ok(ex instanceof ReadingList.Error.Deleted);
+  }
+
+  Assert.ok(!(yield gList.itemForURL(url)), "should fail to get a deleted item by URL");
+  Assert.ok(!(yield gList.item({guid})), "should fail to get a deleted item by GUID");
+
   gItems[0].list = null;
   Assert.equal((yield gList.count()), gItems.length - 1);
   let items = [];
   yield gList.forEachItem(i => items.push(i), {
     sort: "guid",
   });
   checkItems(items, gItems.slice(1));
 
   // delete second item with list.deleteItem()
   yield gList.deleteItem(items[0]);
+  try {
+    yield gList.deleteItem(items[0]);
+    Assert.ok(false, "should not successfully delete the item a second time")
+  } catch(ex) {
+    Assert.ok(ex instanceof ReadingList.Error.Deleted);
+  }
   gItems[1].list = null;
   Assert.equal((yield gList.count()), gItems.length - 2);
   items = [];
   yield gList.forEachItem(i => items.push(i), {
     sort: "guid",
   });
   checkItems(items, gItems.slice(2));
 
@@ -723,21 +750,16 @@ function checkItems(actualItems, expecte
   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]);
     }
   }
 }
 
-function checkError(err) {
-  Assert.ok(err);
-  Assert.ok(err instanceof Cu.getGlobalForObject(Sqlite).Error, err);
-}
-
 function kindOfClone(item) {
   let newItem = {};
   for (let prop in item) {
     newItem[prop] = item[prop];
     if (typeof(newItem[prop]) == "string") {
       newItem[prop] += " -- make this string different";
     }
   }
--- a/browser/components/readinglist/test/xpcshell/test_SQLiteStore.js
+++ b/browser/components/readinglist/test/xpcshell/test_SQLiteStore.js
@@ -1,13 +1,14 @@
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 "use strict";
 
+Cu.import("resource:///modules/readinglist/ReadingList.jsm");
 Cu.import("resource:///modules/readinglist/SQLiteStore.jsm");
 Cu.import("resource://gre/modules/Sqlite.jsm");
 
 var gStore;
 var gItems;
 
 function run_test() {
   run_next_test();
@@ -53,17 +54,20 @@ add_task(function* constraints() {
   // add an item again
   let err = null;
   try {
     yield gStore.addItem(gItems[0]);
   }
   catch (e) {
     err = e;
   }
-  checkError(err, "UNIQUE constraint failed");
+  Assert.ok(err);
+  Assert.ok(err instanceof ReadingList.Error.Exists);
+  Assert.ok(err.message);
+  Assert.ok(err.message.indexOf("An item with the following property already exists:") >= 0);
 
   // add a new item with an existing guid
   function kindOfClone(item) {
     let newItem = {};
     for (let prop in item) {
       newItem[prop] = item[prop];
       if (typeof(newItem[prop]) == "string") {
         newItem[prop] += " -- make this string different";
@@ -75,66 +79,81 @@ add_task(function* constraints() {
   item.guid = gItems[0].guid;
   err = null;
   try {
     yield gStore.addItem(item);
   }
   catch (e) {
     err = e;
   }
-  checkError(err, "UNIQUE constraint failed: items.guid");
+  Assert.ok(err);
+  Assert.ok(err instanceof ReadingList.Error.Exists);
+  Assert.ok(err.message);
+  Assert.ok(err.message.indexOf("An item with the following property already exists: guid") >= 0);
 
   // add a new item with an existing url
   item = kindOfClone(gItems[0]);
   item.url = gItems[0].url;
   err = null;
   try {
     yield gStore.addItem(item);
   }
   catch (e) {
     err = e;
   }
-  checkError(err, "UNIQUE constraint failed: items.url");
+  Assert.ok(err);
+  Assert.ok(err instanceof ReadingList.Error.Exists);
+  Assert.ok(err.message);
+  Assert.ok(err.message.indexOf("An item with the following property already exists: url") >= 0);
 
   // update an item with an existing url
   item.guid = gItems[1].guid;
   err = null;
   try {
     yield gStore.updateItem(item);
   }
   catch (e) {
     err = e;
   }
   // The failure actually happens on items.guid, not items.url, because the item
   // is first looked up by url, and then its other properties are updated on the
   // resulting row.
-  checkError(err, "UNIQUE constraint failed: items.guid");
+  Assert.ok(err);
+  Assert.ok(err instanceof ReadingList.Error.Exists);
+  Assert.ok(err.message);
+  Assert.ok(err.message.indexOf("An item with the following property already exists: guid") >= 0);
 
   // add a new item with an existing resolvedURL
   item = kindOfClone(gItems[0]);
   item.resolvedURL = gItems[0].resolvedURL;
   err = null;
   try {
     yield gStore.addItem(item);
   }
   catch (e) {
     err = e;
   }
-  checkError(err, "UNIQUE constraint failed: items.resolvedURL");
+  Assert.ok(err);
+  Assert.ok(err instanceof ReadingList.Error.Exists);
+  Assert.ok(err.message);
+  Assert.ok(err.message.indexOf("An item with the following property already exists: resolvedURL") >= 0);
 
   // update an item with an existing resolvedURL
   item.url = gItems[1].url;
   err = null;
   try {
     yield gStore.updateItem(item);
   }
   catch (e) {
     err = e;
   }
-  checkError(err, "UNIQUE constraint failed: items.resolvedURL");
+  Assert.ok(err);
+  Assert.ok(err instanceof ReadingList.Error.Exists);
+  Assert.ok(err.message);
+  Assert.ok(err.message.indexOf("An item with the following property already exists: resolvedURL") >= 0);
 
   // add a new item with no guid, which is allowed
   item = kindOfClone(gItems[0]);
   delete item.guid;
   err = null;
   try {
     yield gStore.addItem(item);
   }
@@ -307,15 +326,8 @@ function checkItems(actualItems, expecte
   Assert.equal(actualItems.length, expectedItems.length);
   for (let i = 0; i < expectedItems.length; i++) {
     for (let prop in expectedItems[i]) {
       Assert.ok(prop in actualItems[i], prop);
       Assert.equal(actualItems[i][prop], expectedItems[i][prop]);
     }
   }
 }
-
-function checkError(err, expectedMsgSubstring) {
-  Assert.ok(err);
-  Assert.ok(err instanceof Cu.getGlobalForObject(Sqlite).Error);
-  Assert.ok(err.message);
-  Assert.ok(err.message.indexOf(expectedMsgSubstring) >= 0, err.message);
-}