The methods on nsINavBookmarkService currently allow specifying an arbitrary index for insertion of a new item or folder. They should restrict the index to be at most 1 greater than the current highest index, to avoid holes in the bookmark table.

Patch adds checks for valid aIndex arguments in the relevant methods. Adds some checks to test_bookmarks.js to test the bug.

Comment on attachment 255129 [details] [diff] [review] patch and tests We should also validate the folder id, esp. if we're adding this check. I would recommend you to update this only once dietrich's work lands.

this probably only needs a dedicated unit test

don't think Joey is working on this, unassigning.

i have a patch for this with a test, also checking for folder validity, but i need to add more tests, will be ready soon.

With this approach any point getting the max valid index will also get an "almost-free" check of the parent folder. The test ensures basic operations that involve changing indexes. After this, the only API remaining point that allows setting bad indexes is setItemIndex, but with this patch it will ensure whoever uses it won't be able to go out of bounds, and will be scaried by the warning. Ideally this should go away in future, is not cool to give users power to set data at will on the db. Especially if we move to a preordered tree (i'm actually thinking for recovery purposes we could bring on both methods, using old "indexes" system as a backup to recalculate the tree values). We can also easily enable the preventive maintenance fix for broken indexes in a followup, it's just missing a test to be enabled.

notice that thanks to these checks i found a bug in test_onBeforeItemRemoved_observer.js where a typo is trying to put things into a not existing folder. this was completely undetected before.

Comment on attachment 395335 [details] [diff] [review] patch v1.0 >+ // Insert separators 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); s/separators/folders/ in the comment r=me, thanks!

with fixed comment http://hg.mozilla.org/mozilla-central/rev/d229dcefb12e

Comment on attachment 395335 [details] [diff] [review] patch v1.0 asking approval, this adds more input checks to bookmarks APIs avoiding to use bogus indexes. Has tests.

Comment on attachment 395335 [details] [diff] [review] patch v1.0 a192=beltzner

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h". In Thunderbird 3.0b, you do that as follows: Tools | Message Filters Make sure the correct account is selected. Click "New" Conditions: Body contains places-to-b-and-h Change the action to "Delete Message". Select "Manually Run" from the dropdown at the top. Click OK. Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter. Gerv