468307 - Multiple observer events fire for addition, removal

Status: RESOLVED WORKSFORME

(Firefox :: Bookmarks & History, defect)

Description

[Dan Mills [:thunder]]

I get multiple observer callbacks for each addition, removal.  I did not test changes, but it's possible they also trigger multiple callbacks, I'm not sure.

Additions trigger:
1) onItemAdded: Valid args
2) onItemChanged: all arguments except itemId are empty
3) onItemChanged: for the GUID.  My code is causing this one, since I need GUIDs on everything.

Removals trigger:
1) onItemChanged: all arguments except itemId are empty
2) onItemChanged: for the GUID annotation.  Note that it does not tell me the GUID is being removed, and querying for the GUID at this point will simply generate a new one on-the-fly.
3) onItemRemoved: Valid args

This makes it a little difficult to follow some changes, in particular GUID changes.

Comment 1

[Dan Mills [:thunder]]

I should probably add: sdwilsh and I saw some strange behavior when looking at this bug.  It seemed as if some of the callbacks were being interleaved... as if they were yielding and resuming.  Except they had no yields in them.

I'm not sure how to repro that crazy behavior, but I will try.

Comment 2

[Shawn Wilsher :sdwilsh]

(In reply to comment #0)
> Additions trigger:
> 1) onItemAdded: Valid args
> 2) onItemChanged: all arguments except itemId are empty
> 3) onItemChanged: for the GUID.  My code is causing this one, since I need
> GUIDs on everything.
The first onItemChanged should only be happening for tags, and you should have "tags" set as the property.
Can't help you with the GUID stuff, but you are getting the property as "placesInternal/GUID" right?

> Removals trigger:
> 1) onItemChanged: all arguments except itemId are empty
> 2) onItemChanged: for the GUID annotation.  Note that it does not tell me the
> GUID is being removed, and querying for the GUID at this point will simply
> generate a new one on-the-fly.
> 3) onItemRemoved: Valid args
Looking at the code I would expect the order to be different here (at least for bookmarks, didn't look into folders).  I'd expect it to be (2), (3), (1)
(1) should be happening for tags, and aProperty should be set to "tags".  Is this not happening?

Comment 3

[Shawn Wilsher :sdwilsh]

OK, after talking with thunder, we do some crazyness if a bookmark is a tag.  As far as I can tell, it's just to update our internal state for tag containers.

Looking for a workaround and a real fix.

Comment 4

[Shawn Wilsher :sdwilsh]

So, as far as I can tell, discarding any onItemChanged with aProperty = "tags" should be fine for weave.  I need to figure out if we can stop doing that now though...

Comment 5

[Shawn Wilsher :sdwilsh]

OK, so we notify so result containers can invalidate their tags attribute.  This is really inefficient though because we have to notify *all* observers about this.  I think we can do this simpler and easier!

I'm proposing one of two solutions be adopted:
1) Have the result listeners check if the grandparent folder is the tag folder when an item is added.
2) Make a special interface that the result nodes can register for that deals with this exact case.  It'll be native code only, and private so public consumers are tied up with our mess.  We can get rid of it when we fix tags without anybody really noticing.

Comment 6

[Shawn Wilsher :sdwilsh]

Disregard comment 5.  We can't do that, but we can not notify about the item we just added/removed, which we currently happily notify about.

Comment 7

[Shawn Wilsher :sdwilsh]

Oh neat!  nsAnnotationService::RemoveItemAnnotation always notifies, even if it doesn't remove anything!

Comment 8

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Fixed with tests!

Comment 9

[Dietrich Ayala (:dietrich)]

Comment on attachment 368558 [details] [diff] [review]
v1.0

>@@ -1132,16 +1132,19 @@ nsNavBookmarks::InsertBookmark(PRInt64 a
>     // query for all bookmarks for that URI, notify for each
>     nsTArray<PRInt64> bookmarks;
> 
>     rv = GetBookmarkIdsForURITArray(aItem, &bookmarks);
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>     if (bookmarks.Length()) {
>       for (PRUint32 i = 0; i < bookmarks.Length(); i++) {
>+        // We do not want to notify about the item we just added!
>+        if (bookmarks[i] == rowId)
>+          continue;
>         ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>                             OnItemChanged(bookmarks[i], NS_LITERAL_CSTRING("tags"),
>                                           PR_FALSE, EmptyCString()))

the code is pretty clear, so your comment is redundant. the comment would be more helpful if it provided a summarized explanation of *why* the code is doing what it's doing.

>@@ -1185,20 +1188,23 @@ nsNavBookmarks::RemoveItem(PRInt64 aItem
>     return NS_OK;
>   }
> 
>   ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
>                       OnBeforeItemRemoved(aItemId))
> 
>   mozStorageTransaction transaction(mDBConn, PR_FALSE);
> 
>-  // First, remove item annotations
>+  // First, remove item annotations, but suppress notifying the bookmark
>+  // observers about it.
>   nsAnnotationService* annosvc = nsAnnotationService::GetAnnotationService();
>   NS_ENSURE_TRUE(annosvc, NS_ERROR_OUT_OF_MEMORY);
>+  mSuppressedAnnotationNotificationsItemId = aItemId;
>   rv = annosvc->RemoveItemAnnotations(aItemId);
>+  mSuppressedAnnotationNotificationsItemId = 0;
>   NS_ENSURE_SUCCESS(rv, rv);

ditto

>@@ -3155,17 +3162,21 @@ nsNavBookmarks::OnItemAnnotationSet(PRIn
> NS_IMETHODIMP
> nsNavBookmarks::OnPageAnnotationRemoved(nsIURI* aPage, const nsACString& aName)
> {
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> nsNavBookmarks::OnItemAnnotationRemoved(PRInt64 aItemId, const nsACString& aName)
>-{  
>+{
>+  // Ignore this notification if we are supposed to.
>+  if (aItemId == mSuppressedAnnotationNotificationsItemId)
>+    return NS_OK;
>+

and, ditto.

>diff --git a/toolkit/components/places/src/nsNavBookmarks.h b/toolkit/components/places/src/nsNavBookmarks.h
>--- a/toolkit/components/places/src/nsNavBookmarks.h
>+++ b/toolkit/components/places/src/nsNavBookmarks.h
>@@ -240,16 +240,25 @@ private:
>   nsCOMPtr<mozIStorageStatement> mDBSetItemLastModified;
>   nsCOMPtr<mozIStorageStatement> mDBSetItemIndex;
> 
>   // keywords
>   nsCOMPtr<mozIStorageStatement> mDBGetKeywordForURI;
>   nsCOMPtr<mozIStorageStatement> mDBGetKeywordForBookmark;
>   nsCOMPtr<mozIStorageStatement> mDBGetURIForKeyword;
> 
>+
>+  /**
>+   * Used to track the item id to surpress annotation notifications for.  We do
>+   * this because it isn't useful to notify bookmark observers that an
>+   * annotation has changed for an item when we are in fact removing that item.
>+   * This should hold zero whenever we are not suppressing notifications.
>+   */
>+  PRInt64 mSuppressedAnnotationNotificationsItemId;
>+

s/surpress/suppress/

Comment 10

[Dietrich Ayala (:dietrich)]

Comment on attachment 368558 [details] [diff] [review]
v1.0

r- for semi-racy re-entrance:

bms->removeItem(aId) -> ann->removeItemAnnotations(aId) -> notifies anno observer, who calls bms->removeItem(aSomeOtherId)

thereby overwriting the "suppressed" id.

Comment 11

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

per irc discussion - stop notifying about annotations being removed with the bookmarks API.  There are annotation listeners for this that make more sense to use.

Comment 12

[Dietrich Ayala (:dietrich)]

Comment on attachment 369396 [details] [diff] [review]
v1.1

r=me on the condition that 1) all the unit tests pass locally (maybe there's a specific reason that this is done that we're not seeing) and that 2) the change is documented as a potential extension compat change, and added to changenotes for the release.

Comment 13

[Shawn Wilsher :sdwilsh]

(In reply to comment #12)
> (From update of attachment 369396 [details] [diff] [review])
> r=me on the condition that 1) all the unit tests pass locally (maybe there's a
> specific reason that this is done that we're not seeing) and that 2) the change
> is documented as a potential extension compat change, and added to changenotes
> for the release.
All unit tests don't pass (xpcshell ones do!), so I'll have to come up with a new test.  Grr.

Comment 14

[Shawn Wilsher :sdwilsh]

Attachment: v2.0

This fixes the FUEL bustage.  It also makes us not notify about added annotations and forces consumers to just use the annotation listeners for anything about annotations (tags are, of course, an exception).

I'm also failing in test_result_sort.js, for the section of "test live update".  This sets an annotation on an item, and expects the sort order to change.  I know that this means I need to update some set of containers in nsNavHistoryResult.cpp to also be an annotation service listener.  However, it's not clear to me which one(s) exactly.  I figure marco probably knows, since he's been looking into this area of code a lot lately.  Marco - any advice?

mfinkle - looking for your review on the FUEL changes.  I'll get review from dietrich on the places change when all places unit tests pass.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 371347 [details] [diff] [review]
v2.0

FUEL changes look good to me.

Comment 16

[Marco Bonardo [:mak]]

Comment on attachment 371347 [details] [diff] [review]
v2.0

>diff --git a/toolkit/components/places/tests/bookmarks/test_bookmarks.js b/toolkit/components/places/tests/bookmarks/test_bookmarks.js
>--- a/toolkit/components/places/tests/bookmarks/test_bookmarks.js
>+++ b/toolkit/components/places/tests/bookmarks/test_bookmarks.js
>-  
>-  // ensure that removing an item removes its annotations
>-  do_check_true(annosvc.itemHasAnnotation(newId3, "test-annotation"));
>-  bmsvc.removeItem(newId3);
>-  do_check_false(annosvc.itemHasAnnotation(newId3, "test-annotation"));

why this change? removing an item should still clean up its annotations, or we would end up with orphan annos.

your problem could be (i skipped some line):

2904 nsNavHistoryQueryResultNode::OnItemChanged(PRInt64 aItemId,
2905                                            const nsACString& aProperty,
2906                                            PRBool aIsAnnotationProperty,
2907                                            const nsACString& aValue)
2908 {
2913   if (mLiveUpdate == QUERYUPDATE_COMPLEX_WITH_BOOKMARKS)
2914     (void)Refresh();
2918   return nsNavHistoryResultNode::OnItemChanged(aItemId, aProperty,
2919                                                aIsAnnotationProperty,
2920                                                aValue);
2921 }

you should make query nodes be annotation listeners and check if the annotation they are filtering on is related to the annotation you set/unset.

actually calling onItemChanged causes a call to nsNavHistoryQueryResultNode::OnItemChanged that (apart tags case) calls nsNavHistoryResultNode::OnItemChanged, that updates lastModified then calls view's itemChanged, and finally:

3594   // DO NOT OPTIMIZE THIS TO CHECK aProperty
3595   // the sorting methods fall back to each other so we need to re-sort the
3596   // result even if it's not set to sort by the given property
3597   PRInt32 ourIndex = mParent->FindChild(this);
3598   mParent->EnsureItemPosition(ourIndex);

This code is no more called in your case. you could use the above annotation listener and if annotation change is related call base nsNavHistoryResultNode::OnItemChanged.

Btw, you don't call anymore onitemchanged but setting an anno changes lastModified, that means that if you result is sorted by lastModified it won't update too (you will need a live update test for this, looks like we don't have one)

Comment 17

[RNicoletto]

Is this bug related to bug 432706 ?

Comment 18

[Shawn Wilsher :sdwilsh]

This is smaller scoped, but yes.

Comment 19

[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 20

[Michael Kohler [:mkohler]]

This is a mass change. Every comment has "assigned-to-new" in it.

I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.

Comment 21

[Richard Newman [:rnewman]]

This has an r+ patch. Did it land? Is it still relevant?

(This is a dep check for Bug 554414.)

Comment 22

[(dormant account)]

This doesn't look like a [snappy] bug.

Comment 23

[Marco Bonardo [:mak]]

this should now be better, and likely doesn't apply to the current code.
New bugs should be filed about specific issues with notifications.