Bug 326208 - API should not allow creation of "holes" in bookmark folders, r=dietrich

toolkit/components/places/public/nsINavBookmarksService.idl

@@ -458,9 +458,12 @@ interface nsINavBookmarksService : nsISupports
    *
    * WARNING: This is API is intended for scenarios such as folder sorting,
    *          where the caller manages the indices of *all* items in the folder.
+   *          You must always ensure each index is unique after a reordering.
    *
    *  @param aItemId    The id of the item to modify
    *  @param aNewIndex  The new index
+   * 
+   *  @throws If aNewIndex is out of bounds.
    */
   void setItemIndex(in long long aItemId, in long aNewIndex);
 

toolkit/components/places/src/nsNavBookmarks.cpp

@@ -260,9 +260,12 @@ nsNavBookmarks::InitStatements()
     getter_AddRefs(mDBGetChildren));
   NS_ENSURE_SUCCESS(rv, rv);
 
-  // mDBFolderCount: count all of the children of a given folder
+  // mDBFolderCount: count all of the children of a given folder and checks
+  // that it exists.
   rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
-      "SELECT COUNT(*) FROM moz_bookmarks WHERE parent = ?1"),
+      "SELECT COUNT(*), "
+      "(SELECT id FROM moz_bookmarks WHERE id = ?1) "
+      "FROM moz_bookmarks WHERE parent = ?1"),
     getter_AddRefs(mDBFolderCount));
   NS_ENSURE_SUCCESS(rv, rv);
 
@@ -1120,9 +1123,14 @@ nsNavBookmarks::InsertBookmark(PRInt64 aFolder,
   NS_ENSURE_SUCCESS(rv, rv);
 
   PRInt32 index;
-  if (aIndex == nsINavBookmarksService::DEFAULT_INDEX) {
-    index = FolderCount(aFolder);
-  } else {
+  PRInt32 folderCount;
+  rv = FolderCount(aFolder, &folderCount);
+  NS_ENSURE_SUCCESS(rv, rv);
+  if (aIndex == nsINavBookmarksService::DEFAULT_INDEX ||
+      aIndex >= folderCount) {
+    index = folderCount;
+  }
+  else {
     index = aIndex;
     rv = AdjustIndices(aFolder, index, PR_INT32_MAX, 1);
     NS_ENSURE_SUCCESS(rv, rv);
@@ -1392,9 +1400,12 @@ nsNavBookmarks::CreateContainerWithID(PRInt64 aItemId, PRInt64 aParent,
   mozStorageTransaction transaction(mDBConn, PR_FALSE);
 
   PRInt32 index;
-  nsresult rv;
-  if (*aIndex == nsINavBookmarksService::DEFAULT_INDEX) {
-    index = FolderCount(aParent);
+  PRInt32 folderCount;
+  nsresult rv = FolderCount(aParent, &folderCount);
+  NS_ENSURE_SUCCESS(rv, rv);
+  if (*aIndex == nsINavBookmarksService::DEFAULT_INDEX ||
+      *aIndex >= folderCount) {
+    index = folderCount;
   } else {
     index = *aIndex;
     rv = AdjustIndices(aParent, index, PR_INT32_MAX, 1);
@@ -1432,9 +1443,12 @@ nsNavBookmarks::InsertSeparator(PRInt64 aParent, PRInt32 aIndex,
   mozStorageTransaction transaction(mDBConn, PR_FALSE);
 
   PRInt32 index;
-  nsresult rv;
-  if (aIndex == nsINavBookmarksService::DEFAULT_INDEX) {
-    index = FolderCount(aParent);
+  PRInt32 folderCount;
+  nsresult rv = FolderCount(aParent, &folderCount);
+  NS_ENSURE_SUCCESS(rv, rv);
+  if (aIndex == nsINavBookmarksService::DEFAULT_INDEX ||
+      aIndex >= folderCount) {
+    index = folderCount;
   } else {
     index = aIndex;
     rv = AdjustIndices(aParent, index, PR_INT32_MAX, 1);
@@ -1963,8 +1977,12 @@ nsNavBookmarks::MoveItem(PRInt64 aItemId, PRInt64 aNewParent, PRInt32 aIndex)
 
   // calculate new index
   PRInt32 newIndex;
-  if (aIndex == nsINavBookmarksService::DEFAULT_INDEX) {
-    newIndex = FolderCount(aNewParent);
+  PRInt32 folderCount;
+  rv = FolderCount(aNewParent, &folderCount);
+  NS_ENSURE_SUCCESS(rv, rv);
+  if (aIndex == nsINavBookmarksService::DEFAULT_INDEX ||
+      aIndex >= folderCount) {
+    newIndex = folderCount;
     // If the parent remains the same, then the folder is really being moved
     // to count - 1 (since it's being removed from the old position)
     if (oldParent == aNewParent) {
@@ -2509,19 +2527,25 @@ nsNavBookmarks::QueryFolderChildren(PRInt64 aFolderId,
   return NS_OK;
 }
 
-PRInt32
-nsNavBookmarks::FolderCount(PRInt64 aFolder)
+nsresult
+nsNavBookmarks::FolderCount(PRInt64 aFolderId, PRInt32 *aFolderCount)
 {
+  *aFolderCount = 0;
   mozStorageStatementScoper scope(mDBFolderCount);
 
-  nsresult rv = mDBFolderCount->BindInt64Parameter(0, aFolder);
-  NS_ENSURE_SUCCESS(rv, 0);
+  nsresult rv = mDBFolderCount->BindInt64Parameter(0, aFolderId);
+  NS_ENSURE_SUCCESS(rv, rv);
 
   PRBool results;
   rv = mDBFolderCount->ExecuteStep(&results);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  return mDBFolderCount->AsInt32(0);
+  // Ensure that the folder we are looking for exists.
+  NS_ENSURE_TRUE(mDBFolderCount->AsInt64(1) == aFolderId, NS_ERROR_INVALID_ARG);
+
+  *aFolderCount = mDBFolderCount->AsInt32(0);
+
+  return NS_OK;
 }
 
 NS_IMETHODIMP
@@ -2784,6 +2808,7 @@ NS_IMETHODIMP
 nsNavBookmarks::SetItemIndex(PRInt64 aItemId, PRInt32 aNewIndex)
 {
   NS_ENSURE_ARG_MIN(aItemId, 1);
+  NS_ENSURE_ARG_MIN(aNewIndex, 0);
 
   nsresult rv;
   PRInt32 oldIndex = 0;
@@ -2804,6 +2829,12 @@ nsNavBookmarks::SetItemIndex(PRInt64 aItemId, PRInt32 aNewIndex)
     parent = mDBGetItemProperties->AsInt64(kGetItemPropertiesIndex_Parent);
   }
 
+  // Ensure we are not going out of range.
+  PRInt32 folderCount;
+  rv = FolderCount(parent, &folderCount);
+  NS_ENSURE_SUCCESS(rv, rv);
+  NS_ENSURE_TRUE(aNewIndex < folderCount, NS_ERROR_INVALID_ARG);
+
   mozStorageStatementScoper scoper(mDBSetItemIndex);
   rv = mDBSetItemIndex->BindInt64Parameter(0, aItemId);
   NS_ENSURE_SUCCESS(rv, rv);

toolkit/components/places/src/nsNavBookmarks.h

@@ -134,7 +134,18 @@ private:
   nsresult AdjustIndices(PRInt64 aFolder,
                          PRInt32 aStartIndex, PRInt32 aEndIndex,
                          PRInt32 aDelta);
-  PRInt32 FolderCount(PRInt64 aFolder);
+
+  /**
+   * Calculates number of children for the given folder.
+   *
+   * @param aFolderId Folder to count children for.
+   *
+   * @return aFolderCount The number of children in this folder.
+   *
+   * @throws If folder does not exist.
+   */
+  nsresult FolderCount(PRInt64 aFolderId, PRInt32 *aFolderCount);
+
   nsresult GetFolderType(PRInt64 aFolder, nsACString &aType);
 
   nsresult GetLastChildId(PRInt64 aFolder, PRInt64* aItemId);

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

@@ -0,0 +1,157 @@
+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
+ * vim: sw=2 ts=2 et lcs=trail\:.,tab\:>~ :
+ * ***** 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 395593 unit test code.
+ *
+ * The Initial Developer of the Original Code is
+ * Mozilla Foundation.
+ * Portions created by the Initial Developer are Copyright (C) 2009
+ * the Initial Developer. All Rights Reserved.
+ *
+ * Contributor(s):
+ *   Marco Bonardo <mak77@bonardo.net> (Original Author)
+ *
+ * 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 ***** */
+
+const NUM_BOOKMARKS = 20;
+const NUM_SEPARATORS = 5;
+const NUM_FOLDERS = 10;
+const NUM_ITEMS = NUM_BOOKMARKS + NUM_SEPARATORS + NUM_FOLDERS;
+const MIN_RAND = -5;
+const MAX_RAND = 40;
+
+var bs = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
+         getService(Ci.nsINavBookmarksService);
+
+function check_contiguous_indexes(aBookmarks) {
+  var indexes = [];
+  aBookmarks.forEach(function(aBookmarkId) {
+    let bmIndex = bs.getItemIndex(aBookmarkId);
+    dump("Index: " + bmIndex + "\n");
+    dump("Checking duplicates\n");
+    do_check_eq(indexes.indexOf(bmIndex), -1);
+    dump("Checking out of range, found " + aBookmarks.length + " items\n");
+    do_check_true(bmIndex >= 0 && bmIndex < aBookmarks.length);
+    indexes.push(bmIndex);
+  });
+  dump("Checking all valid indexes have been used\n");
+  do_check_eq(indexes.length, aBookmarks.length);
+}
+
+// main
+function run_test() {
+  var bookmarks = [];
+  // Insert bookmarks with random indexes.
+  for (let i = 0; bookmarks.length < NUM_BOOKMARKS; i++) {
+    let randIndex = Math.round(MIN_RAND + (Math.random() * (MAX_RAND - MIN_RAND)));
+    try {
+      let id = bs.insertBookmark(bs.unfiledBookmarksFolder,
+                                 uri("http://" + i + ".mozilla.org/"),
+                                 randIndex, "Test bookmark " + i);
+      if (randIndex < -1)
+        do_throw("Creating a bookmark at an invalid index should throw");
+      bookmarks.push(id);
+    }
+    catch (ex) {
+      if (randIndex >= -1)
+        do_throw("Creating a bookmark at a valid index should not throw");
+    }
+  }
+  check_contiguous_indexes(bookmarks);
+
+  // Insert separators with random indexes.
+  for (let i = 0; bookmarks.length < NUM_BOOKMARKS + NUM_SEPARATORS; i++) {
+    let randIndex = Math.round(MIN_RAND + (Math.random() * (MAX_RAND - MIN_RAND)));
+    try {
+      let id = bs.insertSeparator(bs.unfiledBookmarksFolder, randIndex);
+      if (randIndex < -1)
+        do_throw("Creating a separator at an invalid index should throw");
+      bookmarks.push(id);
+    }
+    catch (ex) {
+      if (randIndex >= -1)
+        do_throw("Creating a separator at a valid index should not throw");
+    }
+  }
+  check_contiguous_indexes(bookmarks);
+
+  // Insert folders with random indexes.
+  for (let i = 0; bookmarks.length < NUM_ITEMS; i++) {
+    let randIndex = Math.round(MIN_RAND + (Math.random() * (MAX_RAND - MIN_RAND)));
+    try {
+      let id = bs.createFolder(bs.unfiledBookmarksFolder,
+                               "Test folder " + i, randIndex);
+      if (randIndex < -1)
+        do_throw("Creating a folder at an invalid index should throw");
+      bookmarks.push(id);
+    }
+    catch (ex) {
+      if (randIndex >= -1)
+        do_throw("Creating a folder at a valid index should not throw");
+    }
+  }
+  check_contiguous_indexes(bookmarks);
+
+  // Execute some random bookmark delete.
+  for (let i = 0; i < Math.ceil(NUM_ITEMS / 4); i++) {
+    let id = bookmarks.splice(Math.floor(Math.random() * bookmarks.length), 1);
+    dump("Removing item with id " + id + "\n");
+    bs.removeItem(id);
+  }
+  check_contiguous_indexes(bookmarks);
+
+  // Execute some random bookmark move.  This will also try to move it to
+  // invalid index values.
+  for (let i = 0; i < Math.ceil(NUM_ITEMS / 4); i++) {
+    let randIndex = Math.floor(Math.random() * bookmarks.length);
+    let id = bookmarks[randIndex];
+    let newIndex = Math.round(MIN_RAND + (Math.random() * (MAX_RAND - MIN_RAND)));
+    dump("Moving item with id " + id + " to index " + newIndex + "\n");
+    try {
+      bs.moveItem(id, bs.unfiledBookmarksFolder, newIndex);
+      if (newIndex < -1)
+        do_throw("Moving an item to a negative index should throw\n");
+    }
+    catch (ex) {
+      if (newIndex >= -1)
+        do_throw("Moving an item to a valid index should not throw\n");
+    }
+    
+  }
+  check_contiguous_indexes(bookmarks);
+
+  // Ensure setItemIndex throws if we pass it a negative index.
+  try {
+    bs.setItemIndex(bookmarks[0], -1);
+    do_throw("setItemIndex should throw for a negative index");
+  } catch (ex) {}
+  // Ensure setItemIndex throws if we pass it a bad itemId.
+  try {
+    bs.setItemIndex(0, 5);
+    do_throw("setItemIndex should throw for a bad itemId");
+  } catch (ex) {}
+}

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

@@ -108,7 +108,7 @@ function test_removeItem()
 function test_removeFolder()
 {
   // First we add the item we are going to remove.
-  let id = bs.createFolder(bs.unfiledBookmarsFolder, "t", bs.DEFAULT_INDEX);
+  let id = bs.createFolder(bs.unfiledBookmarksFolder, "t", bs.DEFAULT_INDEX);
 
   // Add our observer, and remove it.
   let observer = new Observer(id);
@@ -123,7 +123,7 @@ function test_removeFolder()
 function test_removeFolderChildren()
 {
   // First we add the item we are going to remove.
-  let fid = bs.createFolder(bs.unfiledBookmarsFolder, "tf", bs.DEFAULT_INDEX);
+  let fid = bs.createFolder(bs.unfiledBookmarksFolder, "tf", bs.DEFAULT_INDEX);
   let id = bs.insertBookmark(fid, uri("http://mozilla.org"), bs.DEFAULT_INDEX,
                              "t");