Bug 428133 - removeItem and removeFolder accept ids of the wrong type (r=mano, a=beltzner)

toolkit/components/places/public/nsINavBookmarksService.idl

@@ -263,6 +263,11 @@ interface nsINavBookmarksService : nsISupports
 
   /**
    * Removes a folder from the bookmarks tree.
+   *
+   * NOTE: This API is deprecated. The correct method to use is removeItem.
+   *       This will be removed in the next release after Firefox 3.0. The
+   *       removal is in bug 428558.
+   *
    *  @param aFolder
    *         The id of the folder to remove.
    */

toolkit/components/places/src/nsNavBookmarks.cpp

@@ -657,16 +657,16 @@ nsNavBookmarks::FillBookmarksHash()
 //    @see RecursiveAddBookmarkHash
 
 nsresult
-nsNavBookmarks::AddBookmarkToHash(PRInt64 aBookmarkId, PRTime aMinTime)
+nsNavBookmarks::AddBookmarkToHash(PRInt64 aPlaceId, PRTime aMinTime)
 {
   // this function might be called before our hashtable is initialized (for
   // example, on history import), just ignore these, we'll pick up the add when
   // the hashtable is initialized later
   if (! mBookmarksHash.IsInitialized())
     return NS_OK;
-  if (! mBookmarksHash.Put(aBookmarkId, aBookmarkId))
+  if (! mBookmarksHash.Put(aPlaceId, aPlaceId))
     return NS_ERROR_OUT_OF_MEMORY;
-  return RecursiveAddBookmarkHash(aBookmarkId, aBookmarkId, aMinTime);
+  return RecursiveAddBookmarkHash(aPlaceId, aPlaceId, aMinTime);
 }
 
 
@@ -683,7 +683,7 @@ nsNavBookmarks::AddBookmarkToHash(PRInt64 aBookmarkId, PRTime aMinTime)
 //    to search all history for redirects.
 
 nsresult
-nsNavBookmarks::RecursiveAddBookmarkHash(PRInt64 aBookmarkID,
+nsNavBookmarks::RecursiveAddBookmarkHash(PRInt64 aPlaceID,
                                          PRInt64 aCurrentSource,
                                          PRTime aMinTime)
 {
@@ -717,7 +717,7 @@ nsNavBookmarks::RecursiveAddBookmarkHash(PRInt64 aBookmarkID,
       if (mBookmarksHash.Get(curID, &alreadyExistingOne))
         continue;
 
-      if (! mBookmarksHash.Put(curID, aBookmarkID))
+      if (! mBookmarksHash.Put(curID, aPlaceID))
         return NS_ERROR_OUT_OF_MEMORY;
 
       // save for recursion later
@@ -727,7 +727,7 @@ nsNavBookmarks::RecursiveAddBookmarkHash(PRInt64 aBookmarkID,
 
   // recurse on each found item now that we're done with the statement
   for (PRUint32 i = 0; i < found.Length(); i ++) {
-    rv = RecursiveAddBookmarkHash(aBookmarkID, found[i], aMinTime);
+    rv = RecursiveAddBookmarkHash(aPlaceID, found[i], aMinTime);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
@@ -747,10 +747,10 @@ nsNavBookmarks::RecursiveAddBookmarkHash(PRInt64 aBookmarkID,
 
 PR_STATIC_CALLBACK(PLDHashOperator)
 RemoveBookmarkHashCallback(nsTrimInt64HashKey::KeyType aKey,
-                           PRInt64& aBookmark, void* aUserArg)
+                           PRInt64& aPlaceId, void* aUserArg)
 {
   const PRInt64* removeThisOne = reinterpret_cast<const PRInt64*>(aUserArg);
-  if (aBookmark == *removeThisOne)
+  if (aPlaceId == *removeThisOne)
     return PL_DHASH_REMOVE;
   return PL_DHASH_NEXT;
 }
@@ -986,21 +986,13 @@ nsNavBookmarks::InsertBookmark(PRInt64 aFolder, nsIURI *aItem, PRInt32 aIndex,
 NS_IMETHODIMP
 nsNavBookmarks::RemoveItem(PRInt64 aItemId)
 {
-  mozIStorageConnection *dbConn = DBConn();
-  mozStorageTransaction transaction(dbConn, PR_FALSE);
-
+  nsresult rv;
   PRInt32 childIndex;
   PRInt64 placeId, folderId;
   PRInt32 itemType;
   nsCAutoString buffer;
   nsCAutoString spec;
 
-  // First, remove item annotations
-  nsAnnotationService* annosvc = nsAnnotationService::GetAnnotationService();
-  NS_ENSURE_TRUE(annosvc, NS_ERROR_OUT_OF_MEMORY);
-  nsresult rv = annosvc->RemoveItemAnnotations(aItemId);
-  NS_ENSURE_SUCCESS(rv, rv);
-
   { // scoping to ensure the statement gets reset
     mozStorageStatementScoper scope(mDBGetItemProperties);
     mDBGetItemProperties->BindInt64Parameter(0, aItemId);
@@ -1022,6 +1014,21 @@ nsNavBookmarks::RemoveItem(PRInt64 aItemId)
     }
   }
 
+  if (itemType == TYPE_FOLDER) {
+    rv = RemoveFolder(aItemId);
+    NS_ENSURE_SUCCESS(rv, rv);
+    return NS_OK;
+  }
+
+  mozIStorageConnection *dbConn = DBConn();
+  mozStorageTransaction transaction(dbConn, PR_FALSE);
+
+  // First, remove item annotations
+  nsAnnotationService* annosvc = nsAnnotationService::GetAnnotationService();
+  NS_ENSURE_TRUE(annosvc, NS_ERROR_OUT_OF_MEMORY);
+  rv = annosvc->RemoveItemAnnotations(aItemId);
+  NS_ENSURE_SUCCESS(rv, rv);
+
   buffer.AssignLiteral("DELETE FROM moz_bookmarks WHERE id = ");
   buffer.AppendInt(aItemId);
 
@@ -1402,23 +1409,19 @@ nsNavBookmarks::GetParentAndIndexOfFolder(PRInt64 aFolder, PRInt64* aParent,
 }
 
 NS_IMETHODIMP
-nsNavBookmarks::RemoveFolder(PRInt64 aFolder)
+nsNavBookmarks::RemoveFolder(PRInt64 aFolderId)
 {
   mozIStorageConnection *dbConn = DBConn();
   mozStorageTransaction transaction(dbConn, PR_FALSE);
 
-  // First, remove item annotations
-  nsAnnotationService* annosvc = nsAnnotationService::GetAnnotationService();
-  NS_ENSURE_TRUE(annosvc, NS_ERROR_OUT_OF_MEMORY);
-  nsresult rv = annosvc->RemoveItemAnnotations(aFolder);
-  NS_ENSURE_SUCCESS(rv, rv);
+  nsresult rv;
 
   PRInt64 parent;
-  PRInt32 index;
+  PRInt32 index, type;
   nsCAutoString folderType;
   {
     mozStorageStatementScoper scope(mDBGetItemProperties);
-    rv = mDBGetItemProperties->BindInt64Parameter(0, aFolder);
+    rv = mDBGetItemProperties->BindInt64Parameter(0, aFolderId);
     NS_ENSURE_SUCCESS(rv, rv);
 
     PRBool results;
@@ -1428,30 +1431,42 @@ nsNavBookmarks::RemoveFolder(PRInt64 aFolder)
       return NS_ERROR_INVALID_ARG; // folder is not in the hierarchy
     }
 
+    type = mDBGetItemProperties->AsInt32(kGetItemPropertiesIndex_Type);
     parent = mDBGetItemProperties->AsInt64(kGetItemPropertiesIndex_Parent);
     index = mDBGetItemProperties->AsInt32(kGetItemPropertiesIndex_Position);
     rv = mDBGetItemProperties->GetUTF8String(kGetItemPropertiesIndex_ServiceContractId, folderType);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
+  if (type != TYPE_FOLDER) {
+    NS_WARNING("RemoveFolder(): aFolderId is not a folder!");
+    return NS_ERROR_INVALID_ARG; // aFolderId is not a folder!
+  }
+
+  // First, remove item annotations
+  nsAnnotationService* annosvc = nsAnnotationService::GetAnnotationService();
+  NS_ENSURE_TRUE(annosvc, NS_ERROR_OUT_OF_MEMORY);
+  rv = annosvc->RemoveItemAnnotations(aFolderId);
+  NS_ENSURE_SUCCESS(rv, rv);
+
   // If this is a container bookmark, try to notify its service.
   if (folderType.Length() > 0) {
     // There is a type associated with this folder; it's a livemark.
     nsCOMPtr<nsIDynamicContainer> bmcServ = do_GetService(folderType.get());
     if (bmcServ) {
-      rv = bmcServ->OnContainerRemoving(aFolder);
+      rv = bmcServ->OnContainerRemoving(aFolderId);
       if (NS_FAILED(rv))
         NS_WARNING("Remove folder container notification failed.");
     }
   }
 
   // Remove all of the folder's children
-  RemoveFolderChildren(aFolder);
+  RemoveFolderChildren(aFolderId);
 
   // Remove the folder from its parent
   nsCAutoString buffer;
   buffer.AssignLiteral("DELETE FROM moz_bookmarks WHERE id = ");
-  buffer.AppendInt(aFolder);
+  buffer.AppendInt(aFolderId);
   rv = dbConn->ExecuteSimpleSQL(buffer);
   NS_ENSURE_SUCCESS(rv, rv);
 
@@ -1464,12 +1479,12 @@ nsNavBookmarks::RemoveFolder(PRInt64 aFolder)
   rv = transaction.Commit();
   NS_ENSURE_SUCCESS(rv, rv);
 
-  if (aFolder == mToolbarFolder) {
+  if (aFolderId == mToolbarFolder) {
     mToolbarFolder = 0;
   }
 
   ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
-                      OnItemRemoved(aFolder, parent, index))
+                      OnItemRemoved(aFolderId, parent, index))
 
   return NS_OK;
 }
@@ -2135,6 +2150,8 @@ nsNavBookmarks::FolderCount(PRInt64 aFolder)
 NS_IMETHODIMP
 nsNavBookmarks::IsBookmarked(nsIURI *aURI, PRBool *aBookmarked)
 {
+  NS_ENSURE_ARG(aURI);
+
   nsNavHistory* history = History();
   NS_ENSURE_TRUE(history, NS_ERROR_UNEXPECTED);
 

toolkit/components/places/src/nsTaggingService.js

@@ -201,8 +201,9 @@ TaggingService.prototype = {
     var cc = node.childCount;
     if (wasOpen)
       node.containerOpen = false;
-    if (cc == 0)
-      this._bms.removeFolder(aTagId);
+    if (cc == 0) {
+      this._bms.removeFolder(node.itemId);
+    }
   },
 
   // nsITaggingService

toolkit/components/places/tests/bookmarks/test_removeItem.js

@@ -0,0 +1,70 @@
+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim:set ts=2 sw=2 sts=2 et: */
+/* ***** BEGIN LICENSE BLOCK *****
+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
+ *
+ * The contents of this file are subject to the Mozilla Public License Version
+ * 1.1 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * http://www.mozilla.org/MPL/
+ *
+ * Software distributed under the License is distributed on an "AS IS" basis,
+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
+ * for the specific language governing rights and limitations under the
+ * License.
+ *
+ * The Original Code is Bug 384370 code.
+ *
+ * The Initial Developer of the Original Code is Mozilla Corp.
+ * Portions created by the Initial Developer are Copyright (C) 2008
+ * the Initial Developer. All Rights Reserved.
+ *
+ * Contributor(s):
+ *  Dietrich Ayala <dietrich@mozilla.com>
+ *
+ * Alternatively, the contents of this file may be used under the terms of
+ * either the GNU General Public License Version 2 or later (the "GPL"), or
+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
+ * in which case the provisions of the GPL or the LGPL are applicable instead
+ * of those above. If you wish to allow use of your version of this file only
+ * under the terms of either the GPL or the LGPL, and not to allow others to
+ * use your version of this file under the terms of the MPL, indicate your
+ * decision by deleting the provisions above and replace them with the notice
+ * and other provisions required by the GPL or the LGPL. If you do not delete
+ * the provisions above, a recipient may use your version of this file under
+ * the terms of any one of the MPL, the GPL or the LGPL.
+ *
+ * ***** END LICENSE BLOCK ***** */
+
+Components.utils.import("resource://gre/modules/utils.js");
+var tests = [];
+
+
+const DEFAULT_INDEX = PlacesUtils.bookmarks.DEFAULT_INDEX;
+
+function run_test() {
+  // folder to hold this test
+  var folderId =
+    PlacesUtils.bookmarks.createFolder(PlacesUtils.toolbarFolderId,
+                                       "", DEFAULT_INDEX);
+
+  // add a bookmark to the new folder
+  var bookmarkURI = uri("http://iasdjkf");
+  do_check_false(PlacesUtils.bookmarks.isBookmarked(bookmarkURI));
+  var bookmarkId = PlacesUtils.bookmarks.insertBookmark(folderId, bookmarkURI,
+                                                        DEFAULT_INDEX, "");
+  do_check_eq(PlacesUtils.bookmarks.getItemTitle(bookmarkId), "");
+
+  // try to remove the bookmark using removeFolder
+  try {
+    PlacesUtils.bookmarks.removeFolder(bookmarkId);
+    do_throw("no exception when removing a bookmark via removeFolder()!");
+  } catch(ex) {}
+  do_check_true(PlacesUtils.bookmarks.isBookmarked(bookmarkURI));
+
+  // remove the folder using removeItem
+  PlacesUtils.bookmarks.removeItem(folderId);
+  do_check_eq(PlacesUtils.bookmarks.getBookmarkIdsForURI(bookmarkURI, {}).length, 0);
+  do_check_false(PlacesUtils.bookmarks.isBookmarked(bookmarkURI));
+  do_check_eq(PlacesUtils.bookmarks.getItemIndex(bookmarkId), -1);
+}