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

RESOLVED FIXED in Firefox 3.7a1

Status

()

Firefox
Bookmarks & History
P2
normal
RESOLVED FIXED
12 years ago
8 years ago

People

(Reporter: Brian Ryner (not reading), Assigned: mak)

Tracking

Trunk
Firefox 3.7a1
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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.
Priority: -- → P2
Target Milestone: Firefox 2 alpha2 → Firefox 2 beta1
(Reporter)

Updated

11 years ago
Assignee: bryner → nobody
Target Milestone: Firefox 2 beta1 → ---
Version: unspecified → Trunk

Comment 1

10 years ago
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.
Assignee: nobody → jminta
Status: NEW → ASSIGNED
Attachment #255129 - Flags: review?(mano)
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.
Attachment #255129 - Flags: review?(mano) → review-
(Assignee)

Updated

9 years ago
Whiteboard: [tests needed][good first bug]
(Assignee)

Comment 3

9 years ago
this probably only needs a dedicated unit test
don't think Joey is working on this, unassigning.
Assignee: jminta → nobody
(Assignee)

Updated

8 years ago
Status: ASSIGNED → NEW
(Assignee)

Updated

8 years ago
Blocks: 509868
(Assignee)

Comment 5

8 years ago
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.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
(Assignee)

Updated

8 years ago
Whiteboard: [tests needed][good first bug]
(Assignee)

Comment 6

8 years ago
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.
Attachment #255129 - Attachment is obsolete: true
Attachment #395335 - Flags: review?(dietrich)
(Assignee)

Comment 7

8 years ago
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!
Attachment #395335 - Flags: review?(dietrich) → review+
(Assignee)

Comment 9

8 years ago
with fixed comment
http://hg.mozilla.org/mozilla-central/rev/d229dcefb12e
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a1
(Assignee)

Updated

8 years ago
Flags: in-testsuite+
(Assignee)

Comment 10

8 years ago
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.
Attachment #395335 - Flags: approval1.9.2?
Comment on attachment 395335 [details] [diff] [review]
patch v1.0

a192=beltzner
Attachment #395335 - Flags: approval1.9.2? → approval1.9.2+
(Assignee)

Comment 12

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0d55731f3382
status1.9.2: --- → beta1-fixed
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.