Last Comment Bug 326208 - API should not allow creation of "holes" in bookmark folders
: API should not allow creation of "holes" in bookmark folders
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
P2 normal (vote)
: Firefox 3.7a1
Assigned To: Marco Bonardo [::mak]
: Marco Bonardo [::mak]
Depends on:
Blocks: 509868
  Show dependency treegraph
Reported: 2006-02-06 23:43 PST by Brian Ryner (not reading)
Modified: 2009-11-26 05:52 PST (History)
7 users (show)
mak77: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch and tests (7.24 KB, patch)
2007-02-14 12:01 PST, Joey Minta
asaf: review-
Details | Diff | Splinter Review
patch v1.0 (17.24 KB, patch)
2009-08-19 10:15 PDT, Marco Bonardo [::mak]
dietrich: review+
mbeltzner: approval1.9.2+
Details | Diff | Splinter Review

Description User image Brian Ryner (not reading) 2006-02-06 23:43:11 PST
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 User image Joey Minta 2007-02-14 12:01:58 PST
Created attachment 255129 [details] [diff] [review]
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 User image Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-02-14 13:19:28 PST
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 User image Marco Bonardo [::mak] 2009-01-16 06:33:37 PST
this probably only needs a dedicated unit test
Comment 4 User image Dietrich Ayala (:dietrich) 2009-02-10 14:35:53 PST
don't think Joey is working on this, unassigning.
Comment 5 User image Marco Bonardo [::mak] 2009-08-19 05:49:50 PDT
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 User image Marco Bonardo [::mak] 2009-08-19 10:15:52 PDT
Created attachment 395335 [details] [diff] [review]
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 User image Marco Bonardo [::mak] 2009-08-19 10:17:43 PDT
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 User image Dietrich Ayala (:dietrich) 2009-08-20 10:35:35 PDT
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 User image Marco Bonardo [::mak] 2009-08-21 03:14:46 PDT
with fixed comment
Comment 10 User image Marco Bonardo [::mak] 2009-09-14 05:49:27 PDT
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 User image Mike Beltzner [:beltzner, not reading bugmail] 2009-09-21 09:46:27 PDT
Comment on attachment 395335 [details] [diff] [review]
patch v1.0

Comment 12 User image Marco Bonardo [::mak] 2009-09-22 03:07:38 PDT
Comment 13 User image Gervase Markham [:gerv] 2009-11-26 05:52:17 PST
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.


Note You need to log in before you can comment on or make changes to this bug.