history sidebar doesn't show the same results as fx 2, duplicate uris when grouped by site, last visited most visited I'm splitting out the fix for this from bug #385245, which will just track the optimization. fix in hand.

> includes fix for bug #381898 note to dietrich, that is the nsNavHistory.cpp part of this fix, but we need it for this bug. now, for the history sidebar (when last visited mode) we do what fx 2 does, and get results by uri (like we do for the history menu, but there we limit to the at most 10 results).

note, but switching to having results as uris, we can optimize in treeView.js to avoid calling collapseDuplicates() (because there are no duplicates). see bug #385245 going forward, for the views where can't do results as uris (and we must do results as visits), I think we should also collapse duplicates in C++ in the result (and not in treeView.js). that should help performance and fix bugs like #386399 ("History folder children often duplicated, resulting in bug-like behavior elsewhere")

Comment on attachment 274111 [details] [diff] [review] patch, includes fix for bug #381898 more is needed here, as with this patch I delete on crash now.

here's the stack to the crash, working on a fix: I crash in OnDeleteURI() because the nodes's mParent is null. xpcom_core.dll!nsVoidArray::IndexOf(void * aPossibleElement=0x06ffcd28) Line 381 + 0x3 bytes C++ xpcom_core.dll!nsCOMArray_base::IndexOf(nsISupports * aObject=0x06ffcd28) Line 60 C++ places.dll!nsCOMArray<nsNavHistoryResultNode>::IndexOf(nsNavHistoryResultNode * aObject=0x06ffcd28) Line 178 C++ places.dll!nsNavHistoryContainerResultNode::FindChild(nsNavHistoryResultNode * aNode=0x06ffcd28) Line 623 + 0x16 bytes C++ > places.dll!nsNavHistoryQueryResultNode::OnDeleteURI(nsIURI * aURI=0x0643d000) Line 2479 + 0xc bytes C++ places.dll!nsNavHistoryResult::OnDeleteURI(nsIURI * aURI=0x0643d000) Line 3927 + 0x66 bytes C++ places.dll!nsNavHistory::RemovePage(nsIURI * aURI=0x0643d000) Line 2581 + 0x7b bytes C++ xpcom_core.dll!NS_InvokeByIndex_P(nsISupports * that=0x00000009, unsigned int methodIndex=1, unsigned int paramCount=1235096, nsXPTCVariant * params=0x0653fa58) Line 102 C++ xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=9) Line 2277 + 0x1e bytes C++ xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=CALL_METHOD) Line 2277 + 0x1e bytes C++ xpc3250.dll!XPC_WN_CallMethod(JSContext * cx=0x06501730, JSObject * obj=0x06f55500, unsigned int argc=1, long * argv=0x0704ea78, long * vp=0x0012db34) Line 1467 + 0xe bytes C++ js3250.dll!js_Invoke(JSContext * cx=0x06501730, unsigned int argc=1, unsigned int flags=0) Line 1312 + 0x20 bytes C js3250.dll!js_Interpret(JSContext * cx=0x06501730, unsigned char * pc=0x045a6241, long * result=0x0012e1f0) Line 4024 + 0xf bytes C js3250.dll!js_Invoke(JSContext * cx=0x06501730, unsigned int argc=1, unsigned int flags=2) Line 1331 + 0x13 bytes C xpc3250.dll!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS * wrapper=0x06f5fa68, unsigned short methodIndex=5, const XPTMethodDescriptor * info=0x0422ea78, nsXPTCMiniVariant * nativeParams=0x0012e554) Line 1457 + 0x14 bytes C++ xpc3250.dll!nsXPCWrappedJS::CallMethod(unsigned short methodIndex=5, const XPTMethodDescriptor * info=0x0422ea78, nsXPTCMiniVariant * params=0x0012e554) Line 566 C++ xpcom_core.dll!PrepareAndDispatch(nsXPTCStubBase * self=0x06f5f880, unsigned int methodIndex=5, unsigned int * args=0x0012e614, unsigned int * stackBytesToPop=0x0012e604) Line 114 + 0x21 bytes C++ xpcom_core.dll!SharedStub() Line 142 C++ xpcom_core.dll!NS_InvokeByIndex_P(nsISupports * that=0x00000005, unsigned int methodIndex=1, unsigned int paramCount=1238920, nsXPTCVariant * params=0x0012e640) Line 102 C++ xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=77192968) Line 2277 + 0x1e bytes C++ xpcom_core.dll!NS_InvokeByIndex_P(nsISupports * that=0x00000005, unsigned int methodIndex=1, unsigned int paramCount=1238920, nsXPTCVariant * params=0x0012e640) Line 102 C++ xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=5) Line 2277 + 0x1e bytes C++ xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=CALL_METHOD) Line 2277 + 0x1e bytes C++ xpc3250.dll!XPC_WN_CallMethod(JSContext * cx=0x06501730, JSObject * obj=0x03e036a0, unsigned int argc=1, long * argv=0x0704e914, long * vp=0x0012ea24) Line 1467 + 0xe bytes C++ js3250.dll!js_Invoke(JSContext * cx=0x06501730, unsigned int argc=1, unsigned int flags=0) Line 1312 + 0x20 bytes C js3250.dll!js_Interpret(JSContext * cx=0x06501730, unsigned char * pc=0x044d1755, long * result=0x0012f0e0) Line 4024 + 0xf bytes C js3250.dll!js_Invoke(JSContext * cx=0x06501730, unsigned int argc=1, unsigned int flags=2) Line 1331 + 0x13 bytes C js3250.dll!js_InternalInvoke(JSContext * cx=0x06501730, JSObject * obj=0x00f63f80, long fval=77279744, unsigned int flags=0, unsigned int argc=1, long * argv=0x0704e860, long * rval=0x0012f268) Line 1406 + 0x14 bytes C js3250.dll!JS_CallFunctionValue(JSContext * cx=0x06501730, JSObject * obj=0x00f63f80, long fval=77279744, unsigned int argc=1, long * argv=0x0704e860, long * rval=0x0012f268) Line 4846 + 0x1f bytes C gklayout.dll!nsJSContext::CallEventHandler(nsISupports * aTarget=0x0648fc28, void * aScope=0x0655ad00, void * aHandler=0x049b3200, nsIArray * aargv=0x064bbbe8, nsIVariant * * arv=0x0012f3d4) Line 1851 + 0x24 bytes C++ gklayout.dll!nsJSEventListener::HandleEvent(nsIDOMEvent * aEvent=0x044f07a4) Line 215 + 0x67 bytes C++ gklayout.dll!nsEventListenerManager::HandleEventSubType(nsListenerStruct * aListenerStruct=0x0648fd28, nsIDOMEventListener * aListener=0x0648fcd8, nsIDOMEvent * aDOMEvent=0x044f07a4, nsISupports * aCurrentTarget=0x0648fc28, unsigned int aPhaseFlags=6) Line 1096 + 0x12 bytes C++ gklayout.dll!nsEventListenerManager::HandleEvent(nsPresContext * aPresContext=0x0650c5e8, nsEvent * aEvent=0x0012f744, nsIDOMEvent * * aDOMEvent=0x0012f6a4, nsISupports * aCurrentTarget=0x0648fc28, unsigned int aFlags=6, nsEventStatus * aEventStatus=0x0012f6a8) Line 1216 C++ gklayout.dll!nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=6) Line 202 C++ gklayout.dll!nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=6, nsDispatchingCallback * aCallback=0x00000000) Line 260 C++ gklayout.dll!nsEventDispatcher::Dispatch(nsISupports * aTarget=0x0648fc28, nsPresContext * aPresContext=0x0650c5e8, nsEvent * aEvent=0x0012f744, nsIDOMEvent * aDOMEvent=0x00000000, nsEventStatus * aEventStatus=0x0012f78c, nsDispatchingCallback * aCallback=0x00000000) Line 473 + 0x12 bytes C++ gklayout.dll!nsXULElement::PreHandleEvent(nsEventChainPreVisitor & aVisitor={...}) Line 1510 + 0x29 bytes C++ gklayout.dll!nsEventTargetChainItem::PreHandleEvent(nsEventChainPreVisitor & aVisitor={...}) Line 184 + 0x1c bytes C++ gklayout.dll!nsEventDispatcher::Dispatch(nsISupports * aTarget=0x06ee3a28, nsPresContext * aPresContext=0x0650c5e8, nsEvent * aEvent=0x0012f958, nsIDOMEvent * aDOMEvent=0x00000000, nsEventStatus * aEventStatus=0x0012f94c, nsDispatchingCallback * aCallback=0x00000000) Line 432 C++ gklayout.dll!PresShell::HandleDOMEventWithTarget(nsIContent * aTargetContent=0x06ee3a28, nsEvent * aEvent=0x0012f958, nsEventStatus * aStatus=0x0012f94c) Line 5777 + 0x1c bytes C++ gklayout.dll!nsXULMenuCommandEvent::Run() Line 1630 C++ xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0012fa04) Line 491 C++ xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x00bcc298, int mayWait=1) Line 227 + 0x16 bytes C++ gkwidget.dll!nsBaseAppShell::Run() Line 154 + 0xc bytes C++ tkitcmps.dll!nsAppStartup::Run() Line 170 + 0x1c bytes C++ xul.dll!XRE_main(int argc=1, char * * argv=0x00bc8aa8, const nsXREAppData * aAppData=0x00bc8e88) Line 3057 + 0x25 bytes C++ firefox.exe!main(int argc=1, char * * argv=0x00bc8aa8) Line 153 + 0x12 bytes C++ firefox.exe!__tmainCRTStartup() Line 586 + 0x19 bytes C firefox.exe!mainCRTStartup() Line 403 C kernel32.dll!7c816fd7() [Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]

timeless, you'd only crash if you had my patch (which is not checked in.)

ah, whoops

I have not able to make it crash again, but I did turn the assert into a NS_ENSURE_TRUE(parent, NS_ERROR_UNEXPECTED); to help track down / prevent the crash. Note, this fix will allow for some performance improvements, see comment #3.

this patch includes a problem that dietrich spotted which is: 1) order by last visited (or most visited) 2) click on a link in the history sidebar, where the visit should change the order expected results: the selected item in the history sidebar would change positions actual results: the item would not change positions the problem is that our nsNavHistoryContainerResultNode (for place:sort=4, for example) doesn't have a parent, so we can't use ReplaceChildURIAt() because it uses ReverseUpdateStats() which requires a parent.

Comment on attachment 274877 [details] [diff] [review] patch >+ options.queryType = NHQO.QUERY_TYPE_HISTORY; i think this is default >+ options.resultType = resultType; >+ >+ query.onlyBookmarked = false; i think this is default as well. >@@ -2469,18 +2479,19 @@ nsNavHistoryQueryResultNode::OnDeleteURI > nsCOMArray<nsNavHistoryResultNode> matches; > RecursiveFindURIs(onlyOneEntry, this, spec, &matches); > if (matches.Count() == 0) > return NS_OK; // not found > > for (PRInt32 i = 0; i < matches.Count(); i ++) { > nsNavHistoryResultNode* node = matches[i]; > nsNavHistoryContainerResultNode* parent = node->mParent; >- NS_ASSERTION(parent, "URI nodes should always have parents"); >- >+ // URI nodes should always have parents >+ NS_ENSURE_TRUE(parent, NS_ERROR_UNEXPECTED); >+ > PRInt32 childIndex = parent->FindChild(node); > NS_ASSERTION(childIndex >= 0, "Child not found in parent"); > parent->RemoveChildAt(childIndex); > > if (parent->mChildren.Count() == 0 && parent->IsQuerySubcontainer()) { > // when query subcontainers (like hosts) get empty we should remove them > // as well. Just append this to our list and it will get evaluated later > // in the loop. i'm not clear on why a node (besides the root) would not have a parent

update to the trunk

fixed. Checking in browser/components/places/content/controller.js; /cvsroot/mozilla/browser/components/places/content/controller.js,v <-- control ler.js new revision: 1.173; previous revision: 1.172 done Checking in browser/components/places/content/history-panel.js; /cvsroot/mozilla/browser/components/places/content/history-panel.js,v <-- hist ory-panel.js new revision: 1.22; previous revision: 1.21 done Checking in toolkit/components/places/src/nsNavHistoryResult.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v <-- ns NavHistoryResult.cpp new revision: 1.110; previous revision: 1.109 done

Seth, do you have a places.sqlite that displays this problem? I haven't been able to reproduce this from the description here.

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