Bug 791562 - crash in PlacesFolderConversion::AppendFolder.

r=mano

toolkit/components/places/nsNavBookmarks.cpp

@@ -642,7 +642,7 @@ 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);
@@ -1137,6 +1137,7 @@ 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);
@@ -1272,13 +1273,13 @@ nsNavBookmarks::RemoveFolderChildren(int64_t aFolderId)
 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);
 

toolkit/components/places/nsNavBookmarks.h

@@ -283,6 +283,12 @@ private:
   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,

toolkit/components/places/nsNavHistoryQuery.cpp

@@ -213,43 +213,38 @@ namespace PlacesFolderConversion {
    * @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;
     }
-
-    (void)bs->GetBookmarksMenuFolder(&folderID);
-    if (aFolderID == folderID) {
+    else if (NS_SUCCEEDED(bs->GetBookmarksMenuFolder(&folderID)) &&
+             aFolderID == folderID) {
       aQuery.AppendLiteral(BOOKMARKS_MENU_FOLDER);
-      return;
     }
-
-    (void)bs->GetTagsFolder(&folderID);
-    if (aFolderID == folderID) {
+    else if (NS_SUCCEEDED(bs->GetTagsFolder(&folderID)) &&
+             aFolderID == folderID) {
       aQuery.AppendLiteral(TAGS_FOLDER);
-      return;
     }
-
-    (void)bs->GetUnfiledBookmarksFolder(&folderID);
-    if (aFolderID == folderID) {
+    else if (NS_SUCCEEDED(bs->GetUnfiledBookmarksFolder(&folderID)) &&
+             aFolderID == folderID) {
       aQuery.AppendLiteral(UNFILED_BOOKMARKS_FOLDER);
-      return;
     }
-
-    (void)bs->GetToolbarFolder(&folderID);
-    if (aFolderID == folderID) {
+    else if (NS_SUCCEEDED(bs->GetToolbarFolder(&folderID)) &&
+             aFolderID == folderID) {
       aQuery.AppendLiteral(TOOLBAR_FOLDER);
-      return;
+    }
+    else {
+      // It wasn't one of our named constants, so just convert it to a string.
+      aQuery.AppendInt(aFolderID);
     }
 
-    // It wasn't one of our named constants, so just convert it to a string 
-    aQuery.AppendInt(aFolderID);
+    return NS_OK;
   }
 }
 
@@ -473,7 +468,8 @@ nsNavHistory::QueriesToQueryString(nsINavHistoryQuery **aQueries,
     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);
 

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");
+    }
+  }
+}

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

@@ -30,3 +30,4 @@ tail =
 [test_savedsearches.js]
 [test_675416.js]
 [test_711914.js]
+[test_protectRoots.js]