Bug 791562 - crash in PlacesFolderConversion::AppendFolder.
authorMarco Bonardo <mbonardo@mozilla.com>
Mon, 17 Dec 2012 15:03:49 +0100
changeset 125389 d10f258a912091dcb529862b33cc298f9774b785
parent 125388 048673b6b48b51fc13302cd1e873eddd7a78b9e3
child 125390 f0b590fbfd9f0f8c81459d8687c6efdfd2536c9d
push id2151
push userlsblakk@mozilla.com
push dateTue, 19 Feb 2013 18:06:57 +0000
treeherdermozilla-beta@4952e88741ec [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs791562
milestone20.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 791562 - crash in PlacesFolderConversion::AppendFolder. r=mano
toolkit/components/places/nsNavBookmarks.cpp
toolkit/components/places/nsNavBookmarks.h
toolkit/components/places/nsNavHistoryQuery.cpp
toolkit/components/places/tests/bookmarks/test_protectRoots.js
toolkit/components/places/tests/bookmarks/xpcshell.ini
--- a/toolkit/components/places/nsNavBookmarks.cpp
+++ b/toolkit/components/places/nsNavBookmarks.cpp
@@ -637,17 +637,17 @@ nsNavBookmarks::InsertBookmark(int64_t a
   return NS_OK;
 }
 
 
 NS_IMETHODIMP
 nsNavBookmarks::RemoveItem(int64_t aItemId)
 {
   SAMPLE_LABEL("bookmarks", "RemoveItem");
-  NS_ENSURE_ARG(aItemId != mRoot);
+  NS_ENSURE_ARG(!IsRoot(aItemId));
 
   BookmarkData bookmark;
   nsresult rv = FetchItemInfo(aItemId, bookmark);
   NS_ENSURE_SUCCESS(rv, rv);
 
   NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
                    nsINavBookmarkObserver,
                    OnBeforeItemRemoved(bookmark.id,
@@ -1132,16 +1132,17 @@ nsNavBookmarks::GetDescendantChildren(in
 }
 
 
 NS_IMETHODIMP
 nsNavBookmarks::RemoveFolderChildren(int64_t aFolderId)
 {
   SAMPLE_LABEL("bookmarks", "RemoveFolderChilder");
   NS_ENSURE_ARG_MIN(aFolderId, 1);
+  NS_ENSURE_ARG(aFolderId != mRoot);
 
   BookmarkData folder;
   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;
@@ -1267,23 +1268,23 @@ nsNavBookmarks::RemoveFolderChildren(int
 
   return NS_OK;
 }
 
 
 NS_IMETHODIMP
 nsNavBookmarks::MoveItem(int64_t aItemId, int64_t aNewParent, int32_t aIndex)
 {
-  NS_ENSURE_TRUE(aItemId != mRoot, NS_ERROR_INVALID_ARG);
+  NS_ENSURE_ARG(!IsRoot(aItemId));
   NS_ENSURE_ARG_MIN(aItemId, 1);
   NS_ENSURE_ARG_MIN(aNewParent, 1);
   // -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);
+  NS_ENSURE_ARG(aItemId != aNewParent);
 
   mozStorageTransaction transaction(mDB->MainConn(), false);
 
   BookmarkData bookmark;
   nsresult rv = FetchItemInfo(aItemId, bookmark);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // if parent and index are the same, nothing to do
--- a/toolkit/components/places/nsNavBookmarks.h
+++ b/toolkit/components/places/nsNavBookmarks.h
@@ -278,16 +278,22 @@ private:
   nsMaybeWeakPtrArray<nsINavBookmarkObserver> mObservers;
 
   int64_t mRoot;
   int64_t mMenuRoot;
   int64_t mTagsRoot;
   int64_t mUnfiledRoot;
   int64_t mToolbarRoot;
 
+  inline bool IsRoot(int64_t aFolderId) {
+    return aFolderId == mRoot || aFolderId == mMenuRoot ||
+           aFolderId == mTagsRoot || aFolderId == mUnfiledRoot ||
+           aFolderId == mToolbarRoot;
+  }
+
   nsresult IsBookmarkedInDatabase(int64_t aBookmarkID, bool* aIsBookmarked);
 
   nsresult SetItemDateInternal(enum mozilla::places::BookmarkDate aDateType,
                                int64_t aItemId,
                                PRTime aValue);
 
   // Recursive method to build an array of folder's children
   nsresult GetDescendantChildren(int64_t aFolderId,
--- a/toolkit/components/places/nsNavHistoryQuery.cpp
+++ b/toolkit/components/places/nsNavHistoryQuery.cpp
@@ -208,53 +208,48 @@ namespace PlacesFolderConversion {
    * aQuery.
    *
    * @param aQuery
    *        The string to append the folder string to.  This is generally a
    *        query string, but could really be anything.
    * @param aFolderID
    *        The folder ID to convert to the proper named constant.
    */
-  inline void AppendFolder(nsCString &aQuery, int64_t aFolderID)
+  inline nsresult AppendFolder(nsCString &aQuery, int64_t aFolderID)
   {
     nsNavBookmarks *bs = nsNavBookmarks::GetBookmarksService();
+    NS_ENSURE_STATE(bs);
     int64_t folderID;
 
-    (void)bs->GetPlacesRoot(&folderID);
-    if (aFolderID == folderID) {
+    if (NS_SUCCEEDED(bs->GetPlacesRoot(&folderID)) &&
+        aFolderID == folderID) {
       aQuery.AppendLiteral(PLACES_ROOT_FOLDER);
-      return;
+    }
+    else if (NS_SUCCEEDED(bs->GetBookmarksMenuFolder(&folderID)) &&
+             aFolderID == folderID) {
+      aQuery.AppendLiteral(BOOKMARKS_MENU_FOLDER);
+    }
+    else if (NS_SUCCEEDED(bs->GetTagsFolder(&folderID)) &&
+             aFolderID == folderID) {
+      aQuery.AppendLiteral(TAGS_FOLDER);
     }
-
-    (void)bs->GetBookmarksMenuFolder(&folderID);
-    if (aFolderID == folderID) {
-      aQuery.AppendLiteral(BOOKMARKS_MENU_FOLDER);
-      return;
+    else if (NS_SUCCEEDED(bs->GetUnfiledBookmarksFolder(&folderID)) &&
+             aFolderID == folderID) {
+      aQuery.AppendLiteral(UNFILED_BOOKMARKS_FOLDER);
+    }
+    else if (NS_SUCCEEDED(bs->GetToolbarFolder(&folderID)) &&
+             aFolderID == folderID) {
+      aQuery.AppendLiteral(TOOLBAR_FOLDER);
+    }
+    else {
+      // It wasn't one of our named constants, so just convert it to a string.
+      aQuery.AppendInt(aFolderID);
     }
 
-    (void)bs->GetTagsFolder(&folderID);
-    if (aFolderID == folderID) {
-      aQuery.AppendLiteral(TAGS_FOLDER);
-      return;
-    }
-
-    (void)bs->GetUnfiledBookmarksFolder(&folderID);
-    if (aFolderID == folderID) {
-      aQuery.AppendLiteral(UNFILED_BOOKMARKS_FOLDER);
-      return;
-    }
-
-    (void)bs->GetToolbarFolder(&folderID);
-    if (aFolderID == folderID) {
-      aQuery.AppendLiteral(TOOLBAR_FOLDER);
-      return;
-    }
-
-    // It wasn't one of our named constants, so just convert it to a string 
-    aQuery.AppendInt(aFolderID);
+    return NS_OK;
   }
 }
 
 // nsNavHistory::QueryStringToQueries
 //
 //    From C++ places code, you should use QueryStringToQueryArray, this is
 //    the harder-to-use XPCOM version.
 
@@ -468,17 +463,18 @@ nsNavHistory::QueriesToQueryString(nsINa
 
     // folders
     int64_t *folders = nullptr;
     uint32_t folderCount = 0;
     query->GetFolders(&folderCount, &folders);
     for (uint32_t i = 0; i < folderCount; ++i) {
       AppendAmpersandIfNonempty(queryString);
       queryString += NS_LITERAL_CSTRING(QUERYKEY_FOLDER "=");
-      PlacesFolderConversion::AppendFolder(queryString, folders[i]);
+      nsresult rv = PlacesFolderConversion::AppendFolder(queryString, folders[i]);
+      NS_ENSURE_SUCCESS(rv, rv);
     }
     nsMemory::Free(folders);
 
     // tags
     const nsTArray<nsString> &tags = query->Tags();
     for (uint32_t i = 0; i < tags.Length(); ++i) {
       nsAutoCString escapedTag;
       if (!NS_Escape(NS_ConvertUTF16toUTF8(tags[i]), escapedTag, url_XAlphas))
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/bookmarks/test_protectRoots.js
@@ -0,0 +1,36 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+function run_test()
+{
+  const ROOTS = [
+    PlacesUtils.bookmarksMenuFolderId,
+    PlacesUtils.toolbarFolderId,
+    PlacesUtils.unfiledBookmarksFolderId,
+    PlacesUtils.tagsFolderId,
+    PlacesUtils.placesRootId
+  ];
+
+  for (let root of ROOTS) {
+    do_check_true(PlacesUtils.isRootItem(root));
+
+    try {
+      PlacesUtils.bookmarks.removeItem(root);
+      do_throw("Trying to remove a root should throw");
+    } catch (ex) {}
+
+    try {
+      PlacesUtils.bookmarks.moveItem(root, PlacesUtils.placesRootId, 0);
+      do_throw("Trying to move a root should throw");
+    } catch (ex) {}
+
+    try {
+      PlacesUtils.bookmarks.removeFolderChildren(root);
+      if (root == PlacesUtils.placesRootId)
+        do_throw("Trying to remove children of the main root should throw");
+    } catch (ex) {
+      if (root != PlacesUtils.placesRootId)
+        do_throw("Trying to remove children of other roots should not throw");
+    }
+  }
+}
--- a/toolkit/components/places/tests/bookmarks/xpcshell.ini
+++ b/toolkit/components/places/tests/bookmarks/xpcshell.ini
@@ -25,8 +25,9 @@ tail =
 [test_keywords.js]
 [test_livemarks.js]
 [test_nsINavBookmarkObserver.js]
 [test_onBeforeItemRemoved_observer.js]
 [test_removeItem.js]
 [test_savedsearches.js]
 [test_675416.js]
 [test_711914.js]
+[test_protectRoots.js]