428133 - removeItem and removeFolder accept ids of the wrong type

Status: RESOLVED FIXED

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

Description

[Dietrich Ayala (:dietrich)]

Attachment: fix

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

Comment 1

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

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 :\

Comment 2

[Dietrich Ayala (:dietrich)]

actually, Marco found this today while working on internal code, so not even 3rd party :/

Comment 3

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

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.

Comment 4

[Dietrich Ayala (:dietrich)]

(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.

Comment 5

[Dietrich Ayala (:dietrich)]

Attachment: fix v2 - marco comments addressed

Comment 6

[Asaf Romano (gone)]

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.

Comment 7

[Dietrich Ayala (:dietrich)]

Attachment: for checkin

Comment 8

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

Comment on attachment 314895 [details] [diff] [review]
fix v2 - marco comments addressed

drivers: fix for api sanity and db integrity

Comment 9

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

Comment on attachment 315152 [details] [diff] [review]
for checkin

a1.9=beltzner

Comment 10

[Dietrich Ayala (:dietrich)]

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

Comment 11

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090128 Shiretoko/3.1b3pre

Comment 12

[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