Bug 711914 - Fetching bookmarks information during onBeforeItemRemove may break the bookmarks cache.

r=dietrich

toolkit/components/places/nsNavBookmarks.cpp

@@ -63,10 +63,24 @@
 #define RECENT_BOOKMARKS_THRESHOLD PRTime((PRInt64)1 * 60 * PR_USEC_PER_SEC)
 
 #define BEGIN_CRITICAL_BOOKMARK_CACHE_SECTION(_itemId_) \
+  mUncachableBookmarks.PutEntry(_itemId_); \
   mRecentBookmarksCache.RemoveEntry(_itemId_)
 
 #define END_CRITICAL_BOOKMARK_CACHE_SECTION(_itemId_) \
-  MOZ_ASSERT(!mRecentBookmarksCache.GetEntry(_itemId_))
+  MOZ_ASSERT(!mRecentBookmarksCache.GetEntry(_itemId_)); \
+  MOZ_ASSERT(mUncachableBookmarks.GetEntry(_itemId_)); \
+  mUncachableBookmarks.RemoveEntry(_itemId_)
+
+#define ADD_TO_BOOKMARK_CACHE(_itemId_, _data_) \
+  PR_BEGIN_MACRO \
+  ExpireNonrecentBookmarks(&mRecentBookmarksCache); \
+  if (!mUncachableBookmarks.GetEntry(_itemId_)) { \
+    BookmarkKeyClass* key = mRecentBookmarksCache.PutEntry(_itemId_); \
+    if (key) { \
+      key->bookmark = _data_; \
+    } \
+  } \
+  PR_END_MACRO
 
 #define TOPIC_PLACES_MAINTENANCE "places-maintenance-finished"
 
@@ -266,6 +280,8 @@ nsNavBookmarks::Init()
   NS_ENSURE_STATE(mDB);
 
   mRecentBookmarksCache.Init(RECENT_BOOKMARKS_INITIAL_CACHE_SIZE);
+  mUncachableBookmarks.Init(RECENT_BOOKMARKS_INITIAL_CACHE_SIZE);
+
   nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
   if (os) {
     (void)os->AddObserver(this, TOPIC_PLACES_MAINTENANCE, true);
@@ -563,13 +579,7 @@ nsNavBookmarks::InsertBookmarkInDB(PRInt64 aPlaceId,
   bookmark.parentGuid = aParentGuid;
   bookmark.grandParentId = aGrandParentId;
 
-  // Make space for the new entry.
-  ExpireNonrecentBookmarks(&mRecentBookmarksCache);
-  // Update the recent bookmarks cache.
-  BookmarkKeyClass* key = mRecentBookmarksCache.PutEntry(*_itemId);
-  if (key) {
-    key->bookmark = bookmark;
-  }
+  ADD_TO_BOOKMARK_CACHE(*_itemId, bookmark);
 
   return NS_OK;
 }
@@ -1470,13 +1480,7 @@ nsNavBookmarks::FetchItemInfo(PRInt64 aItemId,
     _bookmark.grandParentId = -1;
   }
 
-  // Make space for the new entry.
-  ExpireNonrecentBookmarks(&mRecentBookmarksCache);
-  // Update the recent bookmarks cache.
-  key = mRecentBookmarksCache.PutEntry(aItemId);
-  if (key) {
-    key->bookmark = _bookmark;
-  }
+  ADD_TO_BOOKMARK_CACHE(aItemId, _bookmark);
 
   return NS_OK;
 }

toolkit/components/places/nsNavBookmarks.h

@@ -473,6 +473,12 @@ private:
    * This is used to speed up repeated requests to the same item id.
    */
   nsTHashtable<BookmarkKeyClass> mRecentBookmarksCache;
+
+  /**
+   * Tracks bookmarks in the cache critical path.  Items should not be
+   * added to the cache till they are removed from this hash.
+   */
+  nsTHashtable<nsTrimInt64HashKey> mUncachableBookmarks;
 };
 
 #endif // nsNavBookmarks_h_

toolkit/components/places/tests/bookmarks/test_711914.js

@@ -0,0 +1,58 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+function run_test() {
+  /**
+   * Requests information to the service, so that bookmark's data is cached.
+   * @param aItemId
+   *        Id of the bookmark to be cached.
+   */
+  function forceBookmarkCaching(aItemId) {
+    let parent = PlacesUtils.bookmarks.getFolderIdForItem(aItemId);
+    PlacesUtils.bookmarks.getFolderIdForItem(parent);
+  }
+
+  let observer = {
+    onBeginUpdateBatch: function () forceBookmarkCaching(itemId1),
+    onEndUpdateBatch: function () forceBookmarkCaching(itemId1),
+    onItemAdded: forceBookmarkCaching,
+    onItemChanged: forceBookmarkCaching,
+    onItemMoved: forceBookmarkCaching,
+    onBeforeItemRemoved: forceBookmarkCaching,
+    onItemRemoved: function (id) {
+      try {
+        forceBookmarkCaching(id);
+        do_throw("trying to fetch a removed bookmark should throw");
+      } catch (ex) {}
+    },
+    onItemVisited: forceBookmarkCaching,
+    QueryInterface: XPCOMUtils.generateQI([Ci.nsINavBookmarkObserver])
+  };
+  PlacesUtils.bookmarks.addObserver(observer, false);
+
+  let folder1 = PlacesUtils.bookmarks
+                           .createFolder(PlacesUtils.bookmarksMenuFolderId,
+                                         "Folder1",
+                                         PlacesUtils.bookmarks.DEFAULT_INDEX);
+  let folder2 = PlacesUtils.bookmarks
+                           .createFolder(folder1,
+                                         "Folder2",
+                                         PlacesUtils.bookmarks.DEFAULT_INDEX);
+  let itemId = PlacesUtils.bookmarks
+                           .insertBookmark(folder2,
+                                           NetUtil.newURI("http://mozilla.org/"),
+                                           PlacesUtils.bookmarks.DEFAULT_INDEX,
+                                           "Mozilla");
+
+  PlacesUtils.bookmarks.removeFolderChildren(folder1);
+
+  // Check title is correctly reported.
+  do_check_eq(PlacesUtils.bookmarks.getItemTitle(folder1), "Folder1");
+  try {
+    PlacesUtils.bookmarks.getItemTitle(folder2);
+    do_throw("trying to fetch a removed bookmark should throw");
+  } catch (ex) {}
+
+  PlacesUtils.bookmarks.removeObserver(observer, false);
+}

toolkit/components/places/tests/bookmarks/xpcshell.ini

@@ -32,4 +32,4 @@ tail =
 [test_restore_guids.js]
 [test_savedsearches.js]
 [test_675416.js]
-
+[test_711914.js]