Build ID: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a6pre) Gecko/20070606 Minefield/3.0a6pre Summary: Crash [@ nsCOMArray_base::InsertObjectAt nsNavHistoryFolderResultNode::OnItemChanged] deleting "ghost" bookmark Steps to Reproduce (sorry, hard to repro; maybe someone can sprinkle null checks?) 1. In the Bookmarks Manager, I had a bookmark for http://ambientpersona.deviantart.com/, among 5 or so others of varied sites 2. I was testing quicksearch/typedown, and typed "a" 3. I right-clicked on its entry (once a few others were successfully filtered), and chose "Delete" 4. I then clicked the red close (X) widget in the quicksearch/typedown textfield, which restored the view 5. Oddly enough, the already-deleted "ambientpersona.deviantart.com" bookmark re-appeared, so I tried deleting it again 6. I immediately crashed, here: 0x84caf000 nsCOMArray_base::InsertObjectAt [mozilla/xpcom/build/nscomarray.cpp, line 89] nsNavHistoryFolderResultNode::OnItemChanged [mozilla/toolkit/components/places/src/nsnavhistoryresult.cpp, line 3210] nsNavHistoryResult::OnItemChanged [mozilla/toolkit/components/places/src/nsnavhistoryresult.cpp, line 3741] nsNavBookmarks::OnDeleteURI [mozilla/toolkit/components/places/src/nsnavbookmarks.cpp, line 2481] nsNavHistory::RemovePage [mozilla/toolkit/components/places/src/nsnavhistory.cpp, line 2465] NS_InvokeByIndex_P [mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp, line 102] XPCWrappedNative::CallMethod [mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2229]

i was not able to reproduce the crash on mac. i was able to easily reproduce the fact that deleting from search results in the organizer is not a persistent delete. only deletes from the view.

I'm not getting the crash, but I cat reproduce the problem that dietrich reproduced. additionally, after right-click-delete, I have seen two items become selected. Investigating.

there are two parts of this bug: 1) remove() and _removeRange() in controller.js need to be made to work with bookmark queries. I've got a patch for that which I'll attach. with that, we properly remove the selected item from bookmarks, but in a queried view, the item remains (but it won't from other views, as it has really been deleted.) 2) for bookmark queries, we are not getting notified when items are removed (and I'll bet on items being added and modified as well) in nsNavHistoryQueryResultNode, we register an everything observer, but I think for bookmark queries, we need register a bookmark observer. there appears to be some existing code nsNavHistoryQueryResultNode::OnItemRemoved(), which should call Refresh() which would invalidate the container. [There might be a spin off bug about restoring selection after invalidating a container] I think this bug will get morphed into: for bookmark queries (in the sidebar and bm organizer) we don't get notified on updates.

This bug is really about changes (add, remove, title modification, etc) made to bookmarks show up in views that are queries (bookmarks sidebar with a quick search, bookmarks organizer with a quick search) the "everything observer" is misnamed, and could be renamed "history observer" for clarity. I'll elaborate on the fix and clean up the comments before seeking review.

great bug, stephen. morphing summary. new patch coming that: 1) renames the observers to to make the code more understandable 2) puts some comments, assertions, and code for when we support QUERY_TYPE_UNIFIED 3) for the "all bookmarks" observers, pass the begin / end batch notifications, otherwise we are super slow on things like reloading live bookmarks when there is a bookmark query open.

Comment on attachment 268879 [details] [diff] [review] patch, updating after mano landed his fix for bug #384623 > > NS_IMETHODIMP > nsNavHistoryQueryResultNode::OnItemVisited(PRInt64 aItemId, > PRInt64 aVisitId, PRTime aTime) > { >- NS_NOTREACHED("Everything observers should not get OnItemVisited, but should get OnVisit instead"); >+ // for bookmark queires, "all bookmark" observer should get OnItemVisited nit: s/queires/queries/ >@@ -2910,21 +2936,21 @@ nsresult > nsNavHistoryFolderResultNode::Refresh() > { > if (! mExpanded) { > // when we are not expanded, we don't update, just invalidate and unhook > ClearChildren(PR_TRUE); > return NS_OK; // no updates in tree state > } > >+ ClearChildren(PR_TRUE); nit: as ClearChildren(PR_TRUE) is called either way, you could put it before (and move it out of) the preceding |if| block. >@@ -3733,19 +3801,20 @@ nsNavHistoryResult::OnItemChanged(PRInt6 > nsresult rv; > nsNavBookmarks* bookmarkService = nsNavBookmarks::GetBookmarksService(); > NS_ENSURE_TRUE(bookmarkService, NS_ERROR_OUT_OF_MEMORY); > > // find the folder to notify about this item > PRInt64 folderId; > rv = bookmarkService->GetFolderIdForItem(aItemId, &folderId); > NS_ENSURE_SUCCESS(rv, rv); >- ENUMERATE_BOOKMARK_OBSERVERS_FOR_FOLDER(folderId, >+ ENUMERATE_BOOKMARK_FOLDER_OBSERVERS(folderId, >+ OnItemChanged(aItemId, aProperty, aIsAnnotationProperty, aValue)); >+ ENUMERATE_ALL_BOOKMARKS_OBSERVERS( > OnItemChanged(aItemId, aProperty, aIsAnnotationProperty, aValue)); >- hrm, i don't think this patch has Mano's changes from bug 384623. r=me, given it plays nice with the recent changes in that code.

> nit: s/queires/queries/ fixed, thanks, > nit: as ClearChildren(PR_TRUE) is called either way, you could put it before > (and move it out of) the preceding |if| block. fixed, thanks. > hrm, i don't think this patch has Mano's changes from bug 384623. sorry, I re-attached the same patch, and not the update one. the last patch includes fixes for your comments as well as the changes needed post-mano's-fix for bug #384623

fixed cvs ci toolkit/components/places/src/nsNavHistory.cpp toolkit/components/places /src/nsNavHistoryResult.cpp toolkit/components/places/src/nsNavHistoryResult.h b rowser/components/places/content/controller.js Checking in toolkit/components/places/src/nsNavHistory.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v <-- nsNavHis tory.cpp new revision: 1.132; previous revision: 1.131 done Checking in toolkit/components/places/src/nsNavHistoryResult.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v <-- ns NavHistoryResult.cpp new revision: 1.103; previous revision: 1.102 done Checking in toolkit/components/places/src/nsNavHistoryResult.h; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.h,v <-- nsNa vHistoryResult.h new revision: 1.42; previous revision: 1.41 done Checking in browser/components/places/content/controller.js; /cvsroot/mozilla/browser/components/places/content/controller.js,v <-- control ler.js new revision: 1.162; previous revision: 1.161 done

Comment on attachment 268924 [details] [diff] [review] as checked in carrying over r=dietrich

it should be possible to write a test in our javascript test suite where we have a search query and additions, removals, changes to those bookmarks should call now call our nsINavBookmarkObserver (where it wouldn't before) adding in-testsuite? to track the writing of that test.

I functionally tested based on my steps in comment 0, as well as ad-hoc tested around (using Undo/Redo, more positions, etc.) and neither see ghost entries nor my earlier crash. I'll be sure to file any potential fallout or unrelated issues in separate bugs. I used: firefox-3.0a6pre.en-US.win32.installer.exe 6081 KB 6/20/2007 5:02:00 AM from: ftp://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/fx-win32-tbox-trunk to test. Verified FIXED.

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