Bug 1149105 - Fix various desktop reading list sync failures, add more logging. r=markh
authorDrew Willcoxon <adw@mozilla.com>
Tue, 31 Mar 2015 16:41:13 +1100
changeset 237207 7bb3942888eb216452ff1455ddb1c9a49fdacd44
parent 237206 4e8fd80f0e7aecd53f994a3c1813482a3703537a
child 237208 d222742756c4f5d60ce13c8102d5024ddc060fc0
push id57898
push usercbook@mozilla.com
push dateThu, 02 Apr 2015 12:14:17 +0000
treeherdermozilla-inbound@63c87250946e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh
bugs1149105
milestone40.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1149105 - Fix various desktop reading list sync failures, add more logging. r=markh
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);
@@ -571,17 +601,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.
 
@@ -591,16 +621,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
@@ -839,19 +870,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
@@ -898,17 +931,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
@@ -991,17 +1024,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
@@ -1009,17 +1042,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);
-}