From 5ae75175873e3bc7285e03faf3cad58e1c3723b0 Mon Sep 17 00:00:00 2001 From: Marco Bonardo Date: Mon, 25 May 2015 17:18:31 +0200 Subject: [PATCH] Bug 1012597 - Part 1: provide a way to invalidate the Places GUIDs cache. r=rnewman --- toolkit/components/places/PlacesUtils.jsm | 53 +++++++++++++++---- ...est_PlacesUtils_invalidateCachedGuidFor.js | 24 +++++++++ .../components/places/tests/unit/xpcshell.ini | 1 + 3 files changed, 69 insertions(+), 9 deletions(-) create mode 100644 toolkit/components/places/tests/unit/test_PlacesUtils_invalidateCachedGuidFor.js diff --git a/toolkit/components/places/PlacesUtils.jsm b/toolkit/components/places/PlacesUtils.jsm index e6ecb275ed1..308956ca78b 100644 --- a/toolkit/components/places/PlacesUtils.jsm +++ b/toolkit/components/places/PlacesUtils.jsm @@ -1464,7 +1464,9 @@ this.PlacesUtils = { * @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 @@ -1472,11 +1474,23 @@ this.PlacesUtils = { * * @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 @@ -1565,7 +1579,7 @@ this.PlacesUtils = { 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) @@ -2166,7 +2180,7 @@ let GuidHelper = { this.ensureObservingRemovedItems(); let itemId = rows[0].getResultByName("id"); - this.idsForGuids.set(aGuid, itemId); + this.updateCache(itemId, aGuid); return itemId; }), @@ -2185,10 +2199,31 @@ let GuidHelper = { 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)) { /** @@ -2201,14 +2236,14 @@ let GuidHelper = { 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), diff --git a/toolkit/components/places/tests/unit/test_PlacesUtils_invalidateCachedGuidFor.js b/toolkit/components/places/tests/unit/test_PlacesUtils_invalidateCachedGuidFor.js new file mode 100644 index 00000000000..d42d59e5d93 --- /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/); +}); diff --git a/toolkit/components/places/tests/unit/xpcshell.ini b/toolkit/components/places/tests/unit/xpcshell.ini index ac6e04331b0..398b65f9e01 100644 --- a/toolkit/components/places/tests/unit/xpcshell.ini +++ b/toolkit/components/places/tests/unit/xpcshell.ini @@ -120,6 +120,7 @@ skip-if = true [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]