326208 - API should not allow creation of "holes" in bookmark folders

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Brian Ryner (not reading)]

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.

Comment 1

[Joey Minta]

Attachment: patch and tests

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

Comment 2

[Asaf Romano (gone)]

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.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

this probably only needs a dedicated unit test

Comment 4

[Dietrich Ayala (:dietrich)]

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

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

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.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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 8

[Dietrich Ayala (:dietrich)]

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!

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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 11

[Mike Beltzner [:beltzner, not reading bugmail]]

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

a192=beltzner

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0d55731f3382

Comment 13

[Gervase Markham [:gerv]]

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