Bug 711914 - Fetching bookmarks information during onBeforeItemRemove may break the bookmarks cache.
authorMarco Bonardo <mbonardo@mozilla.com>
Thu, 12 Jan 2012 17:04:09 +0100
changeset 85574 3c371703da3c1b5d08447509e83fb83128318e87
parent 85573 9e52edb8a5348cf6704ef41d6ae18323263549ea
child 85575 d76bf407d5c1014114a4e7616c6b20d64ae80eae
push id805
push userakeybl@mozilla.com
push dateWed, 01 Feb 2012 18:17:35 +0000
treeherdermozilla-aurora@6fb3bf232436 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs711914
milestone12.0a1
Bug 711914 - Fetching bookmarks information during onBeforeItemRemove may break the bookmarks cache. r=dietrich
toolkit/components/places/nsNavBookmarks.cpp
toolkit/components/places/nsNavBookmarks.h
toolkit/components/places/tests/bookmarks/test_711914.js
toolkit/components/places/tests/bookmarks/xpcshell.ini
--- a/toolkit/components/places/nsNavBookmarks.cpp
+++ b/toolkit/components/places/nsNavBookmarks.cpp
@@ -58,20 +58,34 @@
 #include "mozilla/Util.h"
 
 #define BOOKMARKS_TO_KEYWORDS_INITIAL_CACHE_SIZE 64
 #define RECENT_BOOKMARKS_INITIAL_CACHE_SIZE 10
 // Threashold to expire old bookmarks if the initial cache size is exceeded.
 #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"
 
 using namespace mozilla;
 
 // These columns sit to the right of the kGetInfoIndex_* columns.
 const PRInt32 nsNavBookmarks::kGetChildrenIndex_Position = 14;
 const PRInt32 nsNavBookmarks::kGetChildrenIndex_Type = 15;
@@ -261,16 +275,18 @@ nsresult
 nsNavBookmarks::Init()
 {
   NS_TIME_FUNCTION;
 
   mDB = Database::GetDatabase();
   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);
     (void)os->AddObserver(this, TOPIC_PLACES_SHUTDOWN, true);
     (void)os->AddObserver(this, TOPIC_PLACES_CONNECTION_CLOSED, true);
   }
 
   nsresult rv = ReadRoots();
@@ -558,23 +574,17 @@ nsNavBookmarks::InsertBookmarkInDB(PRInt
     bookmark.lastModified = aDateAdded;
   if (aURI) {
     rv = aURI->GetSpec(bookmark.url);
     NS_ENSURE_SUCCESS(rv, rv);
   }
   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;
 }
 
 
 NS_IMETHODIMP
 nsNavBookmarks::InsertBookmark(PRInt64 aFolder,
                                nsIURI* aURI,
@@ -1465,23 +1475,17 @@ nsNavBookmarks::FetchItemInfo(PRInt64 aI
     NS_ENSURE_SUCCESS(rv, rv);
     rv = stmt->GetInt64(11, &_bookmark.grandParentId);
     NS_ENSURE_SUCCESS(rv, rv);
   }
   else {
     _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;
 }
 
 nsresult
 nsNavBookmarks::SetItemDateInternal(enum BookmarkDate aDateType,
                                     PRInt64 aItemId,
                                     PRTime aValue)
--- a/toolkit/components/places/nsNavBookmarks.h
+++ b/toolkit/components/places/nsNavBookmarks.h
@@ -468,11 +468,17 @@ private:
    */
   nsresult UpdateKeywordsHashForRemovedBookmark(PRInt64 aItemId);
 
   /**
    * Cache for the last fetched BookmarkData entries.
    * 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_
new file mode 100644
--- /dev/null
+++ b/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);
+}
--- a/toolkit/components/places/tests/bookmarks/xpcshell.ini
+++ b/toolkit/components/places/tests/bookmarks/xpcshell.ini
@@ -27,9 +27,9 @@ tail =
 [test_keywords.js]
 [test_livemarks.js]
 [test_nsINavBookmarkObserver.js]
 [test_onBeforeItemRemoved_observer.js]
 [test_removeItem.js]
 [test_restore_guids.js]
 [test_savedsearches.js]
 [test_675416.js]
-
+[test_711914.js]