484026 - getItemGUID creates GUIDs for deleted items

Status: RESOLVED FIXED

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

Description

[Dan Mills [:thunder]]

getItemGUID does not throw when called for a deleted places id.  Instead, it creates a new GUID annotation (which will get expired later, since the item was deleted).

The way I found this out is by observing bookmarks notifications, and calling getItemGUID on onItemRemoved.  To my surprise, I would get new GUIDs that I hadn't seen before.  Upon investigation I found out that places is calling onItemRemoved *after* deleting the item, and thus exposing this bug.

Comment 2

[Saint Wesonga]

Attachment: Patch

Note that the attached testcase currently passes on trunk, but this is because it is the annotation service failing (getItemGUID shouldn't call the service in the first place) - taking out the try catch blocks gives:

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file c:/mozilla/trunk/mozilla-central/toolkit/components/places/src/nsAnnotationService.cpp, line 313

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file c:/mozilla/trunk/mozilla-central/toolkit/components/places/src/nsAnnotationService.cpp, line 524

TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavBookmarksService.getItemGUID]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: c:/mozilla/trunk/obj/_tests/xpcshell/test_places/bookmarks/test_484026.js :: run_test :: line 58"  data: no]

Comment 3

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

so, every time we call getItemGUID, we would go checking if the item exists, but this would slowdown us quite a bit, especially when saving nodes for backups or sync.

I think the main problem here is that annotations service doesn't make difference between an invalid argument and a not existing anno for a valid argument. since we already run a query in getItemAnnotation, we could make so that it will return NS_ERROR_INVALID_ARG if the item does not exist, NS_ERROR_NOT_AVAILABLE if the item exists but the annotation does not exist (should be doable through a LEFT JOIN).

Actually annotations service does not execute any kind of check before reading annos. Annotation service has also other flaws, like the fact itemHasAnno has practically the same cost has getItemAnno. we should spend some time on fixing this service.

The choice here is between fixing annoSvc so that it makes correct error checking, or fixing the caller, both ways have their associated perf penalty, but fixing the caller could end up saying "so why don't we just fix the callers of getItemGUID?"

Supposing the situation could happen for other callers, and we would have to fix all of them, i'd probably go for the annoSvc fix.

Dietrich, what do you think about this?

Comment 4

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

or, since setAnnotation happens quite less frequently than getAnnotation, if we want to avoid some perf loss, we could just fix the set case, that is what causes us to create orphan annos.

Comment 5

[Dietrich Ayala (:dietrich)]

(In reply to comment #4)
> or, since setAnnotation happens quite less frequently than getAnnotation, if we
> want to avoid some perf loss, we could just fix the set case, that is what
> causes us to create orphan annos.

fixing this is the key IMO. the rest can be in followup bugs. agreed that the anno svc needs love. Ubiquity moved off of it for perf reasons. would be good to connect w/ them and figure out what their particular use-cases were.

Comment 6

[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

Comment 7

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

bug 531151 is going to make setPageAnnotation and setItemAnnotation throw if the page or item don't exist, that should fix this issue too, but we will still need a unit test, and i'll review such part. Thanks for having a look into this.

Comment 8

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

Comment on attachment 412419 [details] [diff] [review]
Patch

>diff --git a/toolkit/components/places/src/nsNavBookmarks.cpp b/toolkit/components/places/src/nsNavBookmarks.cpp

these changes should not be needed ideally once setItemAnnotation is fixed in bug 531151 it should throw
So i'm just going to review the test, please ensure that bug is good in fixing this issue.

>diff --git a/toolkit/components/places/tests/bookmarks/test_484026.js b/toolkit/components/places/tests/bookmarks/test_484026.js

>+ * The Original Code is Bug 360134 (item guids) unit test code.

"is Places Unit Test Code."

>+ *
>+ * The Initial Developer of the Original Code is Mozilla Corporation.

"Mozilla Foundation"

>+ * Portions created by the Initial Developer are Copyright (C) 2007

"2009"

>+// Get bookmark service
>+try {
>+  var bmsvc = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
>+              getService(Ci.nsINavBookmarksService);
>+} catch(ex) {
>+  do_throw("Could not get nav-bookmarks-service\n");
>+}

remove the try catch and comment

>+
>+// main

useless comment, remove

>+function run_test() {
>+  var URI = uri("http://foo.tld.com/");

you use this just once, so directly put uri("") in insertBookmark call below

>+  var folderId = bmsvc.createFolder(bmsvc.placesRoot, "test folder",
>+                                    bmsvc.DEFAULT_INDEX);
>+  var bookmarkId = bmsvc.insertBookmark(folderId, URI, bmsvc.DEFAULT_INDEX, "a title");
>+  var separatorId = bmsvc.insertSeparator(folderId, bmsvc.DEFAULT_INDEX);
>+
>+  bmsvc.removeItem(separatorId);
>+  bmsvc.removeItem(bookmarkId);
>+  bmsvc.removeItem(folderId);

why do you need all of this? calling removeItem(folderId) will also remove all of its children before.

>+  

trailing spaces

>+  // getItemGUID should throw when called for a deleted places id

s/places id/item id/

>+  var thrown = false;
>+  try {
>+    bmsvc.getItemGUID(folderId); // should fail
>+  } catch (e) {
>+    do_check_eq(e.result, Cr.NS_ERROR_ILLEGAL_VALUE);
>+    thrown = true;
>+  }
>+  do_check_true(thrown);

you don't need a local var just
try {
  your_method
  do_throw(error)
} catch (e) {
  check e.result
}

if we correctly throw the do_throw() call will be skipped, otherwise it will throw and tell us we are bad guys.

and ditto for other checks

Comment 9

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

or better (since we don't want the catch to handle our throw)

try {
  your_method();
  print(error);
  do_check_true(false);
} catch (e) {
  check e.result
}

and now that i point this to you i noticed sometimes we do the wrong thing in tests catching errors :(

Comment 10

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

(In reply to comment #9)
> or better (since we don't want the catch to handle our throw)

i checked do_throw definition, it is printing error, calling quit, and then throw, so it's safe to do_throw with an empty catch. better, you can ignore comment 9.

Comment 11

[Saint Wesonga]

Attachment: Unit Test

Comment 12

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

Comment on attachment 418523 [details] [diff] [review]
Unit Test

>diff --git a/toolkit/components/places/tests/bookmarks/test_484026.js b/toolkit/components/places/tests/bookmarks/test_484026.js

>+function run_test() {
>+  var folderId = bmsvc.createFolder(bmsvc.placesRoot, "test folder",
>+                                    bmsvc.DEFAULT_INDEX);
>+  var bookmarkId = bmsvc.insertBookmark(folderId, uri("http://foo.tld.com/"), bmsvc.DEFAULT_INDEX, "a title");

please wrap at no more than 80 chars

>+  try {
>+    bmsvc.getItemGUID(folderId);
>+    do_throw("getItemGUID should throw when called for a deleted item id");
>+  } catch (e) {
>+    do_check_eq(e.result, Cr.NS_ERROR_ILLEGAL_VALUE);
>+  }
>+
>+  try {
>+    bmsvc.getItemGUID(bookmarkId);
>+    do_throw("getItemGUID should throw when called for a deleted item id");
>+  } catch (e) {
>+    do_check_eq(e.result, Cr.NS_ERROR_ILLEGAL_VALUE);
>+  }
>+
>+  try {
>+    bmsvc.getItemGUID(separatorId);
>+    do_throw("getItemGUID should throw when called for a deleted item id");
>+  } catch (e) {
>+    do_check_eq(e.result, Cr.NS_ERROR_ILLEGAL_VALUE);
>+  }

please define an helper function that will take an itemId as argument, instead of repeating the same code
and do something like [id1, id2, id3].forEach(yourHelperFunc);


I assume you ran this test against current trunk and it PASSes?

r=me with the above comments fixed and my question answered positively, feel free to ask a further review if you need it.

Thanks for bringing up the good work.

Comment 13

[Saint Wesonga]

Attachment: Unit Test

I have confirmed that this test passes on trunk.

Comment 14

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

Comment on attachment 418794 [details] [diff] [review]
Unit Test

>diff --git a/toolkit/components/places/tests/bookmarks/test_484026.js b/toolkit/components/places/tests/bookmarks/test_484026.js

>+function testThrowsOnDeletedItemId(itemId, index, array) {

there should be no need for index and array arguments in this case, just leave them out

nit: s/itemId/aItemId/ (we prefix input arguments with "a")

r=me, thanks.

Comment 15

[Saint Wesonga]

Attachment: Unit Test

Comment 16

[Nochum Sossonko [:Natch]]

Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/c706c1745d6e