398295 - When I delete multiple items in the Bookmarks Organizer, dozens of assertions appear

Status: VERIFIED FIXED

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

Description

[Daniel.S]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007100205 Minefield/3.0a9pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007100205 Minefield/3.0a9pre

When I select two or more bookmark items in the Bookmarks Organizer and then delete the multiselection many (40 or more for sure) assertions are thrown.
Also some entries in the Error Console do appear.

I just notice that with my profile in use I cannot reliably reproduce this. However, creating a new Profile and multiselecting the four items in the "Mozilla Firefox" folder and deleting them seems to always cause this bug.

Reproducible: Sometimes

Steps to Reproduce:
1. Open the Bookmarks Organizer.
2. Select 2 or more bookmark items in a folder
3. Delete the items.

Actual Results:  
Before the items are finally deleted, many many assertions are thrown.


Expected Results:  
The bookmark items are simply deleted.


I don't have a regression range yet. And maybe I won't have the time to find one because I'm only online on weekends (and today because here's a holiday).

Comment 1

[Daniel.S]

Attachment: First 10 assertions

Here is a screenshot of the first ten assertions thrown.

Also there're many entries in the error console, but they all seem to be the same:
Error: [Exception... "'Component does not have requested interface' when calling method: [nsIInterfaceRequestor::getInterface]"  nsresult: "0x80004002 (NS_NOINTERFACE)"  location: "<unknown>"  data: no]

Comment 2

[Elmar Ludwig]

Yeah, this is bad. Confirming on Linux with a current nightly.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a9pre) Gecko/2007100204 Minefield/3.0a9pre

Comment 3

[Elmar Ludwig]

Steps to reproduce with an existing profile (backup your bookmarks first!):

1. Open Places Organizer
2. Load default bookmarks using "Import and Backup -> Restore -> Choose File"
   (the default set is in <firefox install dir>/defaults/profile/bookmarks.html)
3. Open "Mozilla Firefox" bookmarks folder
4. Select all 4 bookmarks under "Mozilla Firefox" and press Delete

The triggered assertions all complain about a "null node" and occur during redrawing the Places Organizer window (at least on Linux).

I see the exceptions in the error console mentioned in comment #1 only in a new profile when opening the Places Organizer (i.e. before deleting anything), so these may be unrelated.

Comment 4

[Dietrich Ayala (:dietrich)]

The problem occurs here:

http://mxr.mozilla.org/seamonkey/source/browser/components/places/content/treeView.js#673

The error text that kicks it all off:

WARNING: row count changed unexpectedly: 'mRowCount == rowCount', file /Users/dayala/moz/newplaces/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp, line 2750

And the interesting part is that this only occurs for the 3rd item removed (the 2nd item in visible order, as we delete them in reverse order so that the order is preserved for undo).

Comment 5

[Dietrich Ayala (:dietrich)]

Attachment: fix

we were saying invisible rows were valid visible rows in certain cases.

also piggybacking a fix for an invalid warning.

Comment 6

[(not reading, please use [e-mail] instead)]

r=sspitzer

two related questions:

1)  do we have to make a similar change to nsNavHistoryResult::OnItemChanged()?

2) 

a few lines below your change, we have:

   if ((node->IsURI() || node->IsSeparator()) && mOptions->ExcludeItems()) {
     // don't update items when we aren't displaying them, but we do need to
     // adjust everybody's bookmark indices to account for the removal

so we've found the node in mChildren, but we are excluding the items? 

How does that happen?

3)  if the answer to #1 is yes and the answer to #2 is that it can't happen, we might want to fix this code in ::OnItemChanged() as well:

      nsNavHistoryResultNode* node = folder->FindChildById(aItemId, &nodeIndex);
      if (node && !(folder->mOptions->ExcludeItems()) &&

Comment 7

[(not reading, please use [e-mail] instead)]

Comment on attachment 284692 [details] [diff] [review]
fix

r=sspitzer, but see comment #6

Comment 8

[Dietrich Ayala (:dietrich)]

> 
> so we've found the node in mChildren, but we are excluding the items? 
> 
> How does that happen?

good catch, i don't think it can happen. i'm going to remove that.

> 3)  if the answer to #1 is yes and the answer to #2 is that it can't happen, we
> might want to fix this code in ::OnItemChanged() as well:
> 
>       nsNavHistoryResultNode* node = folder->FindChildById(aItemId,
> &nodeIndex);
>       if (node && !(folder->mOptions->ExcludeItems()) &&
> 

Yep, we could remove the ExcludeItems() check there.

Followup patch coming to address these.

Comment 10

[Dietrich Ayala (:dietrich)]

Attachment: fix v2

fixed seth's comments

Comment 11

[(not reading, please use [e-mail] instead)]

dietrich, before the change for bug #371074, we had:

   if (!node->IsFolder() && mOptions->ExcludeItems()) {
     // don't update items when we aren't displaying them, but we do need to
     // adjust everybody's bookmark indices to account for the removal
     ReindexRange(aIndex, PR_INT32_MAX, -1);
     return NS_OK;
   }

and then we had:

  if ((node->IsURI() || node->IsSeparator()) && mOptions->ExcludeItems()) {

Is there some other type, that is not a folder and isn't excluded when we exclude items?  

If no, then r=sspitzer on your patch.  If yes, then we might still need that code.

Comment 12

[Dietrich Ayala (:dietrich)]

(In reply to comment #11)
> dietrich, before the change for bug #371074, we had:
> 
>    if (!node->IsFolder() && mOptions->ExcludeItems()) {
>      // don't update items when we aren't displaying them, but we do need to
>      // adjust everybody's bookmark indices to account for the removal
>      ReindexRange(aIndex, PR_INT32_MAX, -1);
>      return NS_OK;
>    }
> 
> and then we had:
> 
>   if ((node->IsURI() || node->IsSeparator()) && mOptions->ExcludeItems()) {
> 
> Is there some other type, that is not a folder and isn't excluded when we
> exclude items?  
> 
> If no, then r=sspitzer on your patch.  If yes, then we might still need that
> code.
> 

do you mean "that is not a folder, and is excluded"?
                                       ^^
saved searches are not folders, and are not excluded. therefore we don't want the early return. we want to remove them from an excludeItems=true query when they are deleted.

Comment 13

[(not reading, please use [e-mail] instead)]

> do you mean "that is not a folder, and is excluded"?

sorry, yes, I meant to type  "is excluded".

Comment 14

[(not reading, please use [e-mail] instead)]

Comment on attachment 284767 [details] [diff] [review]
fix v2

r=sspitzer, after discussing with dietrich over irc.

Comment 15

[Dietrich Ayala (:dietrich)]

Checking in browser/components/places/content/treeView.js;
/cvsroot/mozilla/browser/components/places/content/treeView.js,v  <--  treeView.js
new revision: 1.18; previous revision: 1.17
done
Checking in toolkit/components/places/src/nsNavHistoryResult.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v  <--  nsNavHistoryResult.cpp
new revision: 1.117; previous revision: 1.116
done

Comment 16

[Dietrich Ayala (:dietrich)]

Backing out the ride-along change, to see if it's the cause of the Ts regression in bug 399807.

Checking in toolkit/components/places/src/nsNavHistoryResult.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v  <--  nsNavHistoryResult.cpp
new revision: 1.118; previous revision: 1.117
done

Comment 17

[Dietrich Ayala (:dietrich)]

No change in Tp (not Ts, as mentioned in previous comment), so backing out the treeView.js change as well.

Checking in browser/components/places/content/treeView.js;
/cvsroot/mozilla/browser/components/places/content/treeView.js,v  <--  treeView.js
new revision: 1.19; previous revision: 1.18
done

Comment 18

[Dietrich Ayala (:dietrich)]

relanded the js change:

Checking in browser/components/places/content/treeView.js;
/cvsroot/mozilla/browser/components/places/content/treeView.js,v  <--  treeView.js
new revision: 1.20; previous revision: 1.19
done

spinning off the toolkit change to bug 399866.

Comment 19

[Al Billings [:abillings - ex-MoCo]]

Verified in Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007102404 Minefield/3.0a9pre.

Comment 20

[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