Closed
Bug 398295
Opened 17 years ago
Closed 17 years ago
When I delete multiple items in the Bookmarks Organizer, dozens of assertions appear
Categories
(Firefox :: Bookmarks & History, defect, P1)
Firefox
Bookmarks & History
Tracking
()
VERIFIED
FIXED
Firefox 3 beta1
People
(Reporter: crazy-daniel, Assigned: dietrich)
References
Details
Attachments
(2 files, 1 obsolete file)
65.44 KB,
image/png
|
Details | |
3.74 KB,
patch
|
moco
:
review+
|
Details | Diff | Splinter Review |
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).
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•17 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•17 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•17 years ago
|
Assignee: nobody → dietrich
Flags: blocking-firefox3?
Priority: -- → P1
Target Milestone: --- → Firefox 3 M9
Updated•17 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Assignee | ||
Comment 4•17 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•17 years ago
|
||
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)
Comment 6•17 years ago
|
||
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•17 years ago
|
||
Comment on attachment 284692 [details] [diff] [review] fix r=sspitzer, but see comment #6
Attachment #284692 -
Flags: review?(sspitzer) → review+
Assignee | ||
Comment 8•17 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 | ||
Comment 10•17 years ago
|
||
fixed seth's comments
Attachment #284692 -
Attachment is obsolete: true
Attachment #284767 -
Flags: review?(sspitzer)
Comment 11•17 years ago
|
||
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•17 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.
Comment 13•17 years ago
|
||
> do you mean "that is not a folder, and is excluded"?
sorry, yes, I meant to type "is excluded".
Comment 14•17 years ago
|
||
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•17 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
Closed: 17 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 16•17 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•17 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•17 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
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Comment 19•17 years ago
|
||
Verified in Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007102404 Minefield/3.0a9pre.
Status: RESOLVED → VERIFIED
Comment 20•15 years ago
|
||
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.
Description
•