Bug 1012597 - Part 1: provide a way to invalidate the Places GUIDs cache. r=rnewman
authorMarco Bonardo <mbonardo@mozilla.com>
Mon, 25 May 2015 17:18:31 +0200
changeset 276328 309a1680ece809629ffdcd82d3ee2662d8796387
parent 276327 db74101d7caa27ebccd33e1c9fd2590c47d84b99
child 276329 6d7fa2ed407402715fb068600f858d6d2a01921b
push id4932
push userjlund@mozilla.com
push dateMon, 10 Aug 2015 18:23:06 +0000
treeherdermozilla-beta@6dd5a4f5f745 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrnewman
bugs1012597
milestone41.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 1012597 - Part 1: provide a way to invalidate the Places GUIDs cache. r=rnewman
toolkit/components/places/PlacesUtils.jsm
toolkit/components/places/tests/unit/test_PlacesUtils_invalidateCachedGuidFor.js
toolkit/components/places/tests/unit/xpcshell.ini
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -1459,29 +1459,43 @@ this.PlacesUtils = {
    * its item id.
    *
    * @param aItemId
    *        an item id
    * @return {Promise}
    * @resolves to the GUID.
    * @rejects if aItemId is invalid.
    */
-  promiseItemGuid: function (aItemId) GuidHelper.getItemGuid(aItemId),
+  promiseItemGuid(aItemId) {
+    return GuidHelper.getItemGuid(aItemId)
+  },
 
   /**
    * Get the item id for an item (a bookmark, a folder or a separator) given
    * its unique id.
    *
    * @param aGuid
    *        an item GUID
-   * @retrun {Promise}
+   * @return {Promise}
    * @resolves to the GUID.
    * @rejects if there's no item for the given GUID.
    */
-  promiseItemId: function (aGuid) GuidHelper.getItemId(aGuid),
+  promiseItemId(aGuid) {
+    return GuidHelper.getItemId(aGuid)
+  },
+
+  /**
+   * Invalidate the GUID cache for the given itemId.
+   *
+   * @param aItemId
+   *        an item id
+   */
+  invalidateCachedGuidFor(aItemId) {
+    GuidHelper.invalidateCacheForItemId(aItemId)
+  },
 
   /**
    * Asynchronously retrieve a JS-object representation of a places bookmarks
    * item (a bookmark, a folder, or a separator) along with all of its
    * descendants.
    *
    * @param [optional] aItemGuid
    *        the (topmost) item to be queried.  If it's not passed, the places
@@ -1560,17 +1574,17 @@ this.PlacesUtils = {
       if (aIncludeParentGuid)
         copyProps("parentGuid");
 
       let itemId = aRow.getResultByName("id");
       if (aOptions.includeItemIds)
         item.id = itemId;
 
       // Cache it for promiseItemId consumers regardless.
-      GuidHelper.idsForGuids.set(item.guid, itemId);
+      GuidHelper.updateCache(itemId, item.guid);
 
       let type = aRow.getResultByName("type");
       if (type == Ci.nsINavBookmarksService.TYPE_BOOKMARK)
         copyProps("charset", "tags", "iconuri");
 
       // Add annotations.
       if (aRow.getResultByName("has_annos")) {
         try {
@@ -2161,17 +2175,17 @@ let GuidHelper = {
     let rows = yield conn.executeCached(
       "SELECT b.id, b.guid from moz_bookmarks b WHERE b.guid = :guid LIMIT 1",
       { guid: aGuid });
     if (rows.length == 0)
       throw new Error("no item found for the given GUID");
 
     this.ensureObservingRemovedItems();
     let itemId = rows[0].getResultByName("id");
-    this.idsForGuids.set(aGuid, itemId);
+    this.updateCache(itemId, aGuid);
     return itemId;
   }),
 
   getItemGuid: Task.async(function* (aItemId) {
     let cached = this.guidsForIds.get(aItemId);
     if (cached !== undefined)
       return cached;
 
@@ -2180,40 +2194,61 @@ let GuidHelper = {
     let rows = yield conn.executeCached(
       "SELECT b.id, b.guid from moz_bookmarks b WHERE b.id = :id LIMIT 1",
       { id: aItemId });
     if (rows.length == 0)
       throw new Error("no item found for the given itemId");
 
     this.ensureObservingRemovedItems();
     let guid = rows[0].getResultByName("guid");
-    this.guidsForIds.set(aItemId, guid);
+    this.updateCache(aItemId, guid);
     return guid;
   }),
 
+  /**
+   * Updates the cache.
+   *
+   * @note This is the only place where the cache should be populated,
+   *       invalidation relies on both Maps being populated at the same time.
+   */
+  updateCache(aItemId, aGuid) {
+    if (typeof(aItemId) != "number" || aItemId <= 0)
+      throw new Error("Trying to update the GUIDs cache with an invalid itemId");
+    if (typeof(aGuid) != "string" || !/^[a-zA-Z0-9\-_]{12}$/.test(aGuid))
+      throw new Error("Trying to update the GUIDs cache with an invalid GUID");
+    this.guidsForIds.set(aItemId, aGuid);
+    this.idsForGuids.set(aGuid, aItemId);
+  },
+
+  invalidateCacheForItemId(aItemId) {
+    let guid = this.guidsForIds.get(aItemId);
+    this.guidsForIds.delete(aItemId);
+    this.idsForGuids.delete(guid);
+  },
+
   ensureObservingRemovedItems: function () {
     if (!("observer" in this)) {
       /**
        * This observers serves two purposes:
        * (1) Invalidate cached id<->GUID paris on when items are removed.
        * (2) Cache GUIDs given us free of charge by onItemAdded/onItemRemoved.
       *      So, for exmaple, when the NewBookmark needs the new GUID, we already
       *      have it cached.
       */
       this.observer = {
         onItemAdded: (aItemId, aParentId, aIndex, aItemType, aURI, aTitle,
                       aDateAdded, aGuid, aParentGuid) => {
-          this.guidsForIds.set(aItemId, aGuid);
-          this.guidsForIds.set(aParentId, aParentGuid);
+          this.updateCache(aItemId, aGuid);
+          this.updateCache(aParentId, aParentGuid);
         },
         onItemRemoved:
         (aItemId, aParentId, aIndex, aItemTyep, aURI, aGuid, aParentGuid) => {
           this.guidsForIds.delete(aItemId);
           this.idsForGuids.delete(aGuid);
-          this.guidsForIds.set(aParentId, aParentGuid);
+          this.updateCache(aParentId, aParentGuid);
         },
 
         QueryInterface: XPCOMUtils.generateQI(Ci.nsINavBookmarkObserver),
 
         onBeginUpdateBatch: function() {},
         onEndUpdateBatch: function() {},
         onItemChanged: function() {},
         onItemVisited: function() {},
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/unit/test_PlacesUtils_invalidateCachedGuidFor.js
@@ -0,0 +1,24 @@
+add_task(function* () {
+  do_print("Add a bookmark.");
+  let bm = yield PlacesUtils.bookmarks.insert({ url: "http://example.com/",
+                                                parentGuid: PlacesUtils.bookmarks.unfiledGuid });
+  let id = yield PlacesUtils.promiseItemId(bm.guid);
+  Assert.equal((yield PlacesUtils.promiseItemGuid(id)), bm.guid);
+
+  // Ensure invalidating a non-existent itemId doesn't throw.
+  PlacesUtils.invalidateCachedGuidFor(null);
+  PlacesUtils.invalidateCachedGuidFor(9999);
+
+  do_print("Change the GUID.");
+  let db = yield PlacesUtils.promiseWrappedConnection();
+  yield db.execute("UPDATE moz_bookmarks SET guid = :guid WHERE id = :id",
+                   { guid: "123456789012", id});
+  // The cache should still point to the wrong id.
+  Assert.equal((yield PlacesUtils.promiseItemGuid(id)), bm.guid);
+
+  do_print("Invalidate the cache.");
+  PlacesUtils.invalidateCachedGuidFor(id);
+  Assert.equal((yield PlacesUtils.promiseItemGuid(id)), "123456789012");
+  Assert.equal((yield PlacesUtils.promiseItemId("123456789012")), id);
+  yield Assert.rejects(PlacesUtils.promiseItemId(bm.guid), /no item found for the given GUID/);
+});
--- a/toolkit/components/places/tests/unit/xpcshell.ini
+++ b/toolkit/components/places/tests/unit/xpcshell.ini
@@ -115,16 +115,17 @@ fail-if = os == "android"
 skip-if = true
 [test_null_interfaces.js]
 [test_onItemChanged_tags.js]
 [test_pageGuid_bookmarkGuid.js]
 [test_frecency_observers.js]
 [test_placeURIs.js]
 [test_PlacesSearchAutocompleteProvider.js]
 [test_PlacesUtils_asyncGetBookmarkIds.js]
+[test_PlacesUtils_invalidateCachedGuidFor.js]
 [test_PlacesUtils_lazyobservers.js]
 [test_placesTxn.js]
 [test_preventive_maintenance.js]
 # Bug 676989: test hangs consistently on Android
 skip-if = os == "android"
 [test_preventive_maintenance_checkAndFixDatabase.js]
 # Bug 676989: test hangs consistently on Android
 skip-if = os == "android"