Bug 675416 - Fetching bookmarks information during onBeforeItemRemove may break the bookmarks cache.
authorMarco Bonardo <mbonardo@mozilla.com>
Mon, 22 Aug 2011 15:05:16 +0200
changeset 75651 1933e939180e4a1900be87fd2105291305da46fd
parent 75650 92b574170b330d2feaebcc5349fc7d834f93dd7d
child 75652 4b1410af17c7ff2bc900a5bd035256034eeda4bf
push id3
push userfelipc@gmail.com
push dateFri, 30 Sep 2011 20:09:13 +0000
bugs675416
milestone9.0a1
Bug 675416 - 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_675416.js
toolkit/components/places/tests/bookmarks/xpcshell.ini
--- a/toolkit/components/places/nsNavBookmarks.cpp
+++ b/toolkit/components/places/nsNavBookmarks.cpp
@@ -62,16 +62,22 @@
 #include "mozilla/FunctionTimer.h"
 #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_) \
+  mRecentBookmarksCache.RemoveEntry(_itemId_)
+
+#define END_CRITICAL_BOOKMARK_CACHE_SECTION(_itemId_) \
+  MOZ_ASSERT(!mRecentBookmarksCache.GetEntry(_itemId_))
+
 #define TOPIC_PLACES_MAINTENANCE "places-maintenance-finished"
 
 const PRInt32 nsNavBookmarks::kFindURIBookmarksIndex_Id = 0;
 const PRInt32 nsNavBookmarks::kFindURIBookmarksIndex_Guid = 1;
 const PRInt32 nsNavBookmarks::kFindURIBookmarksIndex_ParentId = 2;
 const PRInt32 nsNavBookmarks::kFindURIBookmarksIndex_LastModified = 3;
 const PRInt32 nsNavBookmarks::kFindURIBookmarksIndex_ParentGuid = 4;
 const PRInt32 nsNavBookmarks::kFindURIBookmarksIndex_GrandParentId = 5;
@@ -1047,17 +1053,17 @@ nsNavBookmarks::InsertBookmark(PRInt64 a
 
 
 NS_IMETHODIMP
 nsNavBookmarks::RemoveItem(PRInt64 aItemId)
 {
   NS_ENSURE_ARG(aItemId != mRoot);
 
   BookmarkData bookmark;
-  nsresult rv = FetchItemInfo(aItemId, bookmark, true);
+  nsresult rv = FetchItemInfo(aItemId, bookmark);
   NS_ENSURE_SUCCESS(rv, rv);
 
   NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
                    nsINavBookmarkObserver,
                    OnBeforeItemRemoved(bookmark.id,
                                        bookmark.type,
                                        bookmark.parentId,
                                        bookmark.guid,
@@ -1081,16 +1087,18 @@ nsNavBookmarks::RemoveItem(PRInt64 aItem
       }
     }
 
     // Remove all of the folder's children.
     rv = RemoveFolderChildren(bookmark.id);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
+  BEGIN_CRITICAL_BOOKMARK_CACHE_SECTION(bookmark.id);
+
   DECLARE_AND_ASSIGN_SCOPED_LAZY_STMT(stmt, mDBRemoveItem);
   rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("item_id"), bookmark.id);
   NS_ENSURE_SUCCESS(rv, rv);
   rv = stmt->Execute();
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Fix indices in the parent.
   if (bookmark.position != DEFAULT_INDEX) {
@@ -1102,16 +1110,18 @@ nsNavBookmarks::RemoveItem(PRInt64 aItem
   bookmark.lastModified = PR_Now();
   rv = SetItemDateInternal(GetStatement(mDBSetItemLastModified),
                            bookmark.parentId, bookmark.lastModified);
   NS_ENSURE_SUCCESS(rv, rv);
 
   rv = transaction.Commit();
   NS_ENSURE_SUCCESS(rv, rv);
 
+  END_CRITICAL_BOOKMARK_CACHE_SECTION(bookmark.id);
+
   if (!mBatching) {
     ForceWALCheckpoint(mDBConn);
   }
 
   nsCOMPtr<nsIURI> uri;
   if (bookmark.type == TYPE_BOOKMARK) {
     nsNavHistory* history = nsNavHistory::GetHistoryService();
     NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY);
@@ -1496,34 +1506,31 @@ nsNavBookmarks::GetDescendantChildren(PR
 
 
 NS_IMETHODIMP
 nsNavBookmarks::RemoveFolderChildren(PRInt64 aFolderId)
 {
   NS_ENSURE_ARG_MIN(aFolderId, 1);
 
   BookmarkData folder;
-  nsresult rv = FetchItemInfo(aFolderId, folder, true);
+  nsresult rv = FetchItemInfo(aFolderId, folder);
   NS_ENSURE_SUCCESS(rv, rv);
   NS_ENSURE_ARG(folder.type == TYPE_FOLDER);
 
   // Fill folder children array recursively.
   nsTArray<BookmarkData> folderChildrenArray;
   rv = GetDescendantChildren(folder.id, folder.guid, folder.parentId,
                              folderChildrenArray);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Build a string of folders whose children will be removed.
   nsCString foldersToRemove;
   for (PRUint32 i = 0; i < folderChildrenArray.Length(); ++i) {
     BookmarkData& child = folderChildrenArray[i];
 
-    // Invalidate the bookmark cache.
-    mRecentBookmarksCache.RemoveEntry(child.id);
-
     // Notify observers that we are about to remove this child.
     NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
                      nsINavBookmarkObserver,
                      OnBeforeItemRemoved(child.id,
                                          child.type,
                                          child.parentId,
                                          child.guid,
                                          child.parentGuid));
@@ -1538,16 +1545,18 @@ nsNavBookmarks::RemoveFolderChildren(PRI
       if (!child.serviceCID.IsEmpty()) {
         nsCOMPtr<nsIDynamicContainer> svc =
           do_GetService(child.serviceCID.get());
         if (svc) {
           (void)svc->OnContainerRemoving(child.id);
         }
       }
     }
+
+    BEGIN_CRITICAL_BOOKMARK_CACHE_SECTION(child.id);
   }
 
   // Delete items from the database now.
   mozStorageTransaction transaction(mDBConn, PR_FALSE);
 
   nsCOMPtr<mozIStorageStatement> deleteStatement;
   rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
       "DELETE FROM moz_bookmarks "
@@ -1582,16 +1591,17 @@ nsNavBookmarks::RemoveFolderChildren(PRI
       nsNavHistory* history = nsNavHistory::GetHistoryService();
       NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY);
       rv = history->UpdateFrecency(child.placeId);
       NS_ENSURE_SUCCESS(rv, rv);
 
       rv = UpdateKeywordsHashForRemovedBookmark(child.id);
       NS_ENSURE_SUCCESS(rv, rv);
     }
+    END_CRITICAL_BOOKMARK_CACHE_SECTION(child.id);
   }
 
   rv = transaction.Commit();
   NS_ENSURE_SUCCESS(rv, rv);
 
   if (!mBatching) {
     ForceWALCheckpoint(mDBConn);
   }
@@ -1653,17 +1663,17 @@ nsNavBookmarks::MoveItem(PRInt64 aItemId
   // -1 is append, but no other negative number is allowed.
   NS_ENSURE_ARG_MIN(aIndex, -1);
   // Disallow making an item its own parent.
   NS_ENSURE_TRUE(aItemId != aNewParent, NS_ERROR_INVALID_ARG);
 
   mozStorageTransaction transaction(mDBConn, PR_FALSE);
 
   BookmarkData bookmark;
-  nsresult rv = FetchItemInfo(aItemId, bookmark, true);
+  nsresult rv = FetchItemInfo(aItemId, bookmark);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // if parent and index are the same, nothing to do
   if (bookmark.parentId == aNewParent && bookmark.position == aIndex)
     return NS_OK;
 
   // Make sure aNewParent is not aFolder or a subfolder of aFolder.
   // TODO: make this performant, maybe with a nested tree (bug 408991).
@@ -1734,16 +1744,18 @@ nsNavBookmarks::MoveItem(PRInt64 aItemId
     // First, fill the hole from the removal from the old parent.
     rv = AdjustIndices(bookmark.parentId, bookmark.position + 1, PR_INT32_MAX, -1);
     NS_ENSURE_SUCCESS(rv, rv);
     // Now, make room in the new parent for the insertion.
     rv = AdjustIndices(aNewParent, newIndex, PR_INT32_MAX, 1);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
+  BEGIN_CRITICAL_BOOKMARK_CACHE_SECTION(bookmark.id);
+
   {
     // Update parent and position.
     DECLARE_AND_ASSIGN_SCOPED_LAZY_STMT(stmt, mDBMoveItem);
     rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("parent"), aNewParent);
     NS_ENSURE_SUCCESS(rv, rv);
     rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("item_index"), newIndex);
     NS_ENSURE_SUCCESS(rv, rv);
     rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("item_id"), bookmark.id);
@@ -1758,16 +1770,18 @@ nsNavBookmarks::MoveItem(PRInt64 aItemId
   NS_ENSURE_SUCCESS(rv, rv);
   rv = SetItemDateInternal(GetStatement(mDBSetItemLastModified),
                            aNewParent, now);
   NS_ENSURE_SUCCESS(rv, rv);
 
   rv = transaction.Commit();
   NS_ENSURE_SUCCESS(rv, rv);
 
+  END_CRITICAL_BOOKMARK_CACHE_SECTION(bookmark.id);
+
   NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
                    nsINavBookmarkObserver,
                    OnItemMoved(bookmark.id,
                                bookmark.parentId,
                                bookmark.position,
                                aNewParent,
                                newIndex,
                                bookmark.type,
@@ -1783,27 +1797,23 @@ nsNavBookmarks::MoveItem(PRInt64 aItemId
       (void)svc->OnContainerMoved(bookmark.id, aNewParent, newIndex);
     }
   }
   return NS_OK;
 }
 
 nsresult
 nsNavBookmarks::FetchItemInfo(PRInt64 aItemId,
-                              BookmarkData& _bookmark,
-                              bool aInvalidateCache)
+                              BookmarkData& _bookmark)
 {
   // Check if the requested id is in the recent cache and avoid the database
   // lookup if so.  Invalidate the cache after getting data if requested.
   BookmarkKeyClass* key = mRecentBookmarksCache.GetEntry(aItemId);
   if (key) {
     _bookmark = key->bookmark;
-    if (aInvalidateCache) {
-      mRecentBookmarksCache.RemoveEntry(aItemId);
-    }
     return NS_OK;
   }
 
   DECLARE_AND_ASSIGN_SCOPED_LAZY_STMT(stmt, mDBGetItemProperties);
   nsresult rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("item_id"), aItemId);
   NS_ENSURE_SUCCESS(rv, rv);
 
   PRBool hasResult;
@@ -1859,58 +1869,61 @@ nsNavBookmarks::FetchItemInfo(PRInt64 aI
     rv = stmt->GetInt64(kGetItemPropertiesIndex_GrandParentId,
                         &_bookmark.grandParentId);
     NS_ENSURE_SUCCESS(rv, rv);
   }
   else {
     _bookmark.grandParentId = -1;
   }
 
-  if (!aInvalidateCache) {
-    // Make space for the new entry.
-    ExpireNonrecentBookmarks(&mRecentBookmarksCache);
-    // Update the recent bookmarks cache.
-    BookmarkKeyClass* key = mRecentBookmarksCache.PutEntry(aItemId);
-    if (key) {
-      key->bookmark = _bookmark;
-    }
+  // Make space for the new entry.
+  ExpireNonrecentBookmarks(&mRecentBookmarksCache);
+  // Update the recent bookmarks cache.
+  key = mRecentBookmarksCache.PutEntry(aItemId);
+  if (key) {
+    key->bookmark = _bookmark;
   }
+
   return NS_OK;
 }
 
 nsresult
 nsNavBookmarks::SetItemDateInternal(mozIStorageStatement* aStatement,
                                     PRInt64 aItemId,
                                     PRTime aValue)
 {
+  BEGIN_CRITICAL_BOOKMARK_CACHE_SECTION(aItemId);
+
   NS_ENSURE_STATE(aStatement);
   mozStorageStatementScoper scoper(aStatement);
 
   nsresult rv = aStatement->BindInt64ByName(NS_LITERAL_CSTRING("date"), aValue);
   NS_ENSURE_SUCCESS(rv, rv);
   rv = aStatement->BindInt64ByName(NS_LITERAL_CSTRING("item_id"), aItemId);
   NS_ENSURE_SUCCESS(rv, rv);
 
   rv = aStatement->Execute();
   NS_ENSURE_SUCCESS(rv, rv);
 
+  END_CRITICAL_BOOKMARK_CACHE_SECTION(aItemId);
+
   // note, we are not notifying the observers
   // that the item has changed.
 
   return NS_OK;
 }
 
 
 NS_IMETHODIMP
 nsNavBookmarks::SetItemDateAdded(PRInt64 aItemId, PRTime aDateAdded)
 {
   NS_ENSURE_ARG_MIN(aItemId, 1);
 
   BookmarkData bookmark;
-  nsresult rv = FetchItemInfo(aItemId, bookmark, true);
+  nsresult rv = FetchItemInfo(aItemId, bookmark);
   NS_ENSURE_SUCCESS(rv, rv);
   bookmark.dateAdded = aDateAdded;
 
   rv = SetItemDateInternal(GetStatement(mDBSetItemDateAdded),
                            bookmark.id, bookmark.dateAdded);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Note: mDBSetItemDateAdded also sets lastModified to aDateAdded.
@@ -1931,31 +1944,31 @@ nsNavBookmarks::SetItemDateAdded(PRInt64
 
 NS_IMETHODIMP
 nsNavBookmarks::GetItemDateAdded(PRInt64 aItemId, PRTime* _dateAdded)
 {
   NS_ENSURE_ARG_MIN(aItemId, 1);
   NS_ENSURE_ARG_POINTER(_dateAdded);
 
   BookmarkData bookmark;
-  nsresult rv = FetchItemInfo(aItemId, bookmark, false);
+  nsresult rv = FetchItemInfo(aItemId, bookmark);
   NS_ENSURE_SUCCESS(rv, rv);
 
   *_dateAdded = bookmark.dateAdded;
   return NS_OK;
 }
 
 
 NS_IMETHODIMP
 nsNavBookmarks::SetItemLastModified(PRInt64 aItemId, PRTime aLastModified)
 {
   NS_ENSURE_ARG_MIN(aItemId, 1);
 
   BookmarkData bookmark;
-  nsresult rv = FetchItemInfo(aItemId, bookmark, true);
+  nsresult rv = FetchItemInfo(aItemId, bookmark);
   NS_ENSURE_SUCCESS(rv, rv);
   bookmark.lastModified = aLastModified;
 
   rv = SetItemDateInternal(GetStatement(mDBSetItemLastModified),
                            bookmark.id, bookmark.lastModified);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Note: mDBSetItemDateAdded also sets lastModified to aDateAdded.
@@ -1976,17 +1989,17 @@ nsNavBookmarks::SetItemLastModified(PRIn
 
 NS_IMETHODIMP
 nsNavBookmarks::GetItemLastModified(PRInt64 aItemId, PRTime* _lastModified)
 {
   NS_ENSURE_ARG_MIN(aItemId, 1);
   NS_ENSURE_ARG_POINTER(_lastModified);
 
   BookmarkData bookmark;
-  nsresult rv = FetchItemInfo(aItemId, bookmark, false);
+  nsresult rv = FetchItemInfo(aItemId, bookmark);
   NS_ENSURE_SUCCESS(rv, rv);
 
   *_lastModified = bookmark.lastModified;
   return NS_OK;
 }
 
 
 nsresult
@@ -2081,19 +2094,21 @@ nsNavBookmarks::GetItemIdForGUID(const n
 
 
 NS_IMETHODIMP
 nsNavBookmarks::SetItemTitle(PRInt64 aItemId, const nsACString& aTitle)
 {
   NS_ENSURE_ARG_MIN(aItemId, 1);
 
   BookmarkData bookmark;
-  nsresult rv = FetchItemInfo(aItemId, bookmark, true);
+  nsresult rv = FetchItemInfo(aItemId, bookmark);
   NS_ENSURE_SUCCESS(rv, rv);
 
+  BEGIN_CRITICAL_BOOKMARK_CACHE_SECTION(bookmark.id);
+
   DECLARE_AND_ASSIGN_SCOPED_LAZY_STMT(statement, mDBSetItemTitle);
   // Support setting a null title, we support this in insertBookmark.
   if (aTitle.IsVoid()) {
     rv = statement->BindNullByName(NS_LITERAL_CSTRING("item_title"));
   }
   else {
     rv = statement->BindUTF8StringByName(NS_LITERAL_CSTRING("item_title"),
                                          aTitle);
@@ -2104,16 +2119,18 @@ nsNavBookmarks::SetItemTitle(PRInt64 aIt
                                   bookmark.lastModified);
   NS_ENSURE_SUCCESS(rv, rv);
   rv = statement->BindInt64ByName(NS_LITERAL_CSTRING("item_id"), bookmark.id);
   NS_ENSURE_SUCCESS(rv, rv);
 
   rv = statement->Execute();
   NS_ENSURE_SUCCESS(rv, rv);
 
+  END_CRITICAL_BOOKMARK_CACHE_SECTION(bookmark.id);
+
   NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
                    nsINavBookmarkObserver,
                    OnItemChanged(bookmark.id,
                                  NS_LITERAL_CSTRING("title"),
                                  PR_FALSE,
                                  aTitle,
                                  bookmark.lastModified,
                                  bookmark.type,
@@ -2126,79 +2143,79 @@ nsNavBookmarks::SetItemTitle(PRInt64 aIt
 
 NS_IMETHODIMP
 nsNavBookmarks::GetItemTitle(PRInt64 aItemId,
                              nsACString& _title)
 {
   NS_ENSURE_ARG_MIN(aItemId, 1);
 
   BookmarkData bookmark;
-  nsresult rv = FetchItemInfo(aItemId, bookmark, false);
+  nsresult rv = FetchItemInfo(aItemId, bookmark);
   NS_ENSURE_SUCCESS(rv, rv);
 
   _title = bookmark.title;
   return NS_OK;
 }
 
 
 NS_IMETHODIMP
 nsNavBookmarks::GetBookmarkURI(PRInt64 aItemId,
                                nsIURI** _URI)
 {
   NS_ENSURE_ARG_MIN(aItemId, 1);
   NS_ENSURE_ARG_POINTER(_URI);
 
   BookmarkData bookmark;
-  nsresult rv = FetchItemInfo(aItemId, bookmark, false);
+  nsresult rv = FetchItemInfo(aItemId, bookmark);
   NS_ENSURE_SUCCESS(rv, rv);
 
   rv = NS_NewURI(_URI, bookmark.url);
   NS_ENSURE_SUCCESS(rv, rv);
 
   return NS_OK;
 }
 
 
 NS_IMETHODIMP
 nsNavBookmarks::GetItemType(PRInt64 aItemId, PRUint16* _type)
 {
   NS_ENSURE_ARG_MIN(aItemId, 1);
   NS_ENSURE_ARG_POINTER(_type);
 
   BookmarkData bookmark;
-  nsresult rv = FetchItemInfo(aItemId, bookmark, false);
+  nsresult rv = FetchItemInfo(aItemId, bookmark);
   NS_ENSURE_SUCCESS(rv, rv);
 
   *_type = static_cast<PRUint16>(bookmark.type);
   return NS_OK;
 }
 
 
 nsresult
 nsNavBookmarks::GetFolderType(PRInt64 aItemId,
                               nsACString& _type)
 {
   NS_ENSURE_ARG_MIN(aItemId, 1);
 
   BookmarkData bookmark;
-  nsresult rv = FetchItemInfo(aItemId, bookmark, false);
+  nsresult rv = FetchItemInfo(aItemId, bookmark);
   NS_ENSURE_SUCCESS(rv, rv);
 
   _type = bookmark.serviceCID;
   return NS_OK;
 }
 
 
 nsresult
 nsNavBookmarks::ResultNodeForContainer(PRInt64 aItemId,
                                        nsNavHistoryQueryOptions* aOptions,
                                        nsNavHistoryResultNode** aNode)
 {
   BookmarkData bookmark;
-  nsresult rv = FetchItemInfo(aItemId, bookmark, false);
+  nsresult rv = FetchItemInfo(aItemId, bookmark);
   NS_ENSURE_SUCCESS(rv, rv);
 
   if (bookmark.type == TYPE_DYNAMIC_CONTAINER) {
     *aNode = new nsNavHistoryContainerResultNode(EmptyCString(),
                                                  bookmark.title,
                                                  EmptyCString(),
                                                  nsINavHistoryResultNode::RESULT_TYPE_DYNAMIC_CONTAINER,
                                                  PR_TRUE,
@@ -2456,46 +2473,50 @@ nsNavBookmarks::GetBookmarkedURIFor(nsIU
 
 NS_IMETHODIMP
 nsNavBookmarks::ChangeBookmarkURI(PRInt64 aBookmarkId, nsIURI* aNewURI)
 {
   NS_ENSURE_ARG_MIN(aBookmarkId, 1);
   NS_ENSURE_ARG(aNewURI);
 
   BookmarkData bookmark;
-  nsresult rv = FetchItemInfo(aBookmarkId, bookmark, true);
+  nsresult rv = FetchItemInfo(aBookmarkId, bookmark);
   NS_ENSURE_SUCCESS(rv, rv);
   NS_ENSURE_ARG(bookmark.type == TYPE_BOOKMARK);
 
   mozStorageTransaction transaction(mDBConn, PR_FALSE);
 
   nsNavHistory* history = nsNavHistory::GetHistoryService();
   NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY);
   PRInt64 newPlaceId;
   nsCAutoString newPlaceGuid;
   rv = history->GetOrCreateIdForPage(aNewURI, &newPlaceId, newPlaceGuid);
   NS_ENSURE_SUCCESS(rv, rv);
   if (!newPlaceId)
     return NS_ERROR_INVALID_ARG;
 
+  BEGIN_CRITICAL_BOOKMARK_CACHE_SECTION(bookmark.id);
+
   DECLARE_AND_ASSIGN_SCOPED_LAZY_STMT(statement, mDBChangeBookmarkURI);
   rv = statement->BindInt64ByName(NS_LITERAL_CSTRING("page_id"), newPlaceId);
   NS_ENSURE_SUCCESS(rv, rv);
   bookmark.lastModified = PR_Now();
   rv = statement->BindInt64ByName(NS_LITERAL_CSTRING("date"),
                                   bookmark.lastModified);
   NS_ENSURE_SUCCESS(rv, rv);
   rv = statement->BindInt64ByName(NS_LITERAL_CSTRING("item_id"), bookmark.id);
   NS_ENSURE_SUCCESS(rv, rv);
   rv = statement->Execute();
   NS_ENSURE_SUCCESS(rv, rv);
 
   rv = transaction.Commit();
   NS_ENSURE_SUCCESS(rv, rv);
 
+  END_CRITICAL_BOOKMARK_CACHE_SECTION(bookmark.id);
+
   rv = history->UpdateFrecency(newPlaceId);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Upon changing the URI for a bookmark, update the frecency for the old
   // place as well.
   rv = history->UpdateFrecency(bookmark.placeId);
   NS_ENSURE_SUCCESS(rv, rv);
 
@@ -2520,17 +2541,17 @@ nsNavBookmarks::ChangeBookmarkURI(PRInt6
 
 NS_IMETHODIMP
 nsNavBookmarks::GetFolderIdForItem(PRInt64 aItemId, PRInt64* _parentId)
 {
   NS_ENSURE_ARG_MIN(aItemId, 1);
   NS_ENSURE_ARG_POINTER(_parentId);
 
   BookmarkData bookmark;
-  nsresult rv = FetchItemInfo(aItemId, bookmark, false);
+  nsresult rv = FetchItemInfo(aItemId, bookmark);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // this should not happen, but see bug #400448 for details
   NS_ENSURE_TRUE(bookmark.id != bookmark.parentId, NS_ERROR_UNEXPECTED);
 
   *_parentId = bookmark.parentId;
   return NS_OK;
 }
@@ -2646,17 +2667,17 @@ nsNavBookmarks::GetBookmarkIdsForURI(nsI
 
 NS_IMETHODIMP
 nsNavBookmarks::GetItemIndex(PRInt64 aItemId, PRInt32* _index)
 {
   NS_ENSURE_ARG_MIN(aItemId, 1);
   NS_ENSURE_ARG_POINTER(_index);
 
   BookmarkData bookmark;
-  nsresult rv = FetchItemInfo(aItemId, bookmark, false);
+  nsresult rv = FetchItemInfo(aItemId, bookmark);
   // With respect to the API.
   if (NS_FAILED(rv)) {
     *_index = -1;
     return NS_OK;
   }
 
   *_index = bookmark.position;
   return NS_OK;
@@ -2665,38 +2686,42 @@ nsNavBookmarks::GetItemIndex(PRInt64 aIt
 
 NS_IMETHODIMP
 nsNavBookmarks::SetItemIndex(PRInt64 aItemId, PRInt32 aNewIndex)
 {
   NS_ENSURE_ARG_MIN(aItemId, 1);
   NS_ENSURE_ARG_MIN(aNewIndex, 0);
 
   BookmarkData bookmark;
-  nsresult rv = FetchItemInfo(aItemId, bookmark, true);
+  nsresult rv = FetchItemInfo(aItemId, bookmark);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Ensure we are not going out of range.
   PRInt32 folderCount;
   PRInt64 grandParentId;
   nsCAutoString folderGuid;
   rv = FetchFolderInfo(bookmark.parentId, &folderCount, folderGuid, &grandParentId);
   NS_ENSURE_SUCCESS(rv, rv);
   NS_ENSURE_TRUE(aNewIndex < folderCount, NS_ERROR_INVALID_ARG);
   // Check the parent's guid is the expected one.
   MOZ_ASSERT(bookmark.parentGuid == folderGuid);
 
+  BEGIN_CRITICAL_BOOKMARK_CACHE_SECTION(bookmark.id);
+
   DECLARE_AND_ASSIGN_SCOPED_LAZY_STMT(stmt, mDBSetItemIndex);
   rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("item_id"), aItemId);
   NS_ENSURE_SUCCESS(rv, rv);
   rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("item_index"), aNewIndex);
   NS_ENSURE_SUCCESS(rv, rv);
 
   rv = stmt->Execute();
   NS_ENSURE_SUCCESS(rv, rv);
 
+  END_CRITICAL_BOOKMARK_CACHE_SECTION(bookmark.id);
+
   NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
                    nsINavBookmarkObserver,
                    OnItemMoved(bookmark.id,
                                bookmark.parentId,
                                bookmark.position,
                                bookmark.parentId,
                                aNewIndex,
                                bookmark.type,
@@ -2724,17 +2749,17 @@ nsNavBookmarks::UpdateKeywordsHashForRem
 NS_IMETHODIMP
 nsNavBookmarks::SetKeywordForBookmark(PRInt64 aBookmarkId,
                                       const nsAString& aUserCasedKeyword)
 {
   NS_ENSURE_ARG_MIN(aBookmarkId, 1);
 
   // This also ensures the bookmark is valid.
   BookmarkData bookmark;
-  nsresult rv = FetchItemInfo(aBookmarkId, bookmark, true);
+  nsresult rv = FetchItemInfo(aBookmarkId, bookmark);
   NS_ENSURE_SUCCESS(rv, rv);
 
   rv = EnsureKeywordsHash();
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Shortcuts are always lowercased internally.
   nsAutoString keyword(aUserCasedKeyword);
   ToLowerCase(keyword);
@@ -2743,16 +2768,18 @@ nsNavBookmarks::SetKeywordForBookmark(PR
   nsAutoString oldKeyword;
   rv = GetKeywordForBookmark(bookmark.id, oldKeyword);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Trying to set the same value or to remove a nonexistent keyword is a no-op.
   if (keyword.Equals(oldKeyword) || (keyword.IsEmpty() && oldKeyword.IsEmpty()))
     return NS_OK;
 
+  BEGIN_CRITICAL_BOOKMARK_CACHE_SECTION(bookmark.id);
+
   mozStorageTransaction transaction(mDBConn, PR_FALSE);
 
   nsCOMPtr<mozIStorageStatement> updateBookmarkStmt;
   rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
     "UPDATE moz_bookmarks "
     "SET keyword_id = (SELECT id FROM moz_keywords WHERE keyword = :keyword), "
         "lastModified = :date "
     "WHERE id = :item_id "
@@ -2793,16 +2820,18 @@ nsNavBookmarks::SetKeywordForBookmark(PR
                                            bookmark.id);
   NS_ENSURE_SUCCESS(rv, rv);
   rv = updateBookmarkStmt->Execute();
   NS_ENSURE_SUCCESS(rv, rv);
 
   rv = transaction.Commit();
   NS_ENSURE_SUCCESS(rv, rv);
 
+  END_CRITICAL_BOOKMARK_CACHE_SECTION(bookmark.id);
+
   NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
                    nsINavBookmarkObserver,
                    OnItemChanged(bookmark.id,
                                  NS_LITERAL_CSTRING("keyword"),
                                  PR_FALSE,
                                  NS_ConvertUTF16toUTF8(keyword),
                                  bookmark.lastModified,
                                  bookmark.type,
@@ -3138,17 +3167,17 @@ nsNavBookmarks::OnPageChanged(nsIURI* aU
       nsCOMArray<nsNavHistoryQuery> queries;
       nsCOMPtr<nsNavHistoryQueryOptions> options;
       rv = history->QueryStringToQueryArray(changeData.bookmark.url,
                                             &queries, getter_AddRefs(options));
       NS_ENSURE_SUCCESS(rv, rv);
 
       if (queries.Count() == 1 && queries[0]->Folders().Length() == 1) {
         // Fetch missing data.
-        rv = FetchItemInfo(queries[0]->Folders()[0], changeData.bookmark, false);
+        rv = FetchItemInfo(queries[0]->Folders()[0], changeData.bookmark);
         NS_ENSURE_SUCCESS(rv, rv);        
         NotifyItemChanged(changeData);
       }
     }
     else {
       nsRefPtr< AsyncGetBookmarksForURI<ItemChangeMethod, ItemChangeData> > notifier =
         new AsyncGetBookmarksForURI<ItemChangeMethod, ItemChangeData>(this, &nsNavBookmarks::NotifyItemChanged, changeData);
       notifier->Init();
@@ -3190,17 +3219,17 @@ nsNavBookmarks::OnPageAnnotationSet(nsIU
   return NS_OK;
 }
 
 
 NS_IMETHODIMP
 nsNavBookmarks::OnItemAnnotationSet(PRInt64 aItemId, const nsACString& aName)
 {
   BookmarkData bookmark;
-  nsresult rv = FetchItemInfo(aItemId, bookmark, true);
+  nsresult rv = FetchItemInfo(aItemId, bookmark);
   NS_ENSURE_SUCCESS(rv, rv);
 
   bookmark.lastModified = PR_Now();
   rv = SetItemDateInternal(GetStatement(mDBSetItemLastModified),
                            bookmark.id, bookmark.lastModified);
   NS_ENSURE_SUCCESS(rv, rv);
 
   NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
--- a/toolkit/components/places/nsNavBookmarks.h
+++ b/toolkit/components/places/nsNavBookmarks.h
@@ -223,22 +223,19 @@ public:
 
   /**
    * Fetches information about the specified id from the database.
    *
    * @param aItemId
    *        Id of the item to fetch information for.
    * @param aBookmark
    *        BookmarkData to store the information.
-   * @param aInvalidateCache
-   *        True if the cache should discard its entry after the fetching.
    */
   nsresult FetchItemInfo(PRInt64 aItemId,
-                         BookmarkData& _bookmark,
-                         bool aInvalidateCache);
+                         BookmarkData& _bookmark);
 
   /**
    * Finalize all internal statements.
    */
   nsresult FinalizeStatements();
 
   mozIStorageStatement* GetStatementById(BookmarkStatementId aStatementId)
   {
@@ -516,17 +513,17 @@ private:
     RemoveFolderTransaction(PRInt64 aID) : mID(aID) {}
 
     NS_DECL_ISUPPORTS
 
     NS_IMETHOD DoTransaction() {
       nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
       NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY);
       BookmarkData folder;
-      nsresult rv = bookmarks->FetchItemInfo(mID, folder, true);
+      nsresult rv = bookmarks->FetchItemInfo(mID, folder);
       // TODO (Bug 656935): store the BookmarkData struct instead.
       mParent = folder.parentId;
       mIndex = folder.position;
 
       rv = bookmarks->GetItemTitle(mID, mTitle);
       NS_ENSURE_SUCCESS(rv, rv);
 
       nsCAutoString type;
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/bookmarks/test_675416.js
@@ -0,0 +1,57 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.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) {
+    PlacesUtils.bookmarks.getFolderIdForItem(aItemId);
+  }
+
+  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 folderId1 = PlacesUtils.bookmarks
+                             .createFolder(PlacesUtils.bookmarksMenuFolderId,
+                                           "Bookmarks",
+                                           PlacesUtils.bookmarks.DEFAULT_INDEX);
+  let itemId1 = PlacesUtils.bookmarks
+                           .insertBookmark(folderId1,
+                                           NetUtil.newURI("http:/www.wired.com/wiredscience"),
+                                           PlacesUtils.bookmarks.DEFAULT_INDEX,
+                                           "Wired Science");
+
+  PlacesUtils.bookmarks.removeItem(folderId1);
+
+  let folderId2 = PlacesUtils.bookmarks
+                             .createFolder(PlacesUtils.bookmarksMenuFolderId,
+                                           "Science",
+                                           PlacesUtils.bookmarks.DEFAULT_INDEX);
+  let folderId3 = PlacesUtils.bookmarks
+                             .createFolder(folderId2,
+                                           "Blogs",
+                                           PlacesUtils.bookmarks.DEFAULT_INDEX);
+  // Check title is correctly reported.
+  do_check_eq(PlacesUtils.bookmarks.getItemTitle(folderId3), "Blogs");
+  do_check_eq(PlacesUtils.bookmarks.getItemTitle(folderId2), "Science");
+
+  PlacesUtils.bookmarks.removeObserver(observer, false);
+}
--- a/toolkit/components/places/tests/bookmarks/xpcshell.ini
+++ b/toolkit/components/places/tests/bookmarks/xpcshell.ini
@@ -26,9 +26,10 @@ tail =
 [test_getBookmarkedURIFor.js]
 [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]