Magic-Mirror/gecko-dev
1
0
Fork 0
mirror of https://github.com/mozilla/gecko-dev.git synced 2025-02-15 13:09:14 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

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

Browse Source 
r=dietrich
This commit is contained in:
Marco Bonardo 2011-08-22 15:05:16 +02:00
parent 1e015144a5
commit d84964c811
4 changed files with 125 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

101
toolkit/components/places/nsNavBookmarks.cpp
View File

@ -67,6 +67,12 @@
// 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;
@ -1052,7 +1058,7 @@ 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,
@ -1086,6 +1092,8 @@ nsNavBookmarks::RemoveItem(PRInt64 aItemId)
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);
@ -1107,6 +1115,8 @@ nsNavBookmarks::RemoveItem(PRInt64 aItemId)
rv = transaction.Commit();
NS_ENSURE_SUCCESS(rv, rv);
END_CRITICAL_BOOKMARK_CACHE_SECTION(bookmark.id);
if (!mBatching) {
ForceWALCheckpoint(mDBConn);
}
@ -1501,7 +1511,7 @@ 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);
@ -1516,9 +1526,6 @@ nsNavBookmarks::RemoveFolderChildren(PRInt64 aFolderId)
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,
@ -1543,6 +1550,8 @@ nsNavBookmarks::RemoveFolderChildren(PRInt64 aFolderId)
}
}
}
BEGIN_CRITICAL_BOOKMARK_CACHE_SECTION(child.id);
}
// Delete items from the database now.
@ -1587,6 +1596,7 @@ nsNavBookmarks::RemoveFolderChildren(PRInt64 aFolderId)
rv = UpdateKeywordsHashForRemovedBookmark(child.id);
NS_ENSURE_SUCCESS(rv, rv);
}
END_CRITICAL_BOOKMARK_CACHE_SECTION(child.id);
}
rv = transaction.Commit();
@ -1658,7 +1668,7 @@ nsNavBookmarks::MoveItem(PRInt64 aItemId, PRInt64 aNewParent, PRInt32 aIndex)
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
@ -1739,6 +1749,8 @@ nsNavBookmarks::MoveItem(PRInt64 aItemId, PRInt64 aNewParent, PRInt32 aIndex)
NS_ENSURE_SUCCESS(rv, rv);
}
BEGIN_CRITICAL_BOOKMARK_CACHE_SECTION(bookmark.id);
{
// Update parent and position.
DECLARE_AND_ASSIGN_SCOPED_LAZY_STMT(stmt, mDBMoveItem);
@ -1763,6 +1775,8 @@ nsNavBookmarks::MoveItem(PRInt64 aItemId, PRInt64 aNewParent, PRInt32 aIndex)
rv = transaction.Commit();
NS_ENSURE_SUCCESS(rv, rv);
END_CRITICAL_BOOKMARK_CACHE_SECTION(bookmark.id);
NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
nsINavBookmarkObserver,
OnItemMoved(bookmark.id,
@ -1788,17 +1802,13 @@ nsNavBookmarks::MoveItem(PRInt64 aItemId, PRInt64 aNewParent, PRInt32 aIndex)
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;
}
@ -1864,15 +1874,14 @@ nsNavBookmarks::FetchItemInfo(PRInt64 aItemId,
_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;
}
@ -1881,6 +1890,8 @@ nsNavBookmarks::SetItemDateInternal(mozIStorageStatement* aStatement,
PRInt64 aItemId,
PRTime aValue)
{
BEGIN_CRITICAL_BOOKMARK_CACHE_SECTION(aItemId);
NS_ENSURE_STATE(aStatement);
mozStorageStatementScoper scoper(aStatement);
@ -1892,6 +1903,8 @@ nsNavBookmarks::SetItemDateInternal(mozIStorageStatement* aStatement,
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.
@ -1905,7 +1918,7 @@ 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;
@ -1936,7 +1949,7 @@ nsNavBookmarks::GetItemDateAdded(PRInt64 aItemId, PRTime* _dateAdded)
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;
@ -1950,7 +1963,7 @@ 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;
@ -1981,7 +1994,7 @@ nsNavBookmarks::GetItemLastModified(PRInt64 aItemId, PRTime* _lastModified)
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;
@ -2086,9 +2099,11 @@ 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()) {
@ -2109,6 +2124,8 @@ nsNavBookmarks::SetItemTitle(PRInt64 aItemId, const nsACString& aTitle)
rv = statement->Execute();
NS_ENSURE_SUCCESS(rv, rv);
END_CRITICAL_BOOKMARK_CACHE_SECTION(bookmark.id);
NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
nsINavBookmarkObserver,
OnItemChanged(bookmark.id,
@ -2131,7 +2148,7 @@ nsNavBookmarks::GetItemTitle(PRInt64 aItemId,
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;
@ -2147,7 +2164,7 @@ nsNavBookmarks::GetBookmarkURI(PRInt64 aItemId,
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);
@ -2164,7 +2181,7 @@ nsNavBookmarks::GetItemType(PRInt64 aItemId, PRUint16* _type)
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);
@ -2179,7 +2196,7 @@ nsNavBookmarks::GetFolderType(PRInt64 aItemId,
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;
@ -2193,7 +2210,7 @@ nsNavBookmarks::ResultNodeForContainer(PRInt64 aItemId,
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) {
@ -2461,7 +2478,7 @@ nsNavBookmarks::ChangeBookmarkURI(PRInt64 aBookmarkId, nsIURI* aNewURI)
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);
@ -2476,6 +2493,8 @@ nsNavBookmarks::ChangeBookmarkURI(PRInt64 aBookmarkId, nsIURI* aNewURI)
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);
@ -2491,6 +2510,8 @@ nsNavBookmarks::ChangeBookmarkURI(PRInt64 aBookmarkId, nsIURI* aNewURI)
rv = transaction.Commit();
NS_ENSURE_SUCCESS(rv, rv);
END_CRITICAL_BOOKMARK_CACHE_SECTION(bookmark.id);
rv = history->UpdateFrecency(newPlaceId);
NS_ENSURE_SUCCESS(rv, rv);
@ -2525,7 +2546,7 @@ nsNavBookmarks::GetFolderIdForItem(PRInt64 aItemId, PRInt64* _parentId)
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
@ -2651,7 +2672,7 @@ nsNavBookmarks::GetItemIndex(PRInt64 aItemId, PRInt32* _index)
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;
@ -2670,7 +2691,7 @@ nsNavBookmarks::SetItemIndex(PRInt64 aItemId, PRInt32 aNewIndex)
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.
@ -2683,6 +2704,8 @@ nsNavBookmarks::SetItemIndex(PRInt64 aItemId, PRInt32 aNewIndex)
// 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);
@ -2692,6 +2715,8 @@ nsNavBookmarks::SetItemIndex(PRInt64 aItemId, PRInt32 aNewIndex)
rv = stmt->Execute();
NS_ENSURE_SUCCESS(rv, rv);
END_CRITICAL_BOOKMARK_CACHE_SECTION(bookmark.id);
NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
nsINavBookmarkObserver,
OnItemMoved(bookmark.id,
@ -2729,7 +2754,7 @@ nsNavBookmarks::SetKeywordForBookmark(PRInt64 aBookmarkId,
// 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();
@ -2748,6 +2773,8 @@ nsNavBookmarks::SetKeywordForBookmark(PRInt64 aBookmarkId,
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;
@ -2798,6 +2825,8 @@ nsNavBookmarks::SetKeywordForBookmark(PRInt64 aBookmarkId,
rv = transaction.Commit();
NS_ENSURE_SUCCESS(rv, rv);
END_CRITICAL_BOOKMARK_CACHE_SECTION(bookmark.id);
NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
nsINavBookmarkObserver,
OnItemChanged(bookmark.id,
@ -3143,7 +3172,7 @@ nsNavBookmarks::OnPageChanged(nsIURI* aURI,
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);
}
@ -3195,7 +3224,7 @@ 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();

7
toolkit/components/places/nsNavBookmarks.h
View File

@ -228,12 +228,9 @@ public:
* 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.
@ -521,7 +518,7 @@ private:
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;

57
toolkit/components/places/tests/bookmarks/test_675416.js Normal file
View File

@ -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);
}

1
toolkit/components/places/tests/bookmarks/xpcshell.ini
View File

@ -31,4 +31,5 @@ tail =
[test_removeItem.js]
[test_restore_guids.js]
[test_savedsearches.js]
[test_675416.js]