Closed Bug 428133 Opened 14 years ago Closed 14 years ago

removeItem and removeFolder accept ids of the wrong type

Categories

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

defect

Tracking

()

RESOLVED FIXED
Firefox 3

People

(Reporter: dietrich, Assigned: dietrich)

References

Details

(Keywords: addon-compat)

Attachments

(1 file, 2 obsolete files)

Attached patch fix (obsolete) — Splinter Review
removeItem() took folder ids, and removed as if it were an item (leaving orphan children). it should support folders, as we've done with "item" in all the other APIs.

removeFolder() did not complain when passed a non-folder item id, partially removing the item. it should return an error and take no action.


Drivers: This is a very low risk change, that fixes API bugs that could lead to database corruption. Tests are included.

changes:

- if a folder id is passed to removeItem(), dispatch the call to removeFolder()
- if a non-folder id is passed to removeFolder(), return error
- in both apis, do not remove annos until after the input param is validated
- in the tag service, fix a place where a string title could be passed to removeFolder()
- add a param check to IsBookmarked()
- rename some misleading vars
Flags: blocking-firefox3?
Attachment #314735 - Flags: review?(mano)
Assignee: nobody → dietrich
A third party lazy api user (me!) will most probably think that removeItem can remove a folder, and finish with a semi-corrupted DB with orphan items... And that is what happened today while working on another bug :\
actually, Marco found this today while working on internal code, so not even 3rd party :/
Priority: -- → P2
Target Milestone: --- → Firefox 3
a drive-by review
 
+  if (itemType == TYPE_FOLDER) {
+    rv = RemoveFolder(aItemId);
+    NS_ENSURE_SUCCESS(rv, rv);
+    return NS_OK;
+  }


should not be return RemoveFolder(aItemId) ?

 
 NS_IMETHODIMP
-nsNavBookmarks::RemoveFolder(PRInt64 aFolder)
+nsNavBookmarks::RemoveFolder(PRInt64 aFolderId)
 {
   mozIStorageConnection *dbConn = DBConn();
   mozStorageTransaction transaction(dbConn, PR_FALSE);
 
+  nsresult rv;
+
   // First, remove item annotations
   nsAnnotationService* annosvc = nsAnnotationService::GetAnnotationService();
   NS_ENSURE_TRUE(annosvc, NS_ERROR_OUT_OF_MEMORY);
-  nsresult rv = annosvc->RemoveItemAnnotations(aFolder);
+  rv = annosvc->RemoveItemAnnotations(aFolderId);
   NS_ENSURE_SUCCESS(rv, rv);
 

Why this movement of nsresult def?


Index: toolkit/components/places/tests/bookmarks/test_removeItem.js
===================================================================
RCS file: toolkit/components/places/tests/bookmarks/test_removeItem.js
+
+  // add a bookmark to the new folder
+  var bookmarkURI = uri("http://iasdjkf");
+  do_check_false(PlacesUtils.bookmarks.isBookmarked(bookmarkURI));


why this check, you have created the uri now, how can that be bookmarked?


+  // remove the folder using removeItem
+  PlacesUtils.bookmarks.removeItem(folderId);
+  do_check_eq(PlacesUtils.bookmarks.getBookmarkIdsForURI(bookmarkURI, {}).length, 0);
+  do_check_false(PlacesUtils.bookmarks.isBookmarked(bookmarkURI));
+}


imo you should add some child to the folder, and check that after removeItem all children have been removed... Removing a folder with RemoveItem was already working, the problem was that it was leaving orphans.
Flags: blocking-firefox3? → blocking-firefox3+
(In reply to comment #3)
> a drive-by review
> 
> +  if (itemType == TYPE_FOLDER) {
> +    rv = RemoveFolder(aItemId);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    return NS_OK;
> +  }
> 
> 
> should not be return RemoveFolder(aItemId) ?
> 

Failures in the underlying call can be obscured that way. We removed this pattern in a lot of places in nsNavHistoryExpire for this reason.

> +  nsresult rv;
> +
>    // First, remove item annotations
>    nsAnnotationService* annosvc = nsAnnotationService::GetAnnotationService();
>    NS_ENSURE_TRUE(annosvc, NS_ERROR_OUT_OF_MEMORY);
> -  nsresult rv = annosvc->RemoveItemAnnotations(aFolder);
> +  rv = annosvc->RemoveItemAnnotations(aFolderId);
>    NS_ENSURE_SUCCESS(rv, rv);
> 
> 
> Why this movement of nsresult def?

I meant to move the anno removal to after the type-check. Fixing in next patch.

> Index: toolkit/components/places/tests/bookmarks/test_removeItem.js
> ===================================================================
> RCS file: toolkit/components/places/tests/bookmarks/test_removeItem.js
> +
> +  // add a bookmark to the new folder
> +  var bookmarkURI = uri("http://iasdjkf");
> +  do_check_false(PlacesUtils.bookmarks.isBookmarked(bookmarkURI));
> 
> why this check, you have created the uri now, how can that be bookmarked?

if the bookmark was already in the db, it would invalidate the test, so really just a sanity check. could be removed since the db *should* be cleared before each test run.

> 
> 
> +  // remove the folder using removeItem
> +  PlacesUtils.bookmarks.removeItem(folderId);
> +  do_check_eq(PlacesUtils.bookmarks.getBookmarkIdsForURI(bookmarkURI,
> {}).length, 0);
> +  do_check_false(PlacesUtils.bookmarks.isBookmarked(bookmarkURI));
> +}
> 
> 
> imo you should add some child to the folder, and check that after removeItem
> all children have been removed... Removing a folder with RemoveItem was already
> working, the problem was that it was leaving orphans.
> 

that's exactly what i'm doing:

1. create a folder (folderId)
2. add a bookmark to it (bookmarkURI)
3. remove the folder via removeItem(folderId)
4. check for the existence of the child

if the bug was still there, both checks in the above test code would fail because the children of the deleted folder would still be present. i'll add another check that uses the bookmark id itself for more specificity.
Flags: blocking-firefox3+ → blocking-firefox3?
Flags: blocking-firefox3? → blocking-firefox3+
Attachment #314735 - Attachment is obsolete: true
Attachment #314895 - Flags: review?(mano)
Attachment #314735 - Flags: review?(mano)
Comment on attachment 314895 [details] [diff] [review]
fix v2 - marco comments addressed

Please note in the IDL that the correct method to use is removeItem, for all item types, and that removeFolder is deprecated and will be removed in the next release.

r=mano with that.
Attachment #314895 - Flags: review?(mano) → review+
Keywords: late-compat
Blocks: 428558
Attached patch for checkinSplinter Review
Attachment #314895 - Attachment is obsolete: true
Comment on attachment 314895 [details] [diff] [review]
fix v2 - marco comments addressed

drivers: fix for api sanity and db integrity
Attachment #314895 - Flags: approval1.9?
Comment on attachment 315152 [details] [diff] [review]
for checkin

a1.9=beltzner
Attachment #315152 - Flags: approval1.9+
Whiteboard: [has patch][has review]
Checking in toolkit/components/places/public/nsINavBookmarksService.idl;
/cvsroot/mozilla/toolkit/components/places/public/nsINavBookmarksService.idl,v  <--  nsINavBookmarksService.idl
new revision: 1.58; previous revision: 1.57
done
Checking in toolkit/components/places/src/nsNavBookmarks.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v  <--  nsNavBookmarks.cpp
new revision: 1.161; previous revision: 1.160
done
Checking in toolkit/components/places/src/nsTaggingService.js;
/cvsroot/mozilla/toolkit/components/places/src/nsTaggingService.js,v  <--  nsTaggingService.js
new revision: 1.25; previous revision: 1.24
done
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_removeItem.js,v
done
Checking in toolkit/components/places/tests/bookmarks/test_removeItem.js;
/cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_removeItem.js,v  <--  test_removeItem.js
initial revision: 1.1
done
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][has review]
verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090128 Shiretoko/3.1b3pre
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.