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

VERIFIED FIXED in Firefox 3 beta1

Status

()

P1
normal
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: crazy-daniel, Assigned: dietrich)

Tracking

Trunk
Firefox 3 beta1
Points:
---
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

11 years ago
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).
(Reporter)

Comment 1

11 years ago
Created attachment 283244 [details]
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

11 years ago
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
Version: unspecified → Trunk

Comment 3

11 years ago
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.
(Assignee)

Updated

11 years ago
Assignee: nobody → dietrich
Flags: blocking-firefox3?
Priority: -- → P1
Target Milestone: --- → Firefox 3 M9
Flags: blocking-firefox3? → blocking-firefox3+
(Assignee)

Comment 4

11 years ago
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).
(Assignee)

Comment 5

11 years ago
Created attachment 284692 [details] [diff] [review]
fix

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

also piggybacking a fix for an invalid warning.
Attachment #284692 - Flags: review?(sspitzer)
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 on attachment 284692 [details] [diff] [review]
fix

r=sspitzer, but see comment #6
Attachment #284692 - Flags: review?(sspitzer) → review+
(Assignee)

Comment 8

11 years ago
> 
> 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.
(Assignee)

Updated

11 years ago
Duplicate of this bug: 399380
(Assignee)

Comment 10

11 years ago
Created attachment 284767 [details] [diff] [review]
fix v2

fixed seth's comments
Attachment #284692 - Attachment is obsolete: true
Attachment #284767 - Flags: review?(sspitzer)
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.
(Assignee)

Comment 12

11 years ago
(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.
> do you mean "that is not a folder, and is excluded"?

sorry, yes, I meant to type  "is excluded".
Comment on attachment 284767 [details] [diff] [review]
fix v2

r=sspitzer, after discussing with dietrich over irc.
Attachment #284767 - Flags: review?(sspitzer) → review+
(Assignee)

Comment 15

11 years ago
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
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 16

11 years ago
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
(Assignee)

Comment 17

11 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 18

11 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
Verified in Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007102404 Minefield/3.0a9pre.
Status: RESOLVED → VERIFIED
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.